On Wed, Sep 10, 2025 at 10:08:39AM +0200, Christian Couder wrote:

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.

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.

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.

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

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