{"thread":{"id":"64117","subject":"[PATCH 0/2] fast-import: start controlling how commit signatures are handled","startedAt":"2025-09-10T08:09:11Z","lastAt":"2025-09-17T18:28:09Z","messageCount":21,"participants":["Christian Couder","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"526010","messageId":"20250910080839.2142651-1-christian.couder@gmail.com","threadId":"64117","inReplyTo":null,"subject":"[PATCH 0/2] fast-import: start controlling how commit signatures are handled","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-10T08:08:37Z","receivedAt":"2025-09-10T08:09:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Tools like `git-filter-repo` should be able to control how commit\nsignatures are handled when regenerating repository content after it\nhas been filtered (see\nhttps://github.com/newren/git-filter-repo/issues/139). For this\npurpose, they need a way for `git fast-import` to control how commit\nsignatures are handled.\n\nThis small patch series starts to implement such a way by adding a new\n`--signed-commits=<mode>` option to `git fast-import`.\n\nFor now this new option behaves in a very similar way as the option\nwith the same name that already exists in `git fast-export`.\nEspecially it supports exactly the same <mode>s and the same aliases\nfor these modes. For example \"ignore\" is a synonym for \"verbatim\".\n\nIn the future I want to implement new <mode>s like \"strip-if-invalid\",\n\"re-sign\", \"re-sign-if-invalid\" that might be a bit more complex for\nthis option. But for now I prefer to start with the simple modes to\nvalidate the general design of the new option.\n\nIn particular, I am interested in feedback about the following:\n\n  - Should we keep \"ignore\" as a synonym for \"verbatim\" and \"warn\" as\n    a synonym for \"warn-verbatim\"? My opinion is that they might be\n    confusing, so we might want to remove them for `git fast-import`\n    even if we keep them for `git fast-export`. The parsing code might\n    be a bit more complex if we do that though, so for now I have kept\n    the synonyms.\n\n  - Are we still fine with most <mode>s having a \"warn-*\" variant\n    (like the \"warn-strip\" variant of \"strip\" for example)? Or should\n    we have a separate `--verbose` or maybe `--signed-commits-verbose`\n    option dedicated to switching warnings on/off? I think it's good\n    to decide about this before the number of <mode>s increases a lot\n    with new <mode>s like \"strip-if-invalid\", \"re-sign\",\n    \"re-sign-if-invalid\" and possibly others.\n\nCI tests:\n\nThey have all passed. See:\n\nhttps://github.com/chriscool/git/actions/runs/17606244845/job/50017575843\n\n\nChristian Couder (2):\n  gpg-interface: refactor 'enum sign_mode' parsing\n  fast-import: add '--signed-commits=<mode>' option\n\n Documentation/git-fast-import.adoc |   5 ++\n builtin/fast-export.c              |  23 ++----\n builtin/fast-import.c              |  55 +++++++++++----\n gpg-interface.c                    |  17 +++++\n gpg-interface.h                    |  15 ++++\n t/meson.build                      |   1 +\n t/t9305-fast-import-signatures.sh  | 108 +++++++++++++++++++++++++++++\n 7 files changed, 196 insertions(+), 28 deletions(-)\n create mode 100755 t/t9305-fast-import-signatures.sh\n\n-- \n2.51.0.195.g61112aeac3\n\n"},{"id":"526011","messageId":"20250910080839.2142651-2-christian.couder@gmail.com","threadId":"64117","inReplyTo":"20250910080839.2142651-1-christian.couder@gmail.com","subject":"[PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-10T08:08:38Z","receivedAt":"2025-09-10T08:09:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"The definition of 'enum sign_mode' as well as its parsing code are in\n\"builtin/fast-export.c\". This was fine because `git fast-export` was the\nonly command with '--signed-tags=<mode>' or '--signed-commits=<mode>'\noptions.\n\nIn a following commit, we are going to add such options to `git\nfast-import`, which will be simpler, easier and cleaner if we can reuse\nthe 'enum sign_mode' defintion and parsing code.\n\nSo let's move that definition and parsing code from\n\"builtin/fast-export.c\" to \"gpg-interface.{c,h}\".\n\nWhile at it, let's fix a small indentation issue with the arguments of\nparse_opt_sign_mode().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/fast-export.c | 23 +++++++----------------\n gpg-interface.c       | 17 +++++++++++++++++\n gpg-interface.h       | 15 +++++++++++++++\n 3 files changed, 39 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex c06ee0b213..3994a8f898 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -37,8 +37,6 @@ static const char *const fast_export_usage[] = {\n \tNULL\n };\n \n-enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_WARN_VERBATIM, SIGN_WARN_STRIP };\n-\n static int progress;\n static enum sign_mode signed_tag_mode = SIGN_ABORT;\n static enum sign_mode signed_commit_mode = SIGN_STRIP;\n@@ -59,24 +57,17 @@ static struct hashmap anonymized_seeds;\n static struct revision_sources revision_sources;\n \n static int parse_opt_sign_mode(const struct option *opt,\n-\t\t\t\t     const char *arg, int unset)\n+\t\t\t       const char *arg, int unset)\n {\n \tenum sign_mode *val = opt->value;\n+\n \tif (unset)\n \t\treturn 0;\n-\telse if (!strcmp(arg, \"abort\"))\n-\t\t*val = SIGN_ABORT;\n-\telse if (!strcmp(arg, \"verbatim\") || !strcmp(arg, \"ignore\"))\n-\t\t*val = SIGN_VERBATIM;\n-\telse if (!strcmp(arg, \"warn-verbatim\") || !strcmp(arg, \"warn\"))\n-\t\t*val = SIGN_WARN_VERBATIM;\n-\telse if (!strcmp(arg, \"warn-strip\"))\n-\t\t*val = SIGN_WARN_STRIP;\n-\telse if (!strcmp(arg, \"strip\"))\n-\t\t*val = SIGN_STRIP;\n-\telse\n-\t\treturn error(\"Unknown %s mode: %s\", opt->long_name, arg);\n-\treturn 0;\n+\n+\tif (!parse_sign_mode(arg, val))\n+\t\treturn 0;\n+\n+\treturn error(\"Unknown %s mode: %s\", opt->long_name, arg);\n }\n \n static int parse_opt_tag_of_filtered_mode(const struct option *opt,\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 06e7fb5060..2f4f0e32cb 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -1125,3 +1125,20 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \tFREE_AND_NULL(ssh_signing_key_file);\n \treturn ret;\n }\n+\n+int parse_sign_mode(const char *arg, enum sign_mode *mode)\n+{\n+\tif (!strcmp(arg, \"abort\"))\n+\t\t*mode = SIGN_ABORT;\n+\telse if (!strcmp(arg, \"verbatim\") || !strcmp(arg, \"ignore\"))\n+\t\t*mode = SIGN_VERBATIM;\n+\telse if (!strcmp(arg, \"warn-verbatim\") || !strcmp(arg, \"warn\"))\n+\t\t*mode = SIGN_WARN_VERBATIM;\n+\telse if (!strcmp(arg, \"warn-strip\"))\n+\t\t*mode = SIGN_WARN_STRIP;\n+\telse if (!strcmp(arg, \"strip\"))\n+\t\t*mode = SIGN_STRIP;\n+\telse\n+\t\treturn -1;\n+\treturn 0;\n+}\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex 60ddf8bbfa..44856cc55f 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -104,4 +104,19 @@ int check_signature(struct signature_check *sigc,\n void print_signature_buffer(const struct signature_check *sigc,\n \t\t\t    unsigned flags);\n \n+/* Modes for --signed-tags=<mode> and --signed-commits=<mode> options */\n+enum sign_mode {\n+\tSIGN_ABORT,\n+\tSIGN_WARN_VERBATIM,\n+\tSIGN_VERBATIM,\n+\tSIGN_WARN_STRIP,\n+\tSIGN_STRIP,\n+};\n+\n+/*\n+ * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1\n+ * otherwise.\n+ */\n+int parse_sign_mode(const char *arg, enum sign_mode *mode);\n+\n #endif\n-- \n2.51.0.195.g61112aeac3\n\n"},{"id":"526012","messageId":"20250910080839.2142651-3-christian.couder@gmail.com","threadId":"64117","inReplyTo":"20250910080839.2142651-1-christian.couder@gmail.com","subject":"[PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-10T08:08:39Z","receivedAt":"2025-09-10T08:09:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"A '--signed-commits=<mode>' option is already available when using\n`git fast-export` to decide what should be done at export time about\ncommit signatures. At import time though, there is no option, or\nother way, in `git fast-import` to decide about commit signatures.\n\nTo remediate that, let's add a '--signed-commits=<mode>' option to\n`git fast-import` too.\n\nFor now the supported <mode>s are the same as those supported by\n`git fast-export`.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Documentation/git-fast-import.adoc |   5 ++\n builtin/fast-import.c              |  55 +++++++++++----\n t/meson.build                      |   1 +\n t/t9305-fast-import-signatures.sh  | 108 +++++++++++++++++++++++++++++\n 4 files changed, 157 insertions(+), 12 deletions(-)\n create mode 100755 t/t9305-fast-import-signatures.sh\n\ndiff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc\nindex 3144ffcdb6..90f242d058 100644\n--- a/Documentation/git-fast-import.adoc\n+++ b/Documentation/git-fast-import.adoc\n@@ -66,6 +66,11 @@ OPTIONS\n \tremote-helpers that use the `import` capability, as they are\n \talready trusted to run their own code.\n \n+--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::\n+\tSpecify how to handle signed commits.  Behaves in the same way\n+\tas the same option in linkgit:git-fast-export[1], except that\n+\tdefault is 'verbatim' (instead of 'abort').\n+\n Options for Frontends\n ~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 2c35f9345d..f932dd2c65 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -188,6 +188,8 @@ static int global_argc;\n static const char **global_argv;\n static const char *global_prefix;\n \n+static enum sign_mode signed_commit_mode = SIGN_VERBATIM;\n+\n /* Memory pools */\n static struct mem_pool fi_mem_pool = {\n \t.block_alloc = 2*1024*1024 - sizeof(struct mp_block),\n@@ -2785,6 +2787,23 @@ static void store_signature(struct signature_data *stored_sig,\n \t}\n }\n \n+/* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n+static void import_signature(struct signature_data *sig_sha1,\n+\t\t\t     struct signature_data *sig_sha256,\n+\t\t\t     const char *v)\n+{\n+\tstruct signature_data sig = { NULL, NULL, STRBUF_INIT };\n+\n+\tparse_one_signature(&sig, v);\n+\n+\tif (!strcmp(sig.hash_algo, \"sha1\"))\n+\t\tstore_signature(sig_sha1, &sig, \"SHA-1\");\n+\telse if (!strcmp(sig.hash_algo, \"sha256\"))\n+\t\tstore_signature(sig_sha256, &sig, \"SHA-256\");\n+\telse\n+\t\tBUG(\"parse_one_signature() returned unknown hash algo\");\n+}\n+\n static void parse_new_commit(const char *arg)\n {\n \tstatic struct strbuf msg = STRBUF_INIT;\n@@ -2817,19 +2836,28 @@ static void parse_new_commit(const char *arg)\n \tif (!committer)\n \t\tdie(\"Expected committer but didn't get one\");\n \n-\t/* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n \twhile (skip_prefix(command_buf.buf, \"gpgsig \", &v)) {\n-\t\tstruct signature_data sig = { NULL, NULL, STRBUF_INIT };\n-\n-\t\tparse_one_signature(&sig, v);\n-\n-\t\tif (!strcmp(sig.hash_algo, \"sha1\"))\n-\t\t\tstore_signature(&sig_sha1, &sig, \"SHA-1\");\n-\t\telse if (!strcmp(sig.hash_algo, \"sha256\"))\n-\t\t\tstore_signature(&sig_sha256, &sig, \"SHA-256\");\n-\t\telse\n-\t\t\tBUG(\"parse_one_signature() returned unknown hash algo\");\n-\n+\t\tstruct strbuf data = STRBUF_INIT;\n+\t\tswitch (signed_commit_mode) {\n+\t\tcase SIGN_ABORT:\n+\t\t\tdie(\"encountered signed commit; use \"\n+\t\t\t    \"--signed-commits=<mode> to handle it\");\n+\t\tcase SIGN_WARN_VERBATIM:\n+\t\t\twarning(\"importing a commit signature\");\n+\t\t\t/* fallthru */\n+\t\tcase SIGN_VERBATIM:\n+\t\t\timport_signature(&sig_sha1, &sig_sha256, v);\n+\t\t\tbreak;\n+\t\tcase SIGN_WARN_STRIP:\n+\t\t\twarning(\"stripping a commit signature\");\n+\t\t\t/* fallthru */\n+\t\tcase SIGN_STRIP:\n+\t\t\t/* Read signature data and discard it */\n+\t\t\tread_next_command();\n+\t\t\tparse_data(&data, 0, NULL);\n+\t\t\tstrbuf_release(&data);\n+\t\t\tbreak;\n+\t\t}\n \t\tread_next_command();\n \t}\n \n@@ -3501,6 +3529,9 @@ static int parse_one_option(const char *option)\n \t\toption_active_branches(option);\n \t} else if (skip_prefix(option, \"export-pack-edges=\", &option)) {\n \t\toption_export_pack_edges(option);\n+\t} else if (skip_prefix(option, \"signed-commits=\", &option)) {\n+\t\tif (parse_sign_mode(option, &signed_commit_mode))\n+\t\t\tdie(\"unknown --signed-commits mode '%s'\", option);\n \t} else if (!strcmp(option, \"quiet\")) {\n \t\tshow_stats = 0;\n \t\tquiet = 1;\ndiff --git a/t/meson.build b/t/meson.build\nindex 82af229be3..08ad6938e2 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -1032,6 +1032,7 @@ integration_tests = [\n   't9302-fast-import-unpack-limit.sh',\n   't9303-fast-import-compression.sh',\n   't9304-fast-import-marks.sh',\n+  't9305-fast-import-signatures.sh',\n   't9350-fast-export.sh',\n   't9351-fast-export-anonymize.sh',\n   't9400-git-cvsserver-server.sh',\ndiff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh\nnew file mode 100755\nindex 0000000000..5a52691b29\n--- /dev/null\n+++ b/t/t9305-fast-import-signatures.sh\n@@ -0,0 +1,108 @@\n+#!/bin/sh\n+\n+test_description='git fast-import --signed-commits=<mode>'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-gpg.sh\"\n+\n+test_expect_success 'set up unsigned initial commit and import repo' '\n+\ttest_commit first &&\n+\tmkdir new &&\n+\tgit --git-dir=new/.git init\n+'\n+\n+test_expect_success GPG 'set up OpenPGP signed commit' '\n+\tgit checkout -b openpgp-signing main &&\n+\techo \"Content for OpenPGP signing.\" >file-sign &&\n+\tgit add file-sign &&\n+\tgit commit -S -m \"OpenPGP signed commit\" &&\n+\tOPENPGP_SIGNING=$(git rev-parse --verify openpgp-signing)\n+'\n+\n+test_expect_success GPG 'import OpenPGP signature with --signed-commits=verbatim' '\n+\tgit fast-export --signed-commits=verbatim openpgp-signing >output &&\n+\tgit -C new fast-import --quiet --signed-commits=verbatim <output >log 2>&1 &&\n+\tIMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) &&\n+\ttest $OPENPGP_SIGNING = $IMPORTED &&\n+\ttest_must_be_empty log\n+'\n+\n+test_expect_success GPGSM 'set up X.509 signed commit' '\n+\tgit checkout -b x509-signing main &&\n+\ttest_config gpg.format x509 &&\n+\ttest_config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\techo \"Content for X.509 signing.\" >file-sign &&\n+\tgit add file-sign &&\n+\tgit commit -S -m \"X.509 signed commit\" &&\n+\tX509_SIGNING=$(git rev-parse HEAD)\n+'\n+\n+test_expect_success GPGSM 'import X.509 signature fails with --signed-commits=abort' '\n+\tgit fast-export --signed-commits=verbatim x509-signing >output &&\n+\ttest_must_fail git -C new fast-import --quiet --signed-commits=abort <output\n+'\n+\n+test_expect_success GPGSM 'import X.509 signature with --signed-commits=warn-verbatim' '\n+\tgit -C new fast-import --quiet --signed-commits=warn-verbatim <output >log 2>&1 &&\n+\tIMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) &&\n+\ttest $X509_SIGNING = $IMPORTED &&\n+\ttest_grep \"importing a commit signature\" log\n+'\n+\n+test_expect_success GPGSSH 'set up SSH signed commit' '\n+\tgit checkout -b ssh-signing main &&\n+\ttest_config gpg.format ssh &&\n+\ttest_config user.signingkey \"${GPGSSH_KEY_PRIMARY}\" &&\n+\techo \"Content for SSH signing.\" >file-sign &&\n+\tgit add file-sign &&\n+\tgit commit -S -m \"SSH signed commit\" &&\n+\tSSH_SIGNING=$(git rev-parse HEAD)\n+'\n+\n+test_expect_success GPGSSH 'strip SSH signature with --signed-commits=strip' '\n+\tgit fast-export --signed-commits=verbatim ssh-signing >output &&\n+\tgit -C new fast-import --quiet --signed-commits=strip <output >log 2>&1 &&\n+\tIMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) &&\n+\ttest $SSH_SIGNING != $IMPORTED &&\n+\tgit -C new cat-file commit \"$IMPORTED\" >actual &&\n+\ttest_grep ! -E \"^gpgsig\" actual &&\n+\ttest_must_be_empty log\n+'\n+\n+test_expect_success GPG 'setup a commit with dual OpenPGP signatures on its SHA-1 and SHA-256 formats' '\n+\t# Create a signed SHA-256 commit\n+\tgit init --object-format=sha256 explicit-sha256 &&\n+\tgit -C explicit-sha256 config extensions.compatObjectFormat sha1 &&\n+\tgit -C explicit-sha256 checkout -b dual-signed &&\n+\ttest_commit -C explicit-sha256 A &&\n+\techo B >explicit-sha256/B &&\n+\tgit -C explicit-sha256 add B &&\n+\ttest_tick &&\n+\tgit -C explicit-sha256 commit -S -m \"signed\" B &&\n+\tSHA256_B=$(git -C explicit-sha256 rev-parse dual-signed) &&\n+\n+\t# Create the corresponding SHA-1 commit\n+\tSHA1_B=$(git -C explicit-sha256 rev-parse --output-object-format=sha1 dual-signed) &&\n+\n+\t# Check that the resulting SHA-1 commit has both signatures\n+\techo $SHA1_B | git -C explicit-sha256 cat-file --batch >out &&\n+\ttest_grep -E \"^gpgsig \" out &&\n+\ttest_grep -E \"^gpgsig-sha256 \" out\n+'\n+\n+test_expect_success GPG 'strip both OpenPGP signatures with --signed-commits=warn-strip' '\n+\tgit -C explicit-sha256 fast-export --signed-commits=verbatim dual-signed >output &&\n+\ttest_grep -E \"^gpgsig sha1 openpgp\" output &&\n+\ttest_grep -E \"^gpgsig sha256 openpgp\" output &&\n+\tgit -C new fast-import --quiet --signed-commits=warn-strip <output >log 2>&1 &&\n+\tgit -C new cat-file commit refs/heads/dual-signed >actual &&\n+\ttest_grep ! -E \"^gpgsig \" actual &&\n+\ttest_grep ! -E \"^gpgsig-sha256 \" actual &&\n+\ttest_grep \"stripping a commit signature\" log >out &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_done\n-- \n2.51.0.195.g61112aeac3\n\n"},{"id":"526058","messageId":"xmqq348utfox.fsf@gitster.g","threadId":"64117","inReplyTo":"20250910080839.2142651-2-christian.couder@gmail.com","subject":"Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-10T16:50:22Z","receivedAt":"2025-09-10T16:50:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index c06ee0b213..3994a8f898 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -37,8 +37,6 @@ static const char *const fast_export_usage[] = {\n>  \tNULL\n>  };\n>  \n> -enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_WARN_VERBATIM, SIGN_WARN_STRIP };\n> -\n> ...\n>  static int parse_opt_sign_mode(const struct option *opt,\n> -\t\t\t\t     const char *arg, int unset)\n> +\t\t\t       const char *arg, int unset)\n>  {\n>  \tenum sign_mode *val = opt->value;\n> +\n>  \tif (unset)\n>  \t\treturn 0;\n> -\telse if (!strcmp(arg, \"abort\"))\n> -\t\t*val = SIGN_ABORT;\n> -\telse if (!strcmp(arg, \"verbatim\") || !strcmp(arg, \"ignore\"))\n> -\t\t*val = SIGN_VERBATIM;\n> -\telse if (!strcmp(arg, \"warn-verbatim\") || !strcmp(arg, \"warn\"))\n> -\t\t*val = SIGN_WARN_VERBATIM;\n> -\telse if (!strcmp(arg, \"warn-strip\"))\n> -\t\t*val = SIGN_WARN_STRIP;\n> -\telse if (!strcmp(arg, \"strip\"))\n> -\t\t*val = SIGN_STRIP;\n> -\telse\n> -\t\treturn error(\"Unknown %s mode: %s\", opt->long_name, arg);\n> -\treturn 0;\n> +\n> +\tif (!parse_sign_mode(arg, val))\n> +\t\treturn 0;\n> +\n> +\treturn error(\"Unknown %s mode: %s\", opt->long_name, arg);\n>  }\n\nI am not sure if it is correct for the \"unset\" codepath doing\nnothing on *val before returning, but changing that is of course not\nin scope of this patch at all.  And the result of the change in this\nfunction is very pleasing to read.  And ...\n\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 06e7fb5060..2f4f0e32cb 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -1125,3 +1125,20 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n> ...\n> +int parse_sign_mode(const char *arg, enum sign_mode *mode)\n> +{\n> +\tif (!strcmp(arg, \"abort\"))\n> +\t\t*mode = SIGN_ABORT;\n> +\telse if (!strcmp(arg, \"verbatim\") || !strcmp(arg, \"ignore\"))\n> +\t\t*mode = SIGN_VERBATIM;\n> +\telse if (!strcmp(arg, \"warn-verbatim\") || !strcmp(arg, \"warn\"))\n> +\t\t*mode = SIGN_WARN_VERBATIM;\n> +\telse if (!strcmp(arg, \"warn-strip\"))\n> +\t\t*mode = SIGN_WARN_STRIP;\n> +\telse if (!strcmp(arg, \"strip\"))\n> +\t\t*mode = SIGN_STRIP;\n> +\telse\n> +\t\treturn -1;\n> +\treturn 0;\n> +}\n\n... this is of course very much expected.\n\n> diff --git a/gpg-interface.h b/gpg-interface.h\n> index 60ddf8bbfa..44856cc55f 100644\n> --- a/gpg-interface.h\n> +++ b/gpg-interface.h\n> @@ -104,4 +104,19 @@ int check_signature(struct signature_check *sigc,\n>  void print_signature_buffer(const struct signature_check *sigc,\n>  \t\t\t    unsigned flags);\n>  \n> +/* Modes for --signed-tags=<mode> and --signed-commits=<mode> options */\n> +enum sign_mode {\n> +\tSIGN_ABORT,\n> +\tSIGN_WARN_VERBATIM,\n> +\tSIGN_VERBATIM,\n> +\tSIGN_WARN_STRIP,\n> +\tSIGN_STRIP,\n> +};\n\nAnd this is much easier to read in the header file for all to see.\n\n> +/*\n> + * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1\n> + * otherwise.\n> + */\n> +int parse_sign_mode(const char *arg, enum sign_mode *mode);\n> +\n\nNice.\n\n\n"},{"id":"526059","messageId":"xmqqtt1as08f.fsf@gitster.g","threadId":"64117","inReplyTo":"20250910080839.2142651-3-christian.couder@gmail.com","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-10T17:09:36Z","receivedAt":"2025-09-10T17:09:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> @@ -2785,6 +2787,23 @@ static void store_signature(struct signature_data *stored_sig,\n>  \t}\n>  }\n>  \n> +/* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n> +static void import_signature(struct signature_data *sig_sha1,\n> +\t\t\t     struct signature_data *sig_sha256,\n> +\t\t\t     const char *v)\n> +{\n> +\tstruct signature_data sig = { NULL, NULL, STRBUF_INIT };\n> +\n> +\tparse_one_signature(&sig, v);\n> +\n> +\tif (!strcmp(sig.hash_algo, \"sha1\"))\n> +\t\tstore_signature(sig_sha1, &sig, \"SHA-1\");\n> +\telse if (!strcmp(sig.hash_algo, \"sha256\"))\n> +\t\tstore_signature(sig_sha256, &sig, \"SHA-256\");\n> +\telse\n> +\t\tBUG(\"parse_one_signature() returned unknown hash algo\");\n> +}\n> +\n\nTHis is a new function; I am not sure if the division of labor\nbetween this one and its caller is done right.  See below.\n\n> @@ -2817,19 +2836,28 @@ static void parse_new_commit(const char *arg)\n>  \tif (!committer)\n>  \t\tdie(\"Expected committer but didn't get one\");\n>  \n> -\t/* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n>  \twhile (skip_prefix(command_buf.buf, \"gpgsig \", &v)) {\n> -\t\tstruct signature_data sig = { NULL, NULL, STRBUF_INIT };\n> -\n> -\t\tparse_one_signature(&sig, v);\n> -\n> -\t\tif (!strcmp(sig.hash_algo, \"sha1\"))\n> -\t\t\tstore_signature(&sig_sha1, &sig, \"SHA-1\");\n> -\t\telse if (!strcmp(sig.hash_algo, \"sha256\"))\n> -\t\t\tstore_signature(&sig_sha256, &sig, \"SHA-256\");\n> -\t\telse\n> -\t\t\tBUG(\"parse_one_signature() returned unknown hash algo\");\n> -\n> +\t\tstruct strbuf data = STRBUF_INIT;\n> +\t\tswitch (signed_commit_mode) {\n> +\t\tcase SIGN_ABORT:\n> +\t\t\tdie(\"encountered signed commit; use \"\n> +\t\t\t    \"--signed-commits=<mode> to handle it\");\n> +\t\tcase SIGN_WARN_VERBATIM:\n> +\t\t\twarning(\"importing a commit signature\");\n> +\t\t\t/* fallthru */\n> +\t\tcase SIGN_VERBATIM:\n> +\t\t\timport_signature(&sig_sha1, &sig_sha256, v);\n> +\t\t\tbreak;\n> +\t\tcase SIGN_WARN_STRIP:\n> +\t\t\twarning(\"stripping a commit signature\");\n> +\t\t\t/* fallthru */\n> +\t\tcase SIGN_STRIP:\n> +\t\t\t/* Read signature data and discard it */\n> +\t\t\tread_next_command();\n> +\t\t\tparse_data(&data, 0, NULL);\n> +\t\t\tstrbuf_release(&data);\n> +\t\t\tbreak;\n> +\t\t}\n>  \t\tread_next_command();\n>  \t}\n\nI am not sure if this change had to be this way.  The old code\nalways called parse_one_signature(), which was responsible for\nchecking the signature format and then calling read_next_command()\nand parse_data(), so no matter what happened afterwards, we know we\nare consuming the data stream regardless of the conditional execution\nthat happens here.\n\nThe new code calls import_signature() or an inlined sequence of\nread_next_command() plus parse_data(), essentially making each case\narm in the switch() statement responsible individually for consuming\nthe incoming data.  When somebody adds a new case there to specify a\ndifferent way to handle signatures, they have to make sure that they\ndo not forget calling read_next_command() and parse_data() themselves.\n\nEven though I can see, after some code inspection, that no branches\nin the current code forgets to advance the incoming data stream to\nleave us out of sync right now, this change feels like a bit of code\nergonomics regression to me.  Was it so important that we pass a\nbroken signature without inspecting in STRIP mode?  I am guessing\nthat is the reason why the new code tries hard to avoid calling the\nparse_one_signature() function in these case arms.\n\nAn aside.  I think the warning message about importing should have a\nword \"verbatim\" in it, e.g.\n\n\twarning(\"importing a commit signature verbatim\");\n\nThanks.\n"},{"id":"526066","messageId":"xmqqms72rwx4.fsf@gitster.g","threadId":"64117","inReplyTo":"20250910080839.2142651-3-christian.couder@gmail.com","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-10T18:21:11Z","receivedAt":"2025-09-10T18:21:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> A '--signed-commits=<mode>' option is already available when using\n> `git fast-export` to decide what should be done at export time about\n> commit signatures. At import time though, there is no option, or\n> other way, in `git fast-import` to decide about commit signatures.\n>\n> To remediate that, let's add a '--signed-commits=<mode>' option to\n> `git fast-import` too.\n>\n> For now the supported <mode>s are the same as those supported by\n> `git fast-export`.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  Documentation/git-fast-import.adoc |   5 ++\n>  builtin/fast-import.c              |  55 +++++++++++----\n>  t/meson.build                      |   1 +\n>  t/t9305-fast-import-signatures.sh  | 108 +++++++++++++++++++++++++++++\n>  4 files changed, 157 insertions(+), 12 deletions(-)\n>  create mode 100755 t/t9305-fast-import-signatures.sh\n>\n> diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc\n> index 3144ffcdb6..90f242d058 100644\n> --- a/Documentation/git-fast-import.adoc\n> +++ b/Documentation/git-fast-import.adoc\n> @@ -66,6 +66,11 @@ OPTIONS\n>  \tremote-helpers that use the `import` capability, as they are\n>  \talready trusted to run their own code.\n>  \n> +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::\n> +\tSpecify how to handle signed commits.  Behaves in the same way\n> +\tas the same option in linkgit:git-fast-export[1], except that\n> +\tdefault is 'verbatim' (instead of 'abort').\n\nI forgot one thing.  Earlier in 1/2 we saw\n\n    The definition of 'enum sign_mode' as well as its parsing code are in\n    \"builtin/fast-export.c\". This was fine because `git fast-export` was the\n    only command with '--signed-tags=<mode>' or '--signed-commits=<mode>'\n    options.\n\n    In a following commit, we are going to add such options to `git\n    fast-import`, which will be simpler, easier and cleaner if we can reuse\n    the 'enum sign_mode' defintion and parsing code.\n\nand I was implicitly expecting that both commits and tags would be\nsupported on the import side, but this only deals with the commits?\n\nIs patch [3/2] missing from the archive?\n\nThanks.\n"},{"id":"526106","messageId":"aMJm57VmWb3h167z@pks.im","threadId":"64117","inReplyTo":"20250910080839.2142651-2-christian.couder@gmail.com","subject":"Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-11T06:06:31Z","receivedAt":"2025-09-11T06:06:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 10, 2025 at 10:08:38AM +0200, Christian Couder wrote:\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index c06ee0b213..3994a8f898 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -59,24 +57,17 @@ static struct hashmap anonymized_seeds;\n>  static struct revision_sources revision_sources;\n>  \n>  static int parse_opt_sign_mode(const struct option *opt,\n> -\t\t\t\t     const char *arg, int unset)\n> +\t\t\t       const char *arg, int unset)\n>  {\n>  \tenum sign_mode *val = opt->value;\n> +\n>  \tif (unset)\n>  \t\treturn 0;\n> -\telse if (!strcmp(arg, \"abort\"))\n> -\t\t*val = SIGN_ABORT;\n> -\telse if (!strcmp(arg, \"verbatim\") || !strcmp(arg, \"ignore\"))\n> -\t\t*val = SIGN_VERBATIM;\n> -\telse if (!strcmp(arg, \"warn-verbatim\") || !strcmp(arg, \"warn\"))\n> -\t\t*val = SIGN_WARN_VERBATIM;\n> -\telse if (!strcmp(arg, \"warn-strip\"))\n> -\t\t*val = SIGN_WARN_STRIP;\n> -\telse if (!strcmp(arg, \"strip\"))\n> -\t\t*val = SIGN_STRIP;\n> -\telse\n> -\t\treturn error(\"Unknown %s mode: %s\", opt->long_name, arg);\n> -\treturn 0;\n> +\n> +\tif (!parse_sign_mode(arg, val))\n> +\t\treturn 0;\n> +\n> +\treturn error(\"Unknown %s mode: %s\", opt->long_name, arg);\n\nWould it make sense to maybe reverse the error handling and say\nsomething like:\n\n    if (parse_sign_mode(arg, val) < 0)\n        return error(\"Unknown %s mode: %s\", opt->long_name, arg);\n\n    return 0;\n\nThat reads a bit more natural to me at least.\n\n> diff --git a/gpg-interface.h b/gpg-interface.h\n> index 60ddf8bbfa..44856cc55f 100644\n> --- a/gpg-interface.h\n> +++ b/gpg-interface.h\n> @@ -104,4 +104,19 @@ int check_signature(struct signature_check *sigc,\n>  void print_signature_buffer(const struct signature_check *sigc,\n>  \t\t\t    unsigned flags);\n>  \n> +/* Modes for --signed-tags=<mode> and --signed-commits=<mode> options */\n\nNit: let's finish this sentence with a dot.\n\n> +enum sign_mode {\n> +\tSIGN_ABORT,\n> +\tSIGN_WARN_VERBATIM,\n> +\tSIGN_VERBATIM,\n> +\tSIGN_WARN_STRIP,\n> +\tSIGN_STRIP,\n> +};\n> +\n> +/*\n> + * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1\n> + * otherwise.\n> + */\n> +int parse_sign_mode(const char *arg, enum sign_mode *mode);\n\nOkay, makes sense.\n\nPatrick\n"},{"id":"526107","messageId":"aMJm8rSOeQsO_qTG@pks.im","threadId":"64117","inReplyTo":"20250910080839.2142651-3-christian.couder@gmail.com","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-11T06:06:42Z","receivedAt":"2025-09-11T06:06:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 10, 2025 at 10:08:39AM +0200, Christian Couder wrote:\n> diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc\n> index 3144ffcdb6..90f242d058 100644\n> --- a/Documentation/git-fast-import.adoc\n> +++ b/Documentation/git-fast-import.adoc\n> @@ -66,6 +66,11 @@ OPTIONS\n>  \tremote-helpers that use the `import` capability, as they are\n>  \talready trusted to run their own code.\n>  \n> +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::\n> +\tSpecify how to handle signed commits.  Behaves in the same way\n> +\tas the same option in linkgit:git-fast-export[1], except that\n> +\tdefault is 'verbatim' (instead of 'abort').\n\nWe could of course extract the description from git-fast-export(1) and\nmove it into a shared file so that we can include it from both commands.\nNot sure whether that's worth it though.\n\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> index 2c35f9345d..f932dd2c65 100644\n> --- a/builtin/fast-import.c\n> +++ b/builtin/fast-import.c\n> @@ -2785,6 +2787,23 @@ static void store_signature(struct signature_data *stored_sig,\n>  \t}\n>  }\n>  \n> +/* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n\nHm. Does \"up to 2\" indicate that the commit may have two signatures at\nonce? If so...\n\n> +static void import_signature(struct signature_data *sig_sha1,\n> +\t\t\t     struct signature_data *sig_sha256,\n> +\t\t\t     const char *v)\n> +{\n> +\tstruct signature_data sig = { NULL, NULL, STRBUF_INIT };\n> +\n> +\tparse_one_signature(&sig, v);\n> +\n> +\tif (!strcmp(sig.hash_algo, \"sha1\"))\n> +\t\tstore_signature(sig_sha1, &sig, \"SHA-1\");\n> +\telse if (!strcmp(sig.hash_algo, \"sha256\"))\n> +\t\tstore_signature(sig_sha256, &sig, \"SHA-256\");\n\n... then the code here seems to indicate otherwise as you only parse\neither the \"sha1\" signature or the \"sha256\" signature, but never both.\n\n> +\telse\n> +\t\tBUG(\"parse_one_signature() returned unknown hash algo\");\n\nI think we should not label this a bug. It is feasible that we introduce\na third hash algorithm in the future that we don't know to handle yet,\nbut that would not be a programming bug but a normal error. So we should\nprobably `die()` instead.\n\n> @@ -2817,19 +2836,28 @@ static void parse_new_commit(const char *arg)\n>  \tif (!committer)\n>  \t\tdie(\"Expected committer but didn't get one\");\n>  \n> -\t/* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n\nAha, this is where the comment comes from! Here it makes sense as we\nhave a loop, but it doesn't really feel sensible for the extracted\nfunction.\n\n>  \twhile (skip_prefix(command_buf.buf, \"gpgsig \", &v)) {\n> -\t\tstruct signature_data sig = { NULL, NULL, STRBUF_INIT };\n> -\n> -\t\tparse_one_signature(&sig, v);\n> -\n> -\t\tif (!strcmp(sig.hash_algo, \"sha1\"))\n> -\t\t\tstore_signature(&sig_sha1, &sig, \"SHA-1\");\n> -\t\telse if (!strcmp(sig.hash_algo, \"sha256\"))\n> -\t\t\tstore_signature(&sig_sha256, &sig, \"SHA-256\");\n> -\t\telse\n> -\t\t\tBUG(\"parse_one_signature() returned unknown hash algo\");\n\nAnd the call to `BUG()` is preexisting, as well. How about we move the\nextraction of this loop into a separate commit?\n\n> +\t\tstruct strbuf data = STRBUF_INIT;\n> +\t\tswitch (signed_commit_mode) {\n> +\t\tcase SIGN_ABORT:\n> +\t\t\tdie(\"encountered signed commit; use \"\n> +\t\t\t    \"--signed-commits=<mode> to handle it\");\n\nThis message should be marked for translation.\n\n> +\t\tcase SIGN_WARN_VERBATIM:\n> +\t\t\twarning(\"importing a commit signature\");\n> +\t\t\t/* fallthru */\n\nThis and the other warning should be marked for translation.\n\n> +\t\tcase SIGN_VERBATIM:\n> +\t\t\timport_signature(&sig_sha1, &sig_sha256, v);\n> +\t\t\tbreak;\n> +\t\tcase SIGN_WARN_STRIP:\n> +\t\t\twarning(\"stripping a commit signature\");\n> +\t\t\t/* fallthru */\n> +\t\tcase SIGN_STRIP:\n> +\t\t\t/* Read signature data and discard it */\n> +\t\t\tread_next_command();\n> +\t\t\tparse_data(&data, 0, NULL);\n> +\t\t\tstrbuf_release(&data);\n> +\t\t\tbreak;\n> +\t\t}\n>  \t\tread_next_command();\n>  \t}\n>  \n> @@ -3501,6 +3529,9 @@ static int parse_one_option(const char *option)\n>  \t\toption_active_branches(option);\n>  \t} else if (skip_prefix(option, \"export-pack-edges=\", &option)) {\n>  \t\toption_export_pack_edges(option);\n> +\t} else if (skip_prefix(option, \"signed-commits=\", &option)) {\n> +\t\tif (parse_sign_mode(option, &signed_commit_mode))\n> +\t\t\tdie(\"unknown --signed-commits mode '%s'\", option);\n\nDo we want to use `usagef()` instead?\n\n> diff --git a/t/meson.build b/t/meson.build\n> index 82af229be3..08ad6938e2 100644\n> --- a/t/meson.build\n> +++ b/t/meson.build\n> @@ -1032,6 +1032,7 @@ integration_tests = [\n>    't9302-fast-import-unpack-limit.sh',\n>    't9303-fast-import-compression.sh',\n>    't9304-fast-import-marks.sh',\n> +  't9305-fast-import-signatures.sh',\n>    't9350-fast-export.sh',\n>    't9351-fast-export-anonymize.sh',\n>    't9400-git-cvsserver-server.sh',\n> diff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh\n> new file mode 100755\n> index 0000000000..5a52691b29\n> --- /dev/null\n> +++ b/t/t9305-fast-import-signatures.sh\n> @@ -0,0 +1,108 @@\n> +#!/bin/sh\n> +\n> +test_description='git fast-import --signed-commits=<mode>'\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n\nThere shouldn't be a need to specify the initial branch name. You\nalready create the initial commit with `test_commit()`, so the calls to\ngit-checkout(1) can instead say `git checkout -b openpgp-signign first`\nbecause `test_commit()` creates that tag for us.\n\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY/lib-gpg.sh\"\n> +\n> +test_expect_success 'set up unsigned initial commit and import repo' '\n> +\ttest_commit first &&\n> +\tmkdir new &&\n> +\tgit --git-dir=new/.git init\n\nHm. Why don't we just say `git init new` or `git init --bare new`? I\nmight be missing something here.\n\n[snip]\n> +test_expect_success GPG 'setup a commit with dual OpenPGP signatures on its SHA-1 and SHA-256 formats' '\n> +\t# Create a signed SHA-256 commit\n> +\tgit init --object-format=sha256 explicit-sha256 &&\n> +\tgit -C explicit-sha256 config extensions.compatObjectFormat sha1 &&\n> +\tgit -C explicit-sha256 checkout -b dual-signed &&\n> +\ttest_commit -C explicit-sha256 A &&\n> +\techo B >explicit-sha256/B &&\n> +\tgit -C explicit-sha256 add B &&\n> +\ttest_tick &&\n> +\tgit -C explicit-sha256 commit -S -m \"signed\" B &&\n> +\tSHA256_B=$(git -C explicit-sha256 rev-parse dual-signed) &&\n> +\n> +\t# Create the corresponding SHA-1 commit\n> +\tSHA1_B=$(git -C explicit-sha256 rev-parse --output-object-format=sha1 dual-signed) &&\n> +\n> +\t# Check that the resulting SHA-1 commit has both signatures\n> +\techo $SHA1_B | git -C explicit-sha256 cat-file --batch >out &&\n\nYou can instead `git -c explicit-sha256 cat-file -p $SHA1_B >out`.\n\nPatrick\n"},{"id":"526136","messageId":"xmqqcy7xosu8.fsf@gitster.g","threadId":"64117","inReplyTo":"aMJm57VmWb3h167z@pks.im","subject":"Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-11T16:29:51Z","receivedAt":"2025-09-11T16:29:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> +\tif (!parse_sign_mode(arg, val))\n>> +\t\treturn 0;\n>> +\n>> +\treturn error(\"Unknown %s mode: %s\", opt->long_name, arg);\n>\n> Would it make sense to maybe reverse the error handling and say\n> something like:\n>\n>     if (parse_sign_mode(arg, val) < 0)\n>         return error(\"Unknown %s mode: %s\", opt->long_name, arg);\n>\n>     return 0;\n>\n> That reads a bit more natural to me at least.\n\nI also thought that the original had a \"Huh?\" factor, but my\nreaction was more like \"wouldn't it be easier to see if these are\nboth sides of if/else?\", i.e.\n\n\tif (parse_sign_mode(...))\n\t\treturn error(...);\n\telse\n\t\treturn 0;\n\nwhich made me think that the differences were mostly subjective and\nrefrained from commenting.\n\nBut after I see your version, I tend to agree that it is easier to\nsee the flow if the most-straightforward success case ran through to\nthe end, while exceptional cases (i.e. an error return, or the\ninitial \"unset\" case) did early returns.  I have to add, however,\nthat I feel that this is mostly subjective and falls into \"once it\nis in the tree, it's not really worth the patch noise to go and fix\nit up\" category.  If we will have v2 of this series, I do prefer to\nsee it written more in your way, though.\n\nThanks.\n\n"},{"id":"526141","messageId":"xmqq4it9ornz.fsf@gitster.g","threadId":"64117","inReplyTo":"aMJm8rSOeQsO_qTG@pks.im","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-11T16:55:12Z","receivedAt":"2025-09-11T16:55:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> +/* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n>\n> Hm. Does \"up to 2\" indicate that the commit may have two signatures at\n> once? If so...\n>\n>> +static void import_signature(struct signature_data *sig_sha1,\n>> +\t\t\t     struct signature_data *sig_sha256,\n>> +\t\t\t     const char *v)\n>> +{\n>> +\tstruct signature_data sig = { NULL, NULL, STRBUF_INIT };\n>> +\n>> +\tparse_one_signature(&sig, v);\n>> +\n>> +\tif (!strcmp(sig.hash_algo, \"sha1\"))\n>> +\t\tstore_signature(sig_sha1, &sig, \"SHA-1\");\n>> +\telse if (!strcmp(sig.hash_algo, \"sha256\"))\n>> +\t\tstore_signature(sig_sha256, &sig, \"SHA-256\");\n>\n> ... then the code here seems to indicate otherwise as you only parse\n> either the \"sha1\" signature or the \"sha256\" signature, but never both.\n\nCorrect and not quite.  The caller can call you twice in its loop.\nBut if the input was malformed and had two \"sha1\" (and no \"sha256\"),\nthis will not barf (as the original, so it is not a new bug).\n\nIn any case, I also found that \"up to 2\" comment somewhat strange.\nIt was more understandable back when it was near the loop, but not\nhere.\n\n>> +\telse\n>> +\t\tBUG(\"parse_one_signature() returned unknown hash algo\");\n>\n> I think we should not label this a bug. It is feasible that we introduce\n> a third hash algorithm in the future that we don't know to handle yet,\n> but that would not be a programming bug but a normal error. So we should\n> probably `die()` instead.\n\nGood thinking.\n"},{"id":"526177","messageId":"CAP8UFD13RvAD1d588onpv88LW73M_YPJ2aaAHeoveFtcCOAhTA@mail.gmail.com","threadId":"64117","inReplyTo":"xmqqcy7xosu8.fsf@gitster.g","subject":"Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-12T12:40:08Z","receivedAt":"2025-09-12T12:40:22Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 11, 2025 at 6:29 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n> >> +    if (!parse_sign_mode(arg, val))\n> >> +            return 0;\n> >> +\n> >> +    return error(\"Unknown %s mode: %s\", opt->long_name, arg);\n> >\n> > Would it make sense to maybe reverse the error handling and say\n> > something like:\n> >\n> >     if (parse_sign_mode(arg, val) < 0)\n> >         return error(\"Unknown %s mode: %s\", opt->long_name, arg);\n> >\n> >     return 0;\n> >\n> > That reads a bit more natural to me at least.\n\nBefore the above part of the code, there is:\n\n    if (unset)\n        return 0;\n\nSo it felt natural to me to have a flow where we continue to return 0\n(success) if we can and go on otherwise.\n\n> I also thought that the original had a \"Huh?\" factor, but my\n> reaction was more like \"wouldn't it be easier to see if these are\n> both sides of if/else?\", i.e.\n>\n>         if (parse_sign_mode(...))\n>                 return error(...);\n>         else\n>                 return 0;\n>\n> which made me think that the differences were mostly subjective and\n> refrained from commenting.\n\nI agree that the difference is very subjective. If there was something\nlike for example:\n\n```\nif (A)\n       return 0;\n\nif (B)\n       return 0;\n\n...\n\nif (F)\n       return 0;\n\nreturn -1\n```\n\nIt would feel strange if the function ended instead with:\n\n```\nif (!F)\n       return -1;\n\nreturn 0;\n```\n\n> But after I see your version, I tend to agree that it is easier to\n> see the flow if the most-straightforward success case ran through to\n> the end, while exceptional cases (i.e. an error return, or the\n> initial \"unset\" case) did early returns.  I have to add, however,\n> that I feel that this is mostly subjective and falls into \"once it\n> is in the tree, it's not really worth the patch noise to go and fix\n> it up\" category.  If we will have v2 of this series, I do prefer to\n> see it written more in your way, though.\n\nFine, I have written it in the way Patrick suggested in V2.\n\nThanks.\n"},{"id":"526178","messageId":"CAP8UFD33cdNg6qrsmOCbJbC1ZS_VY17UkdYfN_8YChAqrFpXcQ@mail.gmail.com","threadId":"64117","inReplyTo":"aMJm57VmWb3h167z@pks.im","subject":"Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-12T12:40:10Z","receivedAt":"2025-09-12T12:40:25Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 11, 2025 at 8:06 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Wed, Sep 10, 2025 at 10:08:38AM +0200, Christian Couder wrote:\n\n> > diff --git a/gpg-interface.h b/gpg-interface.h\n> > index 60ddf8bbfa..44856cc55f 100644\n> > --- a/gpg-interface.h\n> > +++ b/gpg-interface.h\n> > @@ -104,4 +104,19 @@ int check_signature(struct signature_check *sigc,\n> >  void print_signature_buffer(const struct signature_check *sigc,\n> >                           unsigned flags);\n> >\n> > +/* Modes for --signed-tags=<mode> and --signed-commits=<mode> options */\n>\n> Nit: let's finish this sentence with a dot.\n\nI have added a dot in the V2.\n\nThanks.\n"},{"id":"526184","messageId":"CAP8UFD1GHfVvT4c5cFocwP0KJFXOAybfFWXwK1WABy+igE+xOQ@mail.gmail.com","threadId":"64117","inReplyTo":"aMJm8rSOeQsO_qTG@pks.im","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-12T13:25:41Z","receivedAt":"2025-09-12T13:25:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 11, 2025 at 8:06 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Wed, Sep 10, 2025 at 10:08:39AM +0200, Christian Couder wrote:\n> > diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc\n> > index 3144ffcdb6..90f242d058 100644\n> > --- a/Documentation/git-fast-import.adoc\n> > +++ b/Documentation/git-fast-import.adoc\n> > @@ -66,6 +66,11 @@ OPTIONS\n> >       remote-helpers that use the `import` capability, as they are\n> >       already trusted to run their own code.\n> >\n> > +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::\n> > +     Specify how to handle signed commits.  Behaves in the same way\n> > +     as the same option in linkgit:git-fast-export[1], except that\n> > +     default is 'verbatim' (instead of 'abort').\n>\n> We could of course extract the description from git-fast-export(1) and\n> move it into a shared file so that we can include it from both commands.\n> Not sure whether that's worth it though.\n\nWhen I add more options, I plan to improve on that doc, but for now I\nthink it's Ok.\n\n> > +     else\n> > +             BUG(\"parse_one_signature() returned unknown hash algo\");\n>\n> I think we should not label this a bug. It is feasible that we introduce\n> a third hash algorithm in the future that we don't know to handle yet,\n> but that would not be a programming bug but a normal error. So we should\n> probably `die()` instead.\n\nI changed it to die() in V2.\n\n> > @@ -2817,19 +2836,28 @@ static void parse_new_commit(const char *arg)\n> >       if (!committer)\n> >               die(\"Expected committer but didn't get one\");\n> >\n> > -     /* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n>\n> Aha, this is where the comment comes from! Here it makes sense as we\n> have a loop, but it doesn't really feel sensible for the extracted\n> function.\n\nRight, I have removed the comment altogether in V2.\n\n> >       while (skip_prefix(command_buf.buf, \"gpgsig \", &v)) {\n> > -             struct signature_data sig = { NULL, NULL, STRBUF_INIT };\n> > -\n> > -             parse_one_signature(&sig, v);\n> > -\n> > -             if (!strcmp(sig.hash_algo, \"sha1\"))\n> > -                     store_signature(&sig_sha1, &sig, \"SHA-1\");\n> > -             else if (!strcmp(sig.hash_algo, \"sha256\"))\n> > -                     store_signature(&sig_sha256, &sig, \"SHA-256\");\n> > -             else\n> > -                     BUG(\"parse_one_signature() returned unknown hash algo\");\n>\n> And the call to `BUG()` is preexisting, as well. How about we move the\n> extraction of this loop into a separate commit?\n\nThere is no extraction of this code anymore in V2.\n\n> > +             struct strbuf data = STRBUF_INIT;\n> > +             switch (signed_commit_mode) {\n> > +             case SIGN_ABORT:\n> > +                     die(\"encountered signed commit; use \"\n> > +                         \"--signed-commits=<mode> to handle it\");\n>\n> This message should be marked for translation.\n\nOnly 6 out of 131 messages in die() functions are currently marked for\ntranslation. So I thought that it might be better to mark all messages\nfor translations in a separate series dedicated to that.\n\nAnyway in V2, all the messages in die(), warning() and such introduced\nby this series are marked for translation.\n\n> > @@ -3501,6 +3529,9 @@ static int parse_one_option(const char *option)\n> >               option_active_branches(option);\n> >       } else if (skip_prefix(option, \"export-pack-edges=\", &option)) {\n> >               option_export_pack_edges(option);\n> > +     } else if (skip_prefix(option, \"signed-commits=\", &option)) {\n> > +             if (parse_sign_mode(option, &signed_commit_mode))\n> > +                     die(\"unknown --signed-commits mode '%s'\", option);\n>\n> Do we want to use `usagef()` instead?\n\nOk, it's used in V2.\n\n> > +test_description='git fast-import --signed-commits=<mode>'\n> > +\n> > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>\n> There shouldn't be a need to specify the initial branch name. You\n> already create the initial commit with `test_commit()`, so the calls to\n> git-checkout(1) can instead say `git checkout -b openpgp-signign first`\n> because `test_commit()` creates that tag for us.\n\nI copy pasted a lot of test code from t9350, but yeah in V2 I fixed\nthis and the other issues you mentioned in this new test script.\n\nThanks.\n"},{"id":"526186","messageId":"CAP8UFD0EAuGdMWY2ff_kjaQnZ3w7VP2ftM6Y4Hr3T4HQTsFB3w@mail.gmail.com","threadId":"64117","inReplyTo":"xmqqtt1as08f.fsf@gitster.g","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-12T13:35:45Z","receivedAt":"2025-09-12T13:35:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Sep 10, 2025 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n> > diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> > @@ -2785,6 +2787,23 @@ static void store_signature(struct signature_data *stored_sig,\n> >       }\n> >  }\n> >\n> > +/* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n> > +static void import_signature(struct signature_data *sig_sha1,\n> > +                          struct signature_data *sig_sha256,\n> > +                          const char *v)\n> > +{\n> > +     struct signature_data sig = { NULL, NULL, STRBUF_INIT };\n> > +\n> > +     parse_one_signature(&sig, v);\n> > +\n> > +     if (!strcmp(sig.hash_algo, \"sha1\"))\n> > +             store_signature(sig_sha1, &sig, \"SHA-1\");\n> > +     else if (!strcmp(sig.hash_algo, \"sha256\"))\n> > +             store_signature(sig_sha256, &sig, \"SHA-256\");\n> > +     else\n> > +             BUG(\"parse_one_signature() returned unknown hash algo\");\n> > +}\n> > +\n>\n> THis is a new function; I am not sure if the division of labor\n> between this one and its caller is done right.  See below.\n\nI have removed this new function in V2 as I think I implemented what\nyou suggest below.\n\n> > @@ -2817,19 +2836,28 @@ static void parse_new_commit(const char *arg)\n> >       if (!committer)\n> >               die(\"Expected committer but didn't get one\");\n> >\n> > -     /* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n> >       while (skip_prefix(command_buf.buf, \"gpgsig \", &v)) {\n> > -             struct signature_data sig = { NULL, NULL, STRBUF_INIT };\n> > -\n> > -             parse_one_signature(&sig, v);\n> > -\n> > -             if (!strcmp(sig.hash_algo, \"sha1\"))\n> > -                     store_signature(&sig_sha1, &sig, \"SHA-1\");\n> > -             else if (!strcmp(sig.hash_algo, \"sha256\"))\n> > -                     store_signature(&sig_sha256, &sig, \"SHA-256\");\n> > -             else\n> > -                     BUG(\"parse_one_signature() returned unknown hash algo\");\n> > -\n> > +             struct strbuf data = STRBUF_INIT;\n> > +             switch (signed_commit_mode) {\n> > +             case SIGN_ABORT:\n> > +                     die(\"encountered signed commit; use \"\n> > +                         \"--signed-commits=<mode> to handle it\");\n> > +             case SIGN_WARN_VERBATIM:\n> > +                     warning(\"importing a commit signature\");\n> > +                     /* fallthru */\n> > +             case SIGN_VERBATIM:\n> > +                     import_signature(&sig_sha1, &sig_sha256, v);\n> > +                     break;\n> > +             case SIGN_WARN_STRIP:\n> > +                     warning(\"stripping a commit signature\");\n> > +                     /* fallthru */\n> > +             case SIGN_STRIP:\n> > +                     /* Read signature data and discard it */\n> > +                     read_next_command();\n> > +                     parse_data(&data, 0, NULL);\n> > +                     strbuf_release(&data);\n> > +                     break;\n> > +             }\n> >               read_next_command();\n> >       }\n>\n> I am not sure if this change had to be this way.  The old code\n> always called parse_one_signature(), which was responsible for\n> checking the signature format and then calling read_next_command()\n> and parse_data(), so no matter what happened afterwards, we know we\n> are consuming the data stream regardless of the conditional execution\n> that happens here.\n>\n> The new code calls import_signature() or an inlined sequence of\n> read_next_command() plus parse_data(), essentially making each case\n> arm in the switch() statement responsible individually for consuming\n> the incoming data.  When somebody adds a new case there to specify a\n> different way to handle signatures, they have to make sure that they\n> do not forget calling read_next_command() and parse_data() themselves.\n>\n> Even though I can see, after some code inspection, that no branches\n> in the current code forgets to advance the incoming data stream to\n> leave us out of sync right now, this change feels like a bit of code\n> ergonomics regression to me.  Was it so important that we pass a\n> broken signature without inspecting in STRIP mode?  I am guessing\n> that is the reason why the new code tries hard to avoid calling the\n> parse_one_signature() function in these case arms.\n\nYeah, I thought it was cleaner and a bit faster if we don't parse\nsignatures when in STRIP mode. That's why I did it like this.\n\nNow as it looks like we don't really mind parsing them, I implemented\nthat in V2. In ABORT mode, I still think it might be a bit better to\nABORT right away though, so that's what I implemented in V2.\n\n> An aside.  I think the warning message about importing should have a\n> word \"verbatim\" in it, e.g.\n>\n>         warning(\"importing a commit signature verbatim\");\n\nIn V2, I have changed the message to what you suggest.\n"},{"id":"526189","messageId":"CAP8UFD39y8Xde6CD=dDyYy3MY1NOaZGVuwk4zPcreA5KbKR1dA@mail.gmail.com","threadId":"64117","inReplyTo":"xmqqms72rwx4.fsf@gitster.g","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-12T13:41:20Z","receivedAt":"2025-09-12T13:41:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Sep 10, 2025 at 8:21 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Christian Couder <christian.couder@gmail.com> writes:\n\n> > +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::\n> > +     Specify how to handle signed commits.  Behaves in the same way\n> > +     as the same option in linkgit:git-fast-export[1], except that\n> > +     default is 'verbatim' (instead of 'abort').\n>\n> I forgot one thing.  Earlier in 1/2 we saw\n>\n>     The definition of 'enum sign_mode' as well as its parsing code are in\n>     \"builtin/fast-export.c\". This was fine because `git fast-export` was the\n>     only command with '--signed-tags=<mode>' or '--signed-commits=<mode>'\n>     options.\n>\n>     In a following commit, we are going to add such options to `git\n>     fast-import`, which will be simpler, easier and cleaner if we can reuse\n>     the 'enum sign_mode' defintion and parsing code.\n>\n> and I was implicitly expecting that both commits and tags would be\n> supported on the import side, but this only deals with the commits?\n>\n> Is patch [3/2] missing from the archive?\n\nYeah, the commit message was too optimistic about the features that\nwere going to be added by the next commit.\n\nIn V2, I have changed that paragraph in the commit message to:\n\n   \"In a following commit, we are going to add a similar option to `git\n   fast-import`, which will be simpler, easier and cleaner if we can reuse\n   the 'enum sign_mode' defintion and parsing code.\"\n\nAnd in the cover letter I added the following:\n\n\"In the future I also plan to add a similar `--signed-tags=<mode>` so\nthat the import of tags can also be controlled. But I prefer to\nvalidate the general design of a single new option first.\"\n\nI hope it's Ok.\n\nThanks.\n"},{"id":"526190","messageId":"CAP8UFD1ULfGqzQwn1ZhgCdS5Ri_Y3rd-F+=-zsRKtKFg0k4hVQ@mail.gmail.com","threadId":"64117","inReplyTo":"xmqq4it9ornz.fsf@gitster.g","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-12T13:47:21Z","receivedAt":"2025-09-12T13:47:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 11, 2025 at 6:55 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n> >> +/* Process signatures (up to 2: one \"sha1\" and one \"sha256\") */\n> >\n> > Hm. Does \"up to 2\" indicate that the commit may have two signatures at\n> > once? If so...\n> >\n> >> +static void import_signature(struct signature_data *sig_sha1,\n> >> +                         struct signature_data *sig_sha256,\n> >> +                         const char *v)\n> >> +{\n> >> +    struct signature_data sig = { NULL, NULL, STRBUF_INIT };\n> >> +\n> >> +    parse_one_signature(&sig, v);\n> >> +\n> >> +    if (!strcmp(sig.hash_algo, \"sha1\"))\n> >> +            store_signature(sig_sha1, &sig, \"SHA-1\");\n> >> +    else if (!strcmp(sig.hash_algo, \"sha256\"))\n> >> +            store_signature(sig_sha256, &sig, \"SHA-256\");\n> >\n> > ... then the code here seems to indicate otherwise as you only parse\n> > either the \"sha1\" signature or the \"sha256\" signature, but never both.\n>\n> Correct and not quite.  The caller can call you twice in its loop.\n> But if the input was malformed and had two \"sha1\" (and no \"sha256\"),\n> this will not barf (as the original, so it is not a new bug).\n\nstore_signature() should barf if it has already been called with the\nsame first argument:\n\nstatic void store_signature(struct signature_data *stored_sig,\n                struct signature_data *new_sig,\n                const char *hash_type)\n{\n    if (stored_sig->hash_algo) {\n        warning(\"multiple %s signatures found, \"\n            \"ignoring additional signature\",\n            hash_type);\n        strbuf_release(&new_sig->data);\n        free(new_sig->hash_algo);\n    } else {\n        *stored_sig = *new_sig;\n    }\n}\n\n> In any case, I also found that \"up to 2\" comment somewhat strange.\n> It was more understandable back when it was near the loop, but not\n> here.\n\nIn V2 I just removed that comment, as I am not sure it really helps.\n\nThanks.\n"},{"id":"526192","messageId":"xmqqfrcroj8w.fsf@gitster.g","threadId":"64117","inReplyTo":"CAP8UFD39y8Xde6CD=dDyYy3MY1NOaZGVuwk4zPcreA5KbKR1dA@mail.gmail.com","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-12T14:09:19Z","receivedAt":"2025-09-12T14:09:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> In V2, I have changed that paragraph in the commit message to:\n>\n>    \"In a following commit, we are going to add a similar option to `git\n>    fast-import`, which will be simpler, easier and cleaner if we can reuse\n>    the 'enum sign_mode' defintion and parsing code.\"\n>\n> And in the cover letter I added the following:\n>\n> \"In the future I also plan to add a similar `--signed-tags=<mode>` so\n> that the import of tags can also be controlled. But I prefer to\n> validate the general design of a single new option first.\"\n\nI understood the intentions.\n\nI am not sure if omitting signed-tags and doing only signed-commits\nis a sensible choice, as it does not seem to reduce the patch load\nany meaningful way.  Besides, because historically signed tags are\nmore heavily used than signed commits, if it made sense to do only\none of two, I would have somehow expected that tags would be done\nfirst.\n\nBut anyway, I understood why this was done only half of the scope as\na first step.\n\nThanks.\n"},{"id":"526193","messageId":"xmqqa52zoizs.fsf@gitster.g","threadId":"64117","inReplyTo":"CAP8UFD0EAuGdMWY2ff_kjaQnZ3w7VP2ftM6Y4Hr3T4HQTsFB3w@mail.gmail.com","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-12T14:14:47Z","receivedAt":"2025-09-12T14:14:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n>> ...  Was it so important that we pass a\n>> broken signature without inspecting in STRIP mode?  I am guessing\n>> that is the reason why the new code tries hard to avoid calling the\n>> parse_one_signature() function in these case arms.\n>\n> Yeah, I thought it was cleaner and a bit faster if we don't parse\n> signatures when in STRIP mode. That's why I did it like this.\n\nI do not think parsing performance matters all that much here, but I\nthink it is a good idea to recover from malformed signature lines if\nthe parsing code can detect some.  It is likely that the user may be\nusing STRIP hoping that they can skip bad or unknown signature data\nin the incoming stream, so it is beneficial to be lenient there.\n\n> Now as it looks like we don't really mind parsing them, I implemented\n> that in V2. In ABORT mode, I still think it might be a bit better to\n> ABORT right away though, so that's what I implemented in V2.\n>\n>> An aside.  I think the warning message about importing should have a\n>> word \"verbatim\" in it, e.g.\n>>\n>>         warning(\"importing a commit signature verbatim\");\n>\n> In V2, I have changed the message to what you suggest.\n\nSounds good.\n\nThanks.\n"},{"id":"526337","messageId":"CAP8UFD2RVZHqb4EwULmmR3hJJKuxajeOOLe5_p3DYcz2dmdzYg@mail.gmail.com","threadId":"64117","inReplyTo":"xmqqa52zoizs.fsf@gitster.g","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-15T10:29:28Z","receivedAt":"2025-09-15T10:29:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Sep 12, 2025 at 4:14 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n> >> ...  Was it so important that we pass a\n> >> broken signature without inspecting in STRIP mode?  I am guessing\n> >> that is the reason why the new code tries hard to avoid calling the\n> >> parse_one_signature() function in these case arms.\n> >\n> > Yeah, I thought it was cleaner and a bit faster if we don't parse\n> > signatures when in STRIP mode. That's why I did it like this.\n>\n> I do not think parsing performance matters all that much here, but I\n> think it is a good idea to recover from malformed signature lines if\n> the parsing code can detect some.  It is likely that the user may be\n> using STRIP hoping that they can skip bad or unknown signature data\n> in the incoming stream, so it is beneficial to be lenient there.\n\nYeah, parse_one_signature() expects a signature to be in the 'gpgsig\n<hash-algo> <signature-format>' and might die() in case it cannot\nvalidate <hash-algo> or <signature-format>.\n\nSo it seems to me that I should change the code back so that its\nbehavior is the same as in v1, while trying to avoid what you called\ncode ergonomics regression when you reviewed v1. Ok, let's see if I\nfind a way to do that.\n\nThanks.\n"},{"id":"526373","messageId":"xmqqecs7iu0a.fsf@gitster.g","threadId":"64117","inReplyTo":"CAP8UFD2RVZHqb4EwULmmR3hJJKuxajeOOLe5_p3DYcz2dmdzYg@mail.gmail.com","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-15T16:02:29Z","receivedAt":"2025-09-15T16:02:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> So it seems to me that I should change the code back so that its\n> behavior is the same as in v1, while trying to avoid what you called\n> code ergonomics regression when you reviewed v1. Ok, let's see if I\n> find a way to do that.\n\nSorry, the above reads like (but it is a bit fuzzy and oblique to me\nto be certain) I suggested a wrong approach on v1 which we are\nseeing in the version under review and I am suggesting to reverse\ncourse?  It is a bit too far apart between versions so I am not sure\nif that is what is happening, but if so, sorry for making you polish\nitrepeatedly.\n\n"},{"id":"526610","messageId":"CAP8UFD1yW4SW1Nj=UfBq89UHVGJi6zPuhUEd5XbPBfSBh+x4xg@mail.gmail.com","threadId":"64117","inReplyTo":"xmqqecs7iu0a.fsf@gitster.g","subject":"Re: [PATCH 2/2] fast-import: add '--signed-commits=<mode>' option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-09-17T18:27:56Z","receivedAt":"2025-09-17T18:28:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Sep 15, 2025 at 6:02 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n> > So it seems to me that I should change the code back so that its\n> > behavior is the same as in v1, while trying to avoid what you called\n> > code ergonomics regression when you reviewed v1. Ok, let's see if I\n> > find a way to do that.\n>\n> Sorry, the above reads like (but it is a bit fuzzy and oblique to me\n> to be certain) I suggested a wrong approach on v1 which we are\n> seeing in the version under review and I am suggesting to reverse\n> course?\n\nI just hoped to get hints in case you had an idea I didn't understand\nabout how the code should look like.\n\n> It is a bit too far apart between versions so I am not sure\n> if that is what is happening, but if so, sorry for making you polish\n> itrepeatedly.\n\nNo worries. I hope the V3 I just sent is better than both the v1 and\nv2, or at least that it moves us closer to something better than both\nof them.\n\nThanks.\n"}]}