On Wed, Sep 10, 2025 at 7:09 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> 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.
I have removed this new function in V2 as I think I implemented what
you suggest 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.
Yeah, I thought it was cleaner and a bit faster if we don't parse
signatures when in STRIP mode. That's why I did it like this.
Now as it looks like we don't really mind parsing them, I implemented
that in V2. In ABORT mode, I still think it might be a bit better to
ABORT right away though, so that's what I implemented in V2.
> An aside. I think the warning message about importing should have a
> word "verbatim" in it, e.g.
>
> warning("importing a commit signature verbatim");
In V2, I have changed the message to what you suggest.
