Re: [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag

From: Martin Ågren <martin.agren@gmail.com>
Date: Jan 20, 2019, 09:02 UTC
Message-ID: <CAN0heSr3a9H46j3wiTwwbw7HFh4+4aFs5-qe=gtxYB3vC73KAA@mail.gmail.com>
In-Reply-To: <20190119232334.31646-2-brandon1024.br@gmail.com>

Hi Brandon,

On Sun, 20 Jan 2019 at 00:24, Brandon Richardson <brandon1024.br@gmail.com> wrote:

Show 14 quoted lines

>         # explicit -S of course must sign.
>         echo 10 | git commit-tree -S HEAD^{tree} >oid &&
>         test_line_count = 1 oid &&
> -       git tag tenth-signed $(cat oid)
> +       git tag tenth-signed $(cat oid) &&
> +
> +       # --gpg-sign[=<key-id>] must sign.
> +       echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid &&
> +       test_line_count = 1 oid &&
> +       git tag eleventh-signed $(cat oid) &&
> +       echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid &&
> +       test_line_count = 1 oid &&
> +       git tag twelfth-signed-alt $(cat oid)
>  '

Thank you for following through.

Let's see if there any opinions from others about this more verbose construction, vs placing the oid in a variable and quoting it. We obviously went several years without realizing that using $(...) as an object id risked falling back to HEAD and that a completely broken `git commit-tree -S` would pass the test. So being over-careful and extra obvious might very well be the right thing.

Show 18 quoted lines

>  test_expect_success GPG 'verify and show signatures' '
>         (
>                 for commit in initial second merge fourth-signed \
> -                       fifth-signed sixth-signed seventh-signed tenth-signed
> +                       fifth-signed sixth-signed seventh-signed tenth-signed \
> +                       eleventh-signed
>                 do
>                         git verify-commit $commit &&
>                         git show --pretty=short --show-signature $commit >actual &&
> @@ -82,7 +91,7 @@ test_expect_success GPG 'verify and show signatures' '
>                 done
>         ) &&
>         (
> -               for commit in eighth-signed-alt
> +               for commit in eighth-signed-alt twelfth-signed-alt
>                 do
>                         git show --pretty=short --show-signature $commit >actual &&
>                         grep "Good signature from" actual &&

Ah, good catch. I didn't notice that we had a separate for-loop for this key. This comes from 4baf839fe0 ("t7510: test a commit signed by an unknown key", 2014-06-16). What we want to test here is something different, namely that we're using a specific, named key. But FWIW, I think we're fine, and that we're not abusing the existing difference between these two loops too much.

Martin