From: Elijah Newren <newren@gmail.com>
Date: Mon, 14 Jul 2025 21:07:14 GMT
Subject: Re: [PATCH v6] fast-(import|export): improve on commit signature output format
Message-ID: <CABPp-BHM5afgiUf7GsTPWmrf_tm6mWnvHWMKiZPxApJzN-U8gg@mail.gmail.com>
In-Reply-To: <20250709141253.623563-1-christian.couder@gmail.com>

Sorry for the delay; noticed this in what's cooking and wanted to
double check whether I missed anything, and decided to add a comment
here...

On Wed, Jul 9, 2025 at 7:13 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
[...]
> Let's improve on these limitations by improving fast-export and
> fast-import so that:
>
>   - all the signatures are exported,
>   - at most one signature on the SHA-1 object and one on the SHA-256
>     are imported,
>   - if there is more than one signature on the SHA-1 object or on
>     the SHA-256 object, fast-import emits a warning for each
>     additional signature,
>   - the output format is "gpgsig <git-hash-algo> <signature-format>",
>     where <git-hash-algo> is the Git object format as before, and
>     <signature-format> is the signature type ("openpgp", "x509",
>     "ssh" or "unknown"),
>   - the output is properly documented.
>
[...]
>
> +test_expect_success GPGSM 'setup X.509 signed commit' '
> +
> +       git checkout -b x509-signing main &&
> +       test_config gpg.format x509 &&
> +       test_config user.signingkey $GIT_COMMITTER_EMAIL &&
> +       echo "X.509 content" >file &&
> +       git add file &&
> +       git commit -S -m "X.509 signed commit" &&
> +       X509_COMMIT=$(git rev-parse HEAD) &&
> +       git checkout main
> +
> +'
> +
> +test_expect_success GPGSM 'round-trip X.509 signed commit' '
> +
> +       git fast-export --signed-commits=verbatim x509-signing >output &&
> +       test_grep -E "^gpgsig $GIT_DEFAULT_HASH x509" output &&
> +       (
> +               cd new &&
> +               git fast-import &&
> +               git cat-file commit refs/heads/x509-signing >actual &&
> +               grep "^gpgsig" actual &&
> +               IMPORTED=$(git rev-parse refs/heads/x509-signing) &&
> +               test $X509_COMMIT = $IMPORTED
> +       ) <output
> +
[...]
> +
> +test_expect_success GPGSSH 'round-trip SSH signed commit' '
> +
> +       git fast-export --signed-commits=verbatim ssh-signing >output &&
> +       test_grep -E "^gpgsig $GIT_DEFAULT_HASH ssh" output &&
> +       (
> +               cd new &&
> +               git fast-import &&
> +               git cat-file commit refs/heads/ssh-signing >actual &&
> +               grep "^gpgsig" actual &&
> +               IMPORTED=$(git rev-parse refs/heads/ssh-signing) &&
> +               test $SSH_COMMIT = $IMPORTED
> +       ) <output
> +
> +'
> +

Overall, the patch looks great now.  There's just one little nit-pick
left; I'm still not a fan of tests of the form

  (
    cd dir &&
    git fast-import
    ...lots of other commands...
  ) <output

because I think the "<output" should really be moved to the "git
fast-import" line since it's only meant to be used there.

This series adds 2 such tests.  You did point out in the discussion on
v5 that the testsuite already uses this idiom and you wanted to match
existing style.  (Though there were only 2 tests previously that used
it, and you already modified both as part of this patch.)

However...we've been through enough rounds and this is really just a
nit-pick; I can submit a follow-on patch later to clean up the four
tests and see if others agree with me that this is an eyesore, or if
I'm just weird.

I think it's good to merge down.