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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Sep 10, 2025 at 10:08:39AM +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
> @@ -66,6 +66,11 @@ OPTIONS
>  	remote-helpers that use the `import` capability, as they are
>  	already trusted to run their own code.
>  
> +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> +	Specify how to handle signed commits.  Behaves in the same way
> +	as the same option in linkgit:git-fast-export[1], except that
> +	default is 'verbatim' (instead of 'abort').

We could of course extract the description from git-fast-export(1) and
move it into a shared file so that we can include it from both commands.
Not sure whether that's worth it though.

> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 2c35f9345d..f932dd2c65 100644
> --- 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") */

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.

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

> @@ -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") */

Aha, this is where the comment comes from! Here it makes sense as we
have a loop, but it doesn't really feel sensible for the extracted
function.

>  	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");

And the call to `BUG()` is preexisting, as well. How about we move the
extraction of this loop into a separate commit?

> +		struct strbuf data = STRBUF_INIT;
> +		switch (signed_commit_mode) {
> +		case SIGN_ABORT:
> +			die("encountered signed commit; use "
> +			    "--signed-commits=<mode> to handle it");

This message should be marked for translation.

> +		case SIGN_WARN_VERBATIM:
> +			warning("importing a commit signature");
> +			/* fallthru */

This and the other warning should be marked for translation.

> +		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();
>  	}
>  
> @@ -3501,6 +3529,9 @@ static int parse_one_option(const char *option)
>  		option_active_branches(option);
>  	} else if (skip_prefix(option, "export-pack-edges=", &option)) {
>  		option_export_pack_edges(option);
> +	} else if (skip_prefix(option, "signed-commits=", &option)) {
> +		if (parse_sign_mode(option, &signed_commit_mode))
> +			die("unknown --signed-commits mode '%s'", option);

Do we want to use `usagef()` instead?

> diff --git a/t/meson.build b/t/meson.build
> index 82af229be3..08ad6938e2 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -1032,6 +1032,7 @@ integration_tests = [
>    't9302-fast-import-unpack-limit.sh',
>    't9303-fast-import-compression.sh',
>    't9304-fast-import-marks.sh',
> +  't9305-fast-import-signatures.sh',
>    't9350-fast-export.sh',
>    't9351-fast-export-anonymize.sh',
>    't9400-git-cvsserver-server.sh',
> diff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh
> new file mode 100755
> index 0000000000..5a52691b29
> --- /dev/null
> +++ b/t/t9305-fast-import-signatures.sh
> @@ -0,0 +1,108 @@
> +#!/bin/sh
> +
> +test_description='git fast-import --signed-commits=<mode>'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

There shouldn't be a need to specify the initial branch name. You
already create the initial commit with `test_commit()`, so the calls to
git-checkout(1) can instead say `git checkout -b openpgp-signign first`
because `test_commit()` creates that tag for us.

> +. ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-gpg.sh"
> +
> +test_expect_success 'set up unsigned initial commit and import repo' '
> +	test_commit first &&
> +	mkdir new &&
> +	git --git-dir=new/.git init

Hm. Why don't we just say `git init new` or `git init --bare new`? I
might be missing something here.

[snip]
> +test_expect_success GPG 'setup a commit with dual OpenPGP signatures on its SHA-1 and SHA-256 formats' '
> +	# Create a signed SHA-256 commit
> +	git init --object-format=sha256 explicit-sha256 &&
> +	git -C explicit-sha256 config extensions.compatObjectFormat sha1 &&
> +	git -C explicit-sha256 checkout -b dual-signed &&
> +	test_commit -C explicit-sha256 A &&
> +	echo B >explicit-sha256/B &&
> +	git -C explicit-sha256 add B &&
> +	test_tick &&
> +	git -C explicit-sha256 commit -S -m "signed" B &&
> +	SHA256_B=$(git -C explicit-sha256 rev-parse dual-signed) &&
> +
> +	# Create the corresponding SHA-1 commit
> +	SHA1_B=$(git -C explicit-sha256 rev-parse --output-object-format=sha1 dual-signed) &&
> +
> +	# Check that the resulting SHA-1 commit has both signatures
> +	echo $SHA1_B | git -C explicit-sha256 cat-file --batch >out &&

You can instead `git -c explicit-sha256 cat-file -p $SHA1_B >out`.

Patrick