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