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

From: Christian Couder <christian.couder@gmail.com>
Date: Sep 12, 2025, 13:47 UTC
Message-ID: <CAP8UFD1ULfGqzQwn1ZhgCdS5Ri_Y3rd-F+=-zsRKtKFg0k4hVQ@mail.gmail.com>
In-Reply-To: <xmqq4it9ornz.fsf@gitster.g>

On Thu, Sep 11, 2025 at 6:55 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

>
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> +/* Process signatures (up to 2: one "sha1" and one "sha256") */
> >
> > Hm. Does "up to 2" indicate that the commit may have two signatures at
> > once? If so...
> >
> >> +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");
> >
> > ... then the code here seems to indicate otherwise as you only parse
> > either the "sha1" signature or the "sha256" signature, but never both.
>
> Correct and not quite.  The caller can call you twice in its loop.
> But if the input was malformed and had two "sha1" (and no "sha256"),
> this will not barf (as the original, so it is not a new bug).

store_signature() should barf if it has already been called with the same first argument:

static void store_signature(struct signature_data *stored_sig,
                struct signature_data *new_sig,
                const char *hash_type)
{
    if (stored_sig->hash_algo) {
        warning("multiple %s signatures found, "
            "ignoring additional signature",
            hash_type);
        strbuf_release(&new_sig->data);
        free(new_sig->hash_algo);
    } else {
        *stored_sig = *new_sig;
    }
}

> In any case, I also found that "up to 2" comment somewhat strange.
> It was more understandable back when it was near the loop, but not
> here.

In V2 I just removed that comment, as I am not sure it really helps.

Thanks.