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

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

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

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

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