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

[Junio C Hamano <gitster@pobox.com>]

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).

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.

>> +	else
>> +		BUG("parse_one_signature() returned unknown hash algo");
>
> I think we should not label this a bug. It is feasible that we introduce
> a third hash algorithm in the future that we don't know to handle yet,
> but that would not be a programming bug but a normal error. So we should
> probably `die()` instead.

Good thinking.