From: Christian Couder <christian.couder@gmail.com>
Date: Wed, 09 Jul 2025 10:15:49 GMT
Subject: Re: [PATCH v5] fast-(import|export): improve on commit signature output format
Message-ID: <CAP8UFD2qpFTy0jtrKk4iQ1C+7dZDV7Abqrqz3J6qx2T4Jy0ESA@mail.gmail.com>
In-Reply-To: <CABPp-BF6OvH8oh=jG_8fWoC5gW+9E+wx=uDEk1uerJTOva5isg@mail.gmail.com>

On Wed, Jul 9, 2025 at 1:08 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Tue, Jul 8, 2025 at 2:18 AM Christian Couder
> <christian.couder@gmail.com> wrote:

> > diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
> > index 250d866652..89dec1108f 100644
> > --- a/Documentation/git-fast-import.adoc
> > +++ b/Documentation/git-fast-import.adoc
> > @@ -445,7 +445,7 @@ one).
> >         original-oid?
> >         ('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
> >         'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> > -       ('gpgsig' SP <alg> LF data)?
> > +       ('gpgsig' SP <algo> SP <format> LF data)?
> >         ('encoding' SP <encoding> LF)?
> >         data
> >         ('from' SP <commit-ish> LF)?
> > @@ -518,13 +518,37 @@ their syntax.
> >  ^^^^^^^^
> >
> >  The optional `gpgsig` command is used to include a PGP/GPG signature
> > -that signs the commit data.
> > +or other cryptographic signature that signs the commit data.
> >
> > -Here <alg> specifies which hashing algorithm is used for this
> > -signature, either `sha1` or `sha256`.
> > +....
> > +       'gpgsig' SP <git-hash-algo> SP <signature-format> LF
> > +       data
>
> Should the `data` be moved to the line above, just to make it clear
> it's associated with it?  (Similar to the first line you changed in
> git-fast-import.adoc?)

Yeah, I have changed it in my current version.

> > +....
> > +
> > +The `gpgsig` command takes two arguments:
> > +
> > +* `<git-hash-algo>` specifies which Git object format this signature
> > +  applies to, either `sha1` or `sha256`. This allows to know which
> > +  representation of the commit was signed (the SHA-1 or the SHA-256
> > +  version) which helps with both signature verification and
> > +  interoperability between repos with different hash functions.
>
> Should there also be a note added that fast-import is limited on what
> signatures it can verify if extensions.compatObjectFormat is not set?

It cannot yet verify signatures, but yeah it's probably a good idea to
say that setting this config option might help with verifying
signatures when it will be able to do it. See below.

> > +* `<signature-format>` specifies the type of signature, such as
> > +  `openpgp`, `x509`, `ssh`, or `unknown`. This is a convenience for
> > +  tools that process the stream, so they don't have to parse the ASCII
> > +  armor to identify the signature type.
> > +
> > +A commit may have at most one signature for the SHA-1 object format
> > +(stored in the "gpgsig" header) and one for the SHA-256 object format
> > +(stored in the "gpgsig-sha256" header).
> > +
> > +See below for a detailed description of the `data` command which
> > +contains the raw signature data.
> > +
> > +Signatures are not yet checked in the current implementation though.
>
> ...or maybe that extra note could be added as a parenthetical comment
> here for now?

Yeah, in my current version, there is:

"Signatures are not yet checked in the current implementation
though. (Already setting the `extensions.compatObjectFormat`
configuration option might help with verifying both SHA-1 and SHA-256
object format signatures when it will be implemented.)"

[...]


> > +test_expect_success GPG 'export and import of doubly signed commit' '
> > +       git -C explicit-sha256 fast-export --signed-commits=verbatim dual-signed >output &&
> > +       test_grep -E "^gpgsig sha1 openpgp" output &&
> > +       test_grep -E "^gpgsig sha256 openpgp" output &&
> > +
> > +       (
> > +               cd new &&
> > +               git fast-import &&
> > +               git cat-file commit refs/heads/dual-signed >actual &&
> > +               test_grep -E "^gpgsig " actual &&
> > +               test_grep -E "^gpgsig-sha256 " actual &&
> > +               IMPORTED=$(git rev-parse refs/heads/dual-signed) &&
> > +               if test "$GIT_DEFAULT_HASH" = "sha1"
> > +               then
> > +                       test $SHA1_B = $IMPORTED
> > +               else
> > +                       test $SHA256_B = $IMPORTED
> > +               fi
> > +       ) <output
>
> This last bit seems a bit fragile; could the redirection of output to
> the stdin of `git fast-import` be made specific to that one line
> instead of to the whole range of commands?

I used the same style as many other tests in this file. For example
there are already:

test_expect_success GPG 'signed-commits=verbatim' '

    git fast-export --signed-commits=verbatim --reencode=no
commit-signing >output &&
    grep "^gpgsig sha" output &&
    grep "encoding ISO-8859-1" output &&
    (
        cd new &&
        git fast-import &&
        STRIPPED=$(git rev-parse --verify refs/heads/commit-signing) &&
        test $COMMIT_SIGNING = $STRIPPED
    ) <output

'

test_expect_success GPG 'signed-commits=warn-verbatim' '

    git fast-export --signed-commits=warn-verbatim --reencode=no
commit-signing >output 2>err &&
    grep "^gpgsig sha" output &&
    grep "encoding ISO-8859-1" output &&
    test -s err &&
    (
        cd new &&
        git fast-import &&
        STRIPPED=$(git rev-parse --verify refs/heads/commit-signing) &&
        test $COMMIT_SIGNING = $STRIPPED
    ) <output

'

etc.

So to keep a consistent style in the tests, I would likely have to add
a preparatory commit that changes the style of all these existing
tests. Not sure it's worth it at this point.

> Otherwise, I very much appreciate the work to create a testcase with
> both signature types on a single commit.

Thanks for the kind words and for your reviews!