| Message ID | 20230602023105.17979-1-five231003@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add new "signature" atom | expand |
Kousik Sanagavarapu <five231003@gmail.com> writes: > I have also rebased this to be on top of v2.41.0, the previous version > was on top of v2.41.0-rc0. I am still feverish and feeling weak so no real review from me yet, but there is one thing that immediately jumped at me. > Range-diff against v1: > > 1: 5c97d11b79 ! 1: 87465ef1a8 t/lib-gpg: introduce new prereq GPG2 > @@ t/lib-gpg.sh: test_lazy_prereq GPG ' > + test $? != 127 || exit 1 > + > + case "$gpg_version" in > -+ !"gpg (GnuPG) 2."*) > ++ "gpg (GnuPG) 0."* | "gpg (GnuPG) 1.*") The last '*' being inside double-quote would not be what you intended, I suspect?
On Fri, Jun 2, 2023 at 3:33 AM Junio C Hamano <gitster@pobox.com> wrote: > Kousik Sanagavarapu <five231003@gmail.com> writes: > > I have also rebased this to be on top of v2.41.0, the previous version > > was on top of v2.41.0-rc0. > > I am still feverish and feeling weak so no real review from me yet, > but there is one thing that immediately jumped at me. > > > 1: 5c97d11b79 ! 1: 87465ef1a8 t/lib-gpg: introduce new prereq GPG2 > > @@ t/lib-gpg.sh: test_lazy_prereq GPG ' > > + test $? != 127 || exit 1 > > + > > + case "$gpg_version" in > > -+ !"gpg (GnuPG) 2."*) > > ++ "gpg (GnuPG) 0."* | "gpg (GnuPG) 1.*") > > The last '*' being inside double-quote would not be what you > intended, I suspect? I noticed that, as well, when running my eye over the range-diff. Moreover, I wondered if using `[01]` to avoid the repetition would be worthwhile: case "$gpg_version" in "gpg (GnuPG) "[01].*) though, of course, it's subjective whether that is clearer.
On Fri, Jun 02, 2023 at 04:29:34PM +0900, Junio C Hamano wrote: > Kousik Sanagavarapu <five231003@gmail.com> writes: > > > I have also rebased this to be on top of v2.41.0, the previous version > > was on top of v2.41.0-rc0. > > I am still feverish and feeling weak so no real review from me yet, Please take care and get well soon. > but there is one thing that immediately jumped at me. > > > Range-diff against v1: > > > > 1: 5c97d11b79 ! 1: 87465ef1a8 t/lib-gpg: introduce new prereq GPG2 > > @@ t/lib-gpg.sh: test_lazy_prereq GPG ' > > + test $? != 127 || exit 1 > > + > > + case "$gpg_version" in > > -+ !"gpg (GnuPG) 2."*) > > ++ "gpg (GnuPG) 0."* | "gpg (GnuPG) 1.*") > > The last '*' being inside double-quote would not be what you > intended, I suspect? Yeah, that was kind of a typo, thanks for catching it. Thanks
Eric Sunshine <sunshine@sunshineco.com> writes: > Moreover, I wondered if using `[01]` to avoid the repetition would be > worthwhile: > > case "$gpg_version" in > "gpg (GnuPG) "[01].*) > > though, of course, it's subjective whether that is clearer. Excellent. I'd say that the value of your version is primarily that it is much less error prone than repeating the constant string part that can be misspelt. The glob limiting "begins with either '0' or '1' followed by a dot" might be slightly less easier to understand for less trained eyes, but eyes will not remain untrained forever, so it is OK.
Hi, Thanks for the review. Changes since v1: PATCH 1/2 - Changed the condition so that prereq GPG2 will only fail if we have GPG v0.* or v1.* instead of failing when we don't have v2.* (this will have an effect if in the future GPG v3.*, v4.* were introduced). PATCH 2/2 - Renamed the setup tests to be more clear about their purpose. Common to both the patches is the change where we introduce a newline to a file. Use "echo >" instead of "echo "" >". I have also rebased this to be on top of v2.41.0, the previous version was on top of v2.41.0-rc0. Range-diff against v1: 1: 5c97d11b79 ! 1: 87465ef1a8 t/lib-gpg: introduce new prereq GPG2 @@ t/lib-gpg.sh: test_lazy_prereq GPG ' + test $? != 127 || exit 1 + + case "$gpg_version" in -+ !"gpg (GnuPG) 2."*) ++ "gpg (GnuPG) 0."* | "gpg (GnuPG) 1.*") + say "This test requires a GPG version >= v2.0.0" + exit 1 + ;; @@ t/t7510-signed-commit.sh: test_expect_success GPG 'amending already signed commi +test_expect_success GPG2 'bare signature' ' + git verify-commit fifth-signed 2>expect && -+ echo "" >>expect && ++ echo >>expect && + git log -1 --format="%GG" fifth-signed >actual && + test_cmp expect actual +' 2: e89f14283d ! 2: 690869aa47 ref-filter: add new "signature" atom @@ t/t6300-for-each-ref.sh: test_expect_success 'git for-each-ref with non-existing +GRADE_FORMAT="%(signature:grade)%0a%(signature:key)%0a%(signature:signer)%0a%(signature:fingerprint)%0a%(signature:primarykeyfingerprint)" +TRUSTLEVEL_FORMAT="%(signature:trustlevel)%0a%(signature:key)%0a%(signature:signer)%0a%(signature:fingerprint)%0a%(signature:primarykeyfingerprint)" + -+test_expect_success GPG 'setup: signature gpg' ' ++test_expect_success GPG 'setup for signature atom using gpg' ' + git checkout -b signed && + + test_when_finished "test_unconfig commit.gpgSign" && @@ t/t6300-for-each-ref.sh: test_expect_success 'git for-each-ref with non-existing + git tag seventh-unsigned +' + -+test_expect_success GPGSSH 'setup: signature ssh' ' ++test_expect_success GPGSSH 'setup for signature atom using ssh' ' ++ test_when_finished "test_unconfig gpg.format user.signingkey" && ++ + test_config gpg.format ssh && + test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && + echo "8" >file && @@ t/t6300-for-each-ref.sh: test_expect_success 'git for-each-ref with non-existing + grep -Ev "checking the trustdb|PGP trust model" out.raw >out && + head -3 out >expect && + tail -1 out >>expect && -+ echo "" >>expect && ++ echo >>expect && + git for-each-ref refs/tags/first-signed \ + --format="%(signature)" >actual && + test_cmp expect actual Kousik Sanagavarapu (2): t/lib-gpg: introduce new prereq GPG2 ref-filter: add new "signature" atom Documentation/git-for-each-ref.txt | 27 ++++ ref-filter.c | 111 ++++++++++++++++- t/lib-gpg.sh | 21 ++++ t/t6300-for-each-ref.sh | 191 +++++++++++++++++++++++++++++ t/t7510-signed-commit.sh | 7 ++ 5 files changed, 355 insertions(+), 2 deletions(-)