On Thu, Sep 11, 2025 at 8:06 AM Patrick Steinhardt [off-list ref] wrote:

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

quoted

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.

quoted

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

quoted

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

quoted

      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.

quoted

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

quoted

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

quoted

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