On Wed, Jul 9, 2025 at 1:08 AM Elijah Newren <newren@xxxxxxxxx> wrote:
>
> On Tue, Jul 8, 2025 at 2:18 AM Christian Couder
> <christian.couder@xxxxxxxxx> 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!
