Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option

>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> > @@ -2785,6 +2787,23 @@ static void store_signature(struct signature_data *stored_sig,
> >       }
> >  }
> >
> > +/* Process signatures (up to 2: one "sha1" and one "sha256") */
> > +static void import_signature(struct signature_data *sig_sha1,
> > +                          struct signature_data *sig_sha256,
> > +                          const char *v)
> > +{
> > +     struct signature_data sig = { NULL, NULL, STRBUF_INIT };
> > +
> > +     parse_one_signature(&sig, v);
> > +
> > +     if (!strcmp(sig.hash_algo, "sha1"))
> > +             store_signature(sig_sha1, &sig, "SHA-1");
> > +     else if (!strcmp(sig.hash_algo, "sha256"))
> > +             store_signature(sig_sha256, &sig, "SHA-256");
> > +     else
> > +             BUG("parse_one_signature() returned unknown hash algo");
> > +}
> > +
>
> THis is a new function; I am not sure if the division of labor
> between this one and its caller is done right.  See below.

> > @@ -2817,19 +2836,28 @@ static void parse_new_commit(const char *arg)
> >       if (!committer)
> >               die("Expected committer but didn't get one");
> >
> > -     /* Process signatures (up to 2: one "sha1" and one "sha256") */
> >       while (skip_prefix(command_buf.buf, "gpgsig ", &v)) {
> > -             struct signature_data sig = { NULL, NULL, STRBUF_INIT };
> > -
> > -             parse_one_signature(&sig, v);
> > -
> > -             if (!strcmp(sig.hash_algo, "sha1"))
> > -                     store_signature(&sig_sha1, &sig, "SHA-1");
> > -             else if (!strcmp(sig.hash_algo, "sha256"))
> > -                     store_signature(&sig_sha256, &sig, "SHA-256");
> > -             else
> > -                     BUG("parse_one_signature() returned unknown hash algo");
> > -
> > +             struct strbuf data = STRBUF_INIT;
> > +             switch (signed_commit_mode) {
> > +             case SIGN_ABORT:
> > +                     die("encountered signed commit; use "
> > +                         "--signed-commits=<mode> to handle it");
> > +             case SIGN_WARN_VERBATIM:
> > +                     warning("importing a commit signature");
> > +                     /* fallthru */
> > +             case SIGN_VERBATIM:
> > +                     import_signature(&sig_sha1, &sig_sha256, v);
> > +                     break;
> > +             case SIGN_WARN_STRIP:
> > +                     warning("stripping a commit signature");
> > +                     /* fallthru */
> > +             case SIGN_STRIP:
> > +                     /* Read signature data and discard it */
> > +                     read_next_command();
> > +                     parse_data(&data, 0, NULL);
> > +                     strbuf_release(&data);
> > +                     break;
> > +             }
> >               read_next_command();
> >       }
>
> I am not sure if this change had to be this way.  The old code
> always called parse_one_signature(), which was responsible for
> checking the signature format and then calling read_next_command()
> and parse_data(), so no matter what happened afterwards, we know we
> are consuming the data stream regardless of the conditional execution
> that happens here.
>
> The new code calls import_signature() or an inlined sequence of
> read_next_command() plus parse_data(), essentially making each case
> arm in the switch() statement responsible individually for consuming
> the incoming data.  When somebody adds a new case there to specify a
> different way to handle signatures, they have to make sure that they
> do not forget calling read_next_command() and parse_data() themselves.
>
> Even though I can see, after some code inspection, that no branches
> in the current code forgets to advance the incoming data stream to
> leave us out of sync right now, this change feels like a bit of code
> ergonomics regression to me.  Was it so important that we pass a
> broken signature without inspecting in STRIP mode?  I am guessing
> that is the reason why the new code tries hard to avoid calling the
> parse_one_signature() function in these case arms.