From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 15 Sep 2025 06:27:31 GMT
Subject: Re: [PATCH v2 2/2] fast-import: add '--signed-commits=<mode>' option
Message-ID: <aMex06JNPVeHaw_d@pks.im>
In-Reply-To: <20250912124042.2523683-3-christian.couder@gmail.com>

On Fri, Sep 12, 2025 at 02:40:42PM +0200, Christian Couder wrote:
> diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
> index 3144ffcdb6..90f242d058 100644
> --- a/Documentation/git-fast-import.adoc
> +++ b/Documentation/git-fast-import.adoc
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 2c35f9345d..890f05de4d 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -2817,19 +2819,39 @@ 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 (signed_commit_mode == SIGN_ABORT)
> +			die(_("encountered signed commit; use "
> +			      "--signed-commits=<mode> to handle it"));
>  
> -		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");
> +		parse_one_signature(&sig, v);
>  
> +		switch (signed_commit_mode) {
> +		case SIGN_ABORT:
> +			BUG("SIGN_ABORT should be handled before calling parse_one_signature()");
> +			break;

Let's be defensive and convert this into a `default:` case so that any
unhandled value will cause a BUG.

> +		case SIGN_WARN_VERBATIM:
> +			warning(_("importing a commit signature verbatim"));
> +			/* fallthru */
> +		case SIGN_VERBATIM:
> +			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
> +				die(_("parse_one_signature() returned unknown hash algo"));
> +			break;
> +		case SIGN_WARN_STRIP:
> +			warning(_("stripping a commit signature"));
> +			/* fallthru */
> +		case SIGN_STRIP:
> +			/* Just discard signature data */
> +			strbuf_release(&sig.data);
> +			free(sig.hash_algo);
> +			break;
> +		}
>  		read_next_command();
>  	}
>  

Other than that the patch looks good to me, thanks!

Patrick