[PATCH 0/2] fast-import: start controlling how commit signatures are handled

[PATCH 0/2] fast-import: start controlling how commit signatures are handled

[Christian Couder]

Tools like `git-filter-repo` should be able to control how commit
signatures are handled when regenerating repository content after it
has been filtered (see
https://github.com/newren/git-filter-repo/issues/139). For this
purpose, they need a way for `git fast-import` to control how commit
signatures are handled.

This small patch series starts to implement such a way by adding a new
`--signed-commits=<mode>` option to `git fast-import`.

For now this new option behaves in a very similar way as the option
with the same name that already exists in `git fast-export`.
Especially it supports exactly the same <mode>s and the same aliases
for these modes. For example "ignore" is a synonym for "verbatim".

In the future I want to implement new <mode>s like "strip-if-invalid",
"re-sign", "re-sign-if-invalid" that might be a bit more complex for
this option. But for now I prefer to start with the simple modes to
validate the general design of the new option.

In particular, I am interested in feedback about the following:

  - Should we keep "ignore" as a synonym for "verbatim" and "warn" as
    a synonym for "warn-verbatim"? My opinion is that they might be
    confusing, so we might want to remove them for `git fast-import`
    even if we keep them for `git fast-export`. The parsing code might
    be a bit more complex if we do that though, so for now I have kept
    the synonyms.

  - Are we still fine with most <mode>s having a "warn-*" variant
    (like the "warn-strip" variant of "strip" for example)? Or should
    we have a separate `--verbose` or maybe `--signed-commits-verbose`
    option dedicated to switching warnings on/off? I think it's good
    to decide about this before the number of <mode>s increases a lot
    with new <mode>s like "strip-if-invalid", "re-sign",
    "re-sign-if-invalid" and possibly others.

CI tests:

They have all passed. See:

https://github.com/chriscool/git/actions/runs/17606244845/job/50017575843


Christian Couder (2):
  gpg-interface: refactor 'enum sign_mode' parsing
  fast-import: add '--signed-commits=<mode>' option

 Documentation/git-fast-import.adoc |   5 ++
 builtin/fast-export.c              |  23 ++----
 builtin/fast-import.c              |  55 +++++++++++----
 gpg-interface.c                    |  17 +++++
 gpg-interface.h                    |  15 ++++
 t/meson.build                      |   1 +
 t/t9305-fast-import-signatures.sh  | 108 +++++++++++++++++++++++++++++
 7 files changed, 196 insertions(+), 28 deletions(-)
 create mode 100755 t/t9305-fast-import-signatures.sh

-- 
2.51.0.195.g61112aeac3

[PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing

[Christian Couder]

The definition of 'enum sign_mode' as well as its parsing code are in
"builtin/fast-export.c". This was fine because `git fast-export` was the
only command with '--signed-tags=<mode>' or '--signed-commits=<mode>'
options.

In a following commit, we are going to add such options to `git
fast-import`, which will be simpler, easier and cleaner if we can reuse
the 'enum sign_mode' defintion and parsing code.

So let's move that definition and parsing code from
"builtin/fast-export.c" to "gpg-interface.{c,h}".

While at it, let's fix a small indentation issue with the arguments of
parse_opt_sign_mode().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/fast-export.c | 23 +++++++----------------
 gpg-interface.c       | 17 +++++++++++++++++
 gpg-interface.h       | 15 +++++++++++++++
 3 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index c06ee0b213..3994a8f898 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -37,8 +37,6 @@ static const char *const fast_export_usage[] = {
 	NULL
 };
 
-enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_WARN_VERBATIM, SIGN_WARN_STRIP };
-
 static int progress;
 static enum sign_mode signed_tag_mode = SIGN_ABORT;
 static enum sign_mode signed_commit_mode = SIGN_STRIP;
@@ -59,24 +57,17 @@ static struct hashmap anonymized_seeds;
 static struct revision_sources revision_sources;
 
 static int parse_opt_sign_mode(const struct option *opt,
-				     const char *arg, int unset)
+			       const char *arg, int unset)
 {
 	enum sign_mode *val = opt->value;
+
 	if (unset)
 		return 0;
-	else if (!strcmp(arg, "abort"))
-		*val = SIGN_ABORT;
-	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
-		*val = SIGN_VERBATIM;
-	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
-		*val = SIGN_WARN_VERBATIM;
-	else if (!strcmp(arg, "warn-strip"))
-		*val = SIGN_WARN_STRIP;
-	else if (!strcmp(arg, "strip"))
-		*val = SIGN_STRIP;
-	else
-		return error("Unknown %s mode: %s", opt->long_name, arg);
-	return 0;
+
+	if (!parse_sign_mode(arg, val))
+		return 0;
+
+	return error("Unknown %s mode: %s", opt->long_name, arg);
 }
 
 static int parse_opt_tag_of_filtered_mode(const struct option *opt,
diff --git a/gpg-interface.c b/gpg-interface.c
index 06e7fb5060..2f4f0e32cb 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1125,3 +1125,20 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 	FREE_AND_NULL(ssh_signing_key_file);
 	return ret;
 }
+
+int parse_sign_mode(const char *arg, enum sign_mode *mode)
+{
+	if (!strcmp(arg, "abort"))
+		*mode = SIGN_ABORT;
+	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
+		*mode = SIGN_VERBATIM;
+	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
+		*mode = SIGN_WARN_VERBATIM;
+	else if (!strcmp(arg, "warn-strip"))
+		*mode = SIGN_WARN_STRIP;
+	else if (!strcmp(arg, "strip"))
+		*mode = SIGN_STRIP;
+	else
+		return -1;
+	return 0;
+}
diff --git a/gpg-interface.h b/gpg-interface.h
index 60ddf8bbfa..44856cc55f 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -104,4 +104,19 @@ int check_signature(struct signature_check *sigc,
 void print_signature_buffer(const struct signature_check *sigc,
 			    unsigned flags);
 
+/* Modes for --signed-tags=<mode> and --signed-commits=<mode> options */
+enum sign_mode {
+	SIGN_ABORT,
+	SIGN_WARN_VERBATIM,
+	SIGN_VERBATIM,
+	SIGN_WARN_STRIP,
+	SIGN_STRIP,
+};
+
+/*
+ * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1
+ * otherwise.
+ */
+int parse_sign_mode(const char *arg, enum sign_mode *mode);
+
 #endif
-- 
2.51.0.195.g61112aeac3

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

[Christian Couder]

A '--signed-commits=<mode>' option is already available when using
`git fast-export` to decide what should be done at export time about
commit signatures. At import time though, there is no option, or
other way, in `git fast-import` to decide about commit signatures.

To remediate that, let's add a '--signed-commits=<mode>' option to
`git fast-import` too.

For now the supported <mode>s are the same as those supported by
`git fast-export`.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-fast-import.adoc |   5 ++
 builtin/fast-import.c              |  55 +++++++++++----
 t/meson.build                      |   1 +
 t/t9305-fast-import-signatures.sh  | 108 +++++++++++++++++++++++++++++
 4 files changed, 157 insertions(+), 12 deletions(-)
 create mode 100755 t/t9305-fast-import-signatures.sh

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').
+
 Options for Frontends
 ~~~~~~~~~~~~~~~~~~~~~
 
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
@@ -188,6 +188,8 @@ static int global_argc;
 static const char **global_argv;
 static const char *global_prefix;
 
+static enum sign_mode signed_commit_mode = SIGN_VERBATIM;
+
 /* Memory pools */
 static struct mem_pool fi_mem_pool = {
 	.block_alloc = 2*1024*1024 - sizeof(struct mp_block),
@@ -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");
+}
+
 static void parse_new_commit(const char *arg)
 {
 	static struct strbuf msg = STRBUF_INIT;
@@ -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();
 	}
 
@@ -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);
 	} else if (!strcmp(option, "quiet")) {
 		show_stats = 0;
 		quiet = 1;
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
+
+. ./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
+'
+
+test_expect_success GPG 'set up OpenPGP signed commit' '
+	git checkout -b openpgp-signing main &&
+	echo "Content for OpenPGP signing." >file-sign &&
+	git add file-sign &&
+	git commit -S -m "OpenPGP signed commit" &&
+	OPENPGP_SIGNING=$(git rev-parse --verify openpgp-signing)
+'
+
+test_expect_success GPG 'import OpenPGP signature with --signed-commits=verbatim' '
+	git fast-export --signed-commits=verbatim openpgp-signing >output &&
+	git -C new fast-import --quiet --signed-commits=verbatim <output >log 2>&1 &&
+	IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) &&
+	test $OPENPGP_SIGNING = $IMPORTED &&
+	test_must_be_empty log
+'
+
+test_expect_success GPGSM 'set up X.509 signed commit' '
+	git checkout -b x509-signing main &&
+	test_config gpg.format x509 &&
+	test_config user.signingkey $GIT_COMMITTER_EMAIL &&
+	echo "Content for X.509 signing." >file-sign &&
+	git add file-sign &&
+	git commit -S -m "X.509 signed commit" &&
+	X509_SIGNING=$(git rev-parse HEAD)
+'
+
+test_expect_success GPGSM 'import X.509 signature fails with --signed-commits=abort' '
+	git fast-export --signed-commits=verbatim x509-signing >output &&
+	test_must_fail git -C new fast-import --quiet --signed-commits=abort <output
+'
+
+test_expect_success GPGSM 'import X.509 signature with --signed-commits=warn-verbatim' '
+	git -C new fast-import --quiet --signed-commits=warn-verbatim <output >log 2>&1 &&
+	IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) &&
+	test $X509_SIGNING = $IMPORTED &&
+	test_grep "importing a commit signature" log
+'
+
+test_expect_success GPGSSH 'set up SSH signed commit' '
+	git checkout -b ssh-signing main &&
+	test_config gpg.format ssh &&
+	test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" &&
+	echo "Content for SSH signing." >file-sign &&
+	git add file-sign &&
+	git commit -S -m "SSH signed commit" &&
+	SSH_SIGNING=$(git rev-parse HEAD)
+'
+
+test_expect_success GPGSSH 'strip SSH signature with --signed-commits=strip' '
+	git fast-export --signed-commits=verbatim ssh-signing >output &&
+	git -C new fast-import --quiet --signed-commits=strip <output >log 2>&1 &&
+	IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) &&
+	test $SSH_SIGNING != $IMPORTED &&
+	git -C new cat-file commit "$IMPORTED" >actual &&
+	test_grep ! -E "^gpgsig" actual &&
+	test_must_be_empty log
+'
+
+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 &&
+	test_grep -E "^gpgsig " out &&
+	test_grep -E "^gpgsig-sha256 " out
+'
+
+test_expect_success GPG 'strip both OpenPGP signatures with --signed-commits=warn-strip' '
+	git -C explicit-sha256 fast-export --signed-commits=verbatim dual-signed >output &&
+	test_grep -E "^gpgsig sha1 openpgp" output &&
+	test_grep -E "^gpgsig sha256 openpgp" output &&
+	git -C new fast-import --quiet --signed-commits=warn-strip <output >log 2>&1 &&
+	git -C new cat-file commit refs/heads/dual-signed >actual &&
+	test_grep ! -E "^gpgsig " actual &&
+	test_grep ! -E "^gpgsig-sha256 " actual &&
+	test_grep "stripping a commit signature" log >out &&
+	test_line_count = 2 out
+'
+
+test_done
-- 
2.51.0.195.g61112aeac3

Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing

[Junio C Hamano]

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

> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index c06ee0b213..3994a8f898 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -37,8 +37,6 @@ static const char *const fast_export_usage[] = {
>  	NULL
>  };
>  
> -enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_WARN_VERBATIM, SIGN_WARN_STRIP };
> -
> ...
>  static int parse_opt_sign_mode(const struct option *opt,
> -				     const char *arg, int unset)
> +			       const char *arg, int unset)
>  {
>  	enum sign_mode *val = opt->value;
> +
>  	if (unset)
>  		return 0;
> -	else if (!strcmp(arg, "abort"))
> -		*val = SIGN_ABORT;
> -	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> -		*val = SIGN_VERBATIM;
> -	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
> -		*val = SIGN_WARN_VERBATIM;
> -	else if (!strcmp(arg, "warn-strip"))
> -		*val = SIGN_WARN_STRIP;
> -	else if (!strcmp(arg, "strip"))
> -		*val = SIGN_STRIP;
> -	else
> -		return error("Unknown %s mode: %s", opt->long_name, arg);
> -	return 0;
> +
> +	if (!parse_sign_mode(arg, val))
> +		return 0;
> +
> +	return error("Unknown %s mode: %s", opt->long_name, arg);
>  }

I am not sure if it is correct for the "unset" codepath doing
nothing on *val before returning, but changing that is of course not
in scope of this patch at all.  And the result of the change in this
function is very pleasing to read.  And ...

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 06e7fb5060..2f4f0e32cb 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -1125,3 +1125,20 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
> ...
> +int parse_sign_mode(const char *arg, enum sign_mode *mode)
> +{
> +	if (!strcmp(arg, "abort"))
> +		*mode = SIGN_ABORT;
> +	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> +		*mode = SIGN_VERBATIM;
> +	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
> +		*mode = SIGN_WARN_VERBATIM;
> +	else if (!strcmp(arg, "warn-strip"))
> +		*mode = SIGN_WARN_STRIP;
> +	else if (!strcmp(arg, "strip"))
> +		*mode = SIGN_STRIP;
> +	else
> +		return -1;
> +	return 0;
> +}

... this is of course very much expected.

> diff --git a/gpg-interface.h b/gpg-interface.h
> index 60ddf8bbfa..44856cc55f 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -104,4 +104,19 @@ int check_signature(struct signature_check *sigc,
>  void print_signature_buffer(const struct signature_check *sigc,
>  			    unsigned flags);
>  
> +/* Modes for --signed-tags=<mode> and --signed-commits=<mode> options */
> +enum sign_mode {
> +	SIGN_ABORT,
> +	SIGN_WARN_VERBATIM,
> +	SIGN_VERBATIM,
> +	SIGN_WARN_STRIP,
> +	SIGN_STRIP,
> +};

And this is much easier to read in the header file for all to see.

> +/*
> + * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1
> + * otherwise.
> + */
> +int parse_sign_mode(const char *arg, enum sign_mode *mode);
> +

Nice.

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

[Junio C Hamano]

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

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

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

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

[Junio C Hamano]

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

> A '--signed-commits=<mode>' option is already available when using
> `git fast-export` to decide what should be done at export time about
> commit signatures. At import time though, there is no option, or
> other way, in `git fast-import` to decide about commit signatures.
>
> To remediate that, let's add a '--signed-commits=<mode>' option to
> `git fast-import` too.
>
> For now the supported <mode>s are the same as those supported by
> `git fast-export`.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/git-fast-import.adoc |   5 ++
>  builtin/fast-import.c              |  55 +++++++++++----
>  t/meson.build                      |   1 +
>  t/t9305-fast-import-signatures.sh  | 108 +++++++++++++++++++++++++++++
>  4 files changed, 157 insertions(+), 12 deletions(-)
>  create mode 100755 t/t9305-fast-import-signatures.sh
>
> 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').

I forgot one thing.  Earlier in 1/2 we saw

    The definition of 'enum sign_mode' as well as its parsing code are in
    "builtin/fast-export.c". This was fine because `git fast-export` was the
    only command with '--signed-tags=<mode>' or '--signed-commits=<mode>'
    options.

    In a following commit, we are going to add such options to `git
    fast-import`, which will be simpler, easier and cleaner if we can reuse
    the 'enum sign_mode' defintion and parsing code.

and I was implicitly expecting that both commits and tags would be
supported on the import side, but this only deals with the commits?

Is patch [3/2] missing from the archive?

Thanks.

Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing

[Patrick Steinhardt]

On Wed, Sep 10, 2025 at 10:08:38AM +0200, Christian Couder wrote:
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index c06ee0b213..3994a8f898 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -59,24 +57,17 @@ static struct hashmap anonymized_seeds;
>  static struct revision_sources revision_sources;
>  
>  static int parse_opt_sign_mode(const struct option *opt,
> -				     const char *arg, int unset)
> +			       const char *arg, int unset)
>  {
>  	enum sign_mode *val = opt->value;
> +
>  	if (unset)
>  		return 0;
> -	else if (!strcmp(arg, "abort"))
> -		*val = SIGN_ABORT;
> -	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
> -		*val = SIGN_VERBATIM;
> -	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
> -		*val = SIGN_WARN_VERBATIM;
> -	else if (!strcmp(arg, "warn-strip"))
> -		*val = SIGN_WARN_STRIP;
> -	else if (!strcmp(arg, "strip"))
> -		*val = SIGN_STRIP;
> -	else
> -		return error("Unknown %s mode: %s", opt->long_name, arg);
> -	return 0;
> +
> +	if (!parse_sign_mode(arg, val))
> +		return 0;
> +
> +	return error("Unknown %s mode: %s", opt->long_name, arg);

Would it make sense to maybe reverse the error handling and say
something like:

    if (parse_sign_mode(arg, val) < 0)
        return error("Unknown %s mode: %s", opt->long_name, arg);

    return 0;

That reads a bit more natural to me at least.

> diff --git a/gpg-interface.h b/gpg-interface.h
> index 60ddf8bbfa..44856cc55f 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -104,4 +104,19 @@ int check_signature(struct signature_check *sigc,
>  void print_signature_buffer(const struct signature_check *sigc,
>  			    unsigned flags);
>  
> +/* Modes for --signed-tags=<mode> and --signed-commits=<mode> options */

Nit: let's finish this sentence with a dot.

> +enum sign_mode {
> +	SIGN_ABORT,
> +	SIGN_WARN_VERBATIM,
> +	SIGN_VERBATIM,
> +	SIGN_WARN_STRIP,
> +	SIGN_STRIP,
> +};
> +
> +/*
> + * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1
> + * otherwise.
> + */
> +int parse_sign_mode(const char *arg, enum sign_mode *mode);

Okay, makes sense.

Patrick

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

[Patrick Steinhardt]

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

Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> +	if (!parse_sign_mode(arg, val))
>> +		return 0;
>> +
>> +	return error("Unknown %s mode: %s", opt->long_name, arg);
>
> Would it make sense to maybe reverse the error handling and say
> something like:
>
>     if (parse_sign_mode(arg, val) < 0)
>         return error("Unknown %s mode: %s", opt->long_name, arg);
>
>     return 0;
>
> That reads a bit more natural to me at least.

I also thought that the original had a "Huh?" factor, but my
reaction was more like "wouldn't it be easier to see if these are
both sides of if/else?", i.e.

	if (parse_sign_mode(...))
		return error(...);
	else
		return 0;

which made me think that the differences were mostly subjective and
refrained from commenting.

But after I see your version, I tend to agree that it is easier to
see the flow if the most-straightforward success case ran through to
the end, while exceptional cases (i.e. an error return, or the
initial "unset" case) did early returns.  I have to add, however,
that I feel that this is mostly subjective and falls into "once it
is in the tree, it's not really worth the patch noise to go and fix
it up" category.  If we will have v2 of this series, I do prefer to
see it written more in your way, though.

Thanks.

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

[Junio C Hamano]

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.

Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing

[Christian Couder]

On Thu, Sep 11, 2025 at 6:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> +    if (!parse_sign_mode(arg, val))
> >> +            return 0;
> >> +
> >> +    return error("Unknown %s mode: %s", opt->long_name, arg);
> >
> > Would it make sense to maybe reverse the error handling and say
> > something like:
> >
> >     if (parse_sign_mode(arg, val) < 0)
> >         return error("Unknown %s mode: %s", opt->long_name, arg);
> >
> >     return 0;
> >
> > That reads a bit more natural to me at least.

Before the above part of the code, there is:

    if (unset)
        return 0;

So it felt natural to me to have a flow where we continue to return 0
(success) if we can and go on otherwise.

> I also thought that the original had a "Huh?" factor, but my
> reaction was more like "wouldn't it be easier to see if these are
> both sides of if/else?", i.e.
>
>         if (parse_sign_mode(...))
>                 return error(...);
>         else
>                 return 0;
>
> which made me think that the differences were mostly subjective and
> refrained from commenting.

I agree that the difference is very subjective. If there was something
like for example:

```
if (A)
       return 0;

if (B)
       return 0;

...

if (F)
       return 0;

return -1
```

It would feel strange if the function ended instead with:

```
if (!F)
       return -1;

return 0;
```

> But after I see your version, I tend to agree that it is easier to
> see the flow if the most-straightforward success case ran through to
> the end, while exceptional cases (i.e. an error return, or the
> initial "unset" case) did early returns.  I have to add, however,
> that I feel that this is mostly subjective and falls into "once it
> is in the tree, it's not really worth the patch noise to go and fix
> it up" category.  If we will have v2 of this series, I do prefer to
> see it written more in your way, though.

Fine, I have written it in the way Patrick suggested in V2.

Thanks.

Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing

[Christian Couder]

On Thu, Sep 11, 2025 at 8:06 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Wed, Sep 10, 2025 at 10:08:38AM +0200, Christian Couder wrote:

> > diff --git a/gpg-interface.h b/gpg-interface.h
> > index 60ddf8bbfa..44856cc55f 100644
> > --- a/gpg-interface.h
> > +++ b/gpg-interface.h
> > @@ -104,4 +104,19 @@ int check_signature(struct signature_check *sigc,
> >  void print_signature_buffer(const struct signature_check *sigc,
> >                           unsigned flags);
> >
> > +/* Modes for --signed-tags=<mode> and --signed-commits=<mode> options */
>
> Nit: let's finish this sentence with a dot.

I have added a dot in the V2.

Thanks.

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

[Christian Couder]

On Thu, Sep 11, 2025 at 8:06 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> 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.

When I add more options, I plan to improve on that doc, but for now I
think it's Ok.

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

I changed it to die() in V2.

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

Right, I have removed the comment altogether in V2.

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

There is no extraction of this code anymore in V2.

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

Only 6 out of 131 messages in die() functions are currently marked for
translation. So I thought that it might be better to mark all messages
for translations in a separate series dedicated to that.

Anyway in V2, all the messages in die(), warning() and such introduced
by this series are marked for translation.

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

Ok, it's used in V2.

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

I copy pasted a lot of test code from t9350, but yeah in V2 I fixed
this and the other issues you mentioned in this new test script.

Thanks.

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

[Christian Couder]

On Wed, Sep 10, 2025 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > 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.

I have removed this new function in V2 as I think I implemented what
you suggest below.

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

Yeah, I thought it was cleaner and a bit faster if we don't parse
signatures when in STRIP mode. That's why I did it like this.

Now as it looks like we don't really mind parsing them, I implemented
that in V2. In ABORT mode, I still think it might be a bit better to
ABORT right away though, so that's what I implemented in V2.

> An aside.  I think the warning message about importing should have a
> word "verbatim" in it, e.g.
>
>         warning("importing a commit signature verbatim");

In V2, I have changed the message to what you suggest.

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

[Christian Couder]

On Wed, Sep 10, 2025 at 8:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > +--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').
>
> I forgot one thing.  Earlier in 1/2 we saw
>
>     The definition of 'enum sign_mode' as well as its parsing code are in
>     "builtin/fast-export.c". This was fine because `git fast-export` was the
>     only command with '--signed-tags=<mode>' or '--signed-commits=<mode>'
>     options.
>
>     In a following commit, we are going to add such options to `git
>     fast-import`, which will be simpler, easier and cleaner if we can reuse
>     the 'enum sign_mode' defintion and parsing code.
>
> and I was implicitly expecting that both commits and tags would be
> supported on the import side, but this only deals with the commits?
>
> Is patch [3/2] missing from the archive?

Yeah, the commit message was too optimistic about the features that
were going to be added by the next commit.

In V2, I have changed that paragraph in the commit message to:

   "In a following commit, we are going to add a similar option to `git
   fast-import`, which will be simpler, easier and cleaner if we can reuse
   the 'enum sign_mode' defintion and parsing code."

And in the cover letter I added the following:

"In the future I also plan to add a similar `--signed-tags=<mode>` so
that the import of tags can also be controlled. But I prefer to
validate the general design of a single new option first."

I hope it's Ok.

Thanks.

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

[Christian Couder]

On Thu, Sep 11, 2025 at 6:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

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

[Junio C Hamano]

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

> In V2, I have changed that paragraph in the commit message to:
>
>    "In a following commit, we are going to add a similar option to `git
>    fast-import`, which will be simpler, easier and cleaner if we can reuse
>    the 'enum sign_mode' defintion and parsing code."
>
> And in the cover letter I added the following:
>
> "In the future I also plan to add a similar `--signed-tags=<mode>` so
> that the import of tags can also be controlled. But I prefer to
> validate the general design of a single new option first."

I understood the intentions.

I am not sure if omitting signed-tags and doing only signed-commits
is a sensible choice, as it does not seem to reduce the patch load
any meaningful way.  Besides, because historically signed tags are
more heavily used than signed commits, if it made sense to do only
one of two, I would have somehow expected that tags would be done
first.

But anyway, I understood why this was done only half of the scope as
a first step.

Thanks.

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

[Junio C Hamano]

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

>> ...  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.
>
> Yeah, I thought it was cleaner and a bit faster if we don't parse
> signatures when in STRIP mode. That's why I did it like this.

I do not think parsing performance matters all that much here, but I
think it is a good idea to recover from malformed signature lines if
the parsing code can detect some.  It is likely that the user may be
using STRIP hoping that they can skip bad or unknown signature data
in the incoming stream, so it is beneficial to be lenient there.

> Now as it looks like we don't really mind parsing them, I implemented
> that in V2. In ABORT mode, I still think it might be a bit better to
> ABORT right away though, so that's what I implemented in V2.
>
>> An aside.  I think the warning message about importing should have a
>> word "verbatim" in it, e.g.
>>
>>         warning("importing a commit signature verbatim");
>
> In V2, I have changed the message to what you suggest.

Sounds good.

Thanks.

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

[Christian Couder]

On Fri, Sep 12, 2025 at 4:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> >> ...  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.
> >
> > Yeah, I thought it was cleaner and a bit faster if we don't parse
> > signatures when in STRIP mode. That's why I did it like this.
>
> I do not think parsing performance matters all that much here, but I
> think it is a good idea to recover from malformed signature lines if
> the parsing code can detect some.  It is likely that the user may be
> using STRIP hoping that they can skip bad or unknown signature data
> in the incoming stream, so it is beneficial to be lenient there.

Yeah, parse_one_signature() expects a signature to be in the 'gpgsig
<hash-algo> <signature-format>' and might die() in case it cannot
validate <hash-algo> or <signature-format>.

So it seems to me that I should change the code back so that its
behavior is the same as in v1, while trying to avoid what you called
code ergonomics regression when you reviewed v1. Ok, let's see if I
find a way to do that.

Thanks.

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

[Junio C Hamano]

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

> So it seems to me that I should change the code back so that its
> behavior is the same as in v1, while trying to avoid what you called
> code ergonomics regression when you reviewed v1. Ok, let's see if I
> find a way to do that.

Sorry, the above reads like (but it is a bit fuzzy and oblique to me
to be certain) I suggested a wrong approach on v1 which we are
seeing in the version under review and I am suggesting to reverse
course?  It is a bit too far apart between versions so I am not sure
if that is what is happening, but if so, sorry for making you polish
itrepeatedly.

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

[Christian Couder]

On Mon, Sep 15, 2025 at 6:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > So it seems to me that I should change the code back so that its
> > behavior is the same as in v1, while trying to avoid what you called
> > code ergonomics regression when you reviewed v1. Ok, let's see if I
> > find a way to do that.
>
> Sorry, the above reads like (but it is a bit fuzzy and oblique to me
> to be certain) I suggested a wrong approach on v1 which we are
> seeing in the version under review and I am suggesting to reverse
> course?

I just hoped to get hints in case you had an idea I didn't understand
about how the code should look like.

> It is a bit too far apart between versions so I am not sure
> if that is what is happening, but if so, sorry for making you polish
> itrepeatedly.

No worries. I hope the V3 I just sent is better than both the v1 and
v2, or at least that it moves us closer to something better than both
of them.

Thanks.