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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2025, 17:09 UTC
Message-ID: <xmqqtt1as08f.fsf@gitster.g>
In-Reply-To: <20250910080839.2142651-3-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 22 quoted lines

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

Show 40 quoted lines

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

An aside. I think the warning message about importing should have a word "verbatim" in it, e.g.

	warning("importing a commit signature verbatim");

Thanks.