show_one_mergetag: print non-parent in hex form.
PATCH 1/1 show_one_mergetag: print non-parent in hex form.
From: Harald van Dijk <[email protected]>
When a mergetag names a non-parent, which can occur after a shallow clone, its hash was previously printed as raw data. Print it in hex form instead.
---
show_one_mergetag: print non-parent in hex form.
When a mergetag names a non-parent, which can occur after a shallow
clone, its hash was previously printed as raw data. Print it in hex form
instead.
Signed-off-by: Harald van Dijk [email protected] [[email protected]]
Before, after a shallow clone of
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/:
$ git show -s --show-signature
commit a2f0b878c3ca531a1706cb2a8b079cea3b17bafc (grafted, HEAD -> master, origin/master, origin/HEAD)
tag kbuild-fixes-v5.6-2 names a non-parent 꼋<CB>)/<B9><A5>u{^L<8A><B7>u^_A<B0><A1>^D<F8>
No signature
Author: Linus Torvalds <[email protected]>
Date: Thu Feb 27 11:26:33 2020 -0800
[...]
After:
$ git show -s --show-signature
commit a2f0b878c3ca531a1706cb2a8b079cea3b17bafc (grafted, HEAD -> master, origin/master, origin/HEAD)
tag kbuild-fixes-v5.6-2 names a non-parent eabc8bcb292fb9a5757b0c8ab7751f41b0a104f8
No signature
Author: Linus Torvalds <[email protected]>
Date: Thu Feb 27 11:26:33 2020 -0800Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-568%2Fhvdijk%2Fmergetag-hex-form-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-568/hvdijk/mergetag-hex-form-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/568
log-tree.c | 2 +- t/t4202-log.sh | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-)
log-tree.c +1 -1
@@ -517,7 +517,7 @@ static int show_one_mergetag(struct commit *commit, "merged tag '%s'\n", tag->tag); else if ((nth = which_parent(&tag->tagged->oid, commit)) < 0)strbuf_addf (&verify_message,"tag %s names a non-parent %s\n" , - tag->tag, tag->tagged->oid.hash); + tag->tag,oid_to_hex (&tag->tagged->oid)); else strbuf_addf(&verify_message, "parent #%d, tagged '%s'\n", nth + 1, tag->tag);
t/t4202-log.sh +20 -0
@@ -1634,6 +1634,26 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' ' grep "^| | gpg: Good signature" actual ' +test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' ' + test_when_finished "git reset --hard && git checkout master" && + git checkout -b plain-shallow master && + echo aaa >bar && + git add bar && + git commit -m bar_commit && + git checkout --detach master && + echo bbb >baz && + git add baz && + git commit -m baz_commit && + git tag -s -m signed_tag_msg signed_tag_shallow && + hash=$(git rev-parse HEAD) && + git checkout plain-shallow && + git merge --no-ff -m msg signed_tag_shallow && + git clone --depth 1 --no-local . shallow && + test_when_finished "rm -rf shallow" && + git -C shallow log --graph --show-signature -n1 plain-shallow >actual && + grep "tag signed_tag_shallow names a non-parent $hash" actual +' + test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' ' test_when_finished "git reset --hard && git checkout master" && test_config gpg.format x509 &&
base-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc
Signature · 1 line
gitgitgadget
Quoted 5 lines · Harald van Dijk via GitGitGadget
From: Harald van Dijk <[email protected]>
When a mergetag names a non-parent, which can occur after a shallow clone, its hash was previously printed as raw data. Print it in hex form instead.
Seems like a good idea.
Quoted 8 lines
t/t4202-log.sh +8
-0+test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' ' + test_when_finished "git reset --hard && git checkout master" && + git checkout -b plain-shallow master && + echo aaa >bar && + git add bar && + git commit -m bar_commit && + git checkout --detach master && + echo bbb >baz &&
t/t4202-log.sh test_expect_success GPG 'log --graph --show-signature for merged tag' ' · line 1654
Quoted 10 lines
t/t4202-log.sh +10
-0+ git add baz && + git commit -m baz_commit && + git tag -s -m signed_tag_msg signed_tag_shallow && + hash=$(git rev-parse HEAD) && + git checkout plain-shallow && + git merge --no-ff -m msg signed_tag_shallow && + git clone --depth 1 --no-local . shallow && + test_when_finished "rm -rf shallow" && + git -C shallow log --graph --show-signature -n1 plain-shallow >actual && + grep "tag signed_tag_shallow names a non-parent $hash" actual
I appreciate you computing this value with git rev-parse.
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Hi Harald,
Quoted 3 lines · Harald van Dijk via GitGitGadget
When a mergetag names a non-parent, which can occur after a shallow clone, its hash was previously printed as raw data. Print it in hex form instead.
Minor nit: "..., its hash is printed as raw data. ...". (It *is* indeed being printed as raw data as you set out to write this patch.)
Review
Quoted 4 lines
else if ((nth =
which_parent(&tag->tagged->oid, commit)) < 0)
strbuf_addf(&verify_message, "tag %s names a non-parent %s\n",(quoted diff) +1
-1- tag->tag, tag->tagged->oid.hash); + tag->tag, oid_to_hex(&tag->tagged->oid));
Looks obviously correct.
Review
Quoted 6 lines
(quoted diff) +6
-0+test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' ' + test_when_finished "git reset --hard && git checkout master" && + git checkout -b plain-shallow master && + echo aaa >bar && + git add bar && + git commit -m bar_commit &&
These last three lines could be "test_commit bar".
Review
Quoted 4 lines
(quoted diff) +4
-0+ git checkout --detach master && + echo bbb >baz && + git add baz && + git commit -m baz_commit &&
Similarly here.
Review
Quoted 9 lines
(quoted diff) +9
-0+ git tag -s -m signed_tag_msg signed_tag_shallow && + hash=$(git rev-parse HEAD) && + git checkout plain-shallow && + git merge --no-ff -m msg signed_tag_shallow && + git clone --depth 1 --no-local . shallow && + test_when_finished "rm -rf shallow" && + git -C shallow log --graph --show-signature -n1 plain-shallow >actual && + grep "tag signed_tag_shallow names a non-parent $hash" actual +'
But I also wonder if we even need the "bar" commit. Similarly, "--detach"-ing when checking out master seems unnecessary, unless we're afraid of "messing up" master, by modifying the expectations of later tests? Was that something you were concerned about?
I realize the test you add is similar to the ones surrounding it. But it does look tempting to squash in the diff below. The test still fails before and passes after. What do you think about this? Does this correctly capture your scenario?
(I suppose even the "test_commit baz" could be dropped, but then this test needs to assume that "master" already contains a commit.)
Martin
t/t4202-log.sh +3 -9
@@ -1636,17 +1636,11 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' ' test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' ' test_when_finished "git reset --hard && git checkout master" && - git checkout -b plain-shallow master && - echo aaa >bar && - git add bar && - git commit -m bar_commit && - git checkout --detach master && - echo bbb >baz && - git add baz && - git commit -m baz_commit && + git checkout master && + test_commit baz && git tag -s -m signed_tag_msg signed_tag_shallow && hash=$(git rev-parse HEAD) && - git checkout plain-shallow && + git checkout -b plain-shallow HEAD~ && git merge --no-ff -m msg signed_tag_shallow && git clone --depth 1 --no-local . shallow && test_when_finished "rm -rf shallow" &&
Signature · 1 line
2.25.1
Hi Martin,
Quoted 4 lines · Martin Ågren
But I also wonder if we even need the "bar" commit. Similarly, "--detach"-ing when checking out master seems unnecessary, unless we're afraid of "messing up" master, by modifying the expectations of later tests? Was that something you were concerned about?
The "bar" commit was present in the test I based mine on, where it was equally unnecessary except possibly for making the test easier to understand. I have no feelings whether it is better to have it in or leave it out, other than that it should be consistent across tests.
Not messing up the master branch is what multiple tests in this file, specifically the test I based mine on, do. If the test I had based mine on and my test had both done
echo aaa >bar &&
git add bar &&
git commit -m bar_commit
on the master branch, whichever test appeared second would fail, because git commit would not detect any modifications. This seems needlessly fragile to me, so I agree with the pattern used by the existing tests.
Quoted 4 lines
I realize the test you add is similar to the ones surrounding it. But it does look tempting to squash in the diff below. The test still fails before and passes after. What do you think about this? Does this correctly capture your scenario?
Yeah, the test set up is literally a copy of the test immediately above, except modified not to use conflicting ref names.
It could be simplified, but in that case, the other tests should be simplified the same way, and I did not think it was appropriate to make such changes here.
Cheers,
Harald van Dijk
Hi Harald,
Quoted 15 lines · Harald van Dijk
Quoted 4 lines · Martin Ågren
But I also wonder if we even need the "bar" commit. Similarly, "--detach"-ing when checking out master seems unnecessary, unless we're afraid of "messing up" master, by modifying the expectations of later tests? Was that something you were concerned about?
The "bar" commit was present in the test I based mine on, where it was equally unnecessary except possibly for making the test easier to understand. I have no feelings whether it is better to have it in or leave it out, other than that it should be consistent across tests.
Not messing up the master branch is what multiple tests in this file, specifically the test I based mine on, do. If the test I had based mine on and my test had both done
Quoted 7 lines
echo aaa >bar && git add bar && git commit -m bar_commiton the master branch, whichever test appeared second would fail, because git commit would not detect any modifications. This seems needlessly fragile to me, so I agree with the pattern used by the existing tests.
Yeah, that makes sense. Thanks for explaining.
Quoted 11 lines
Quoted 4 lines
I realize the test you add is similar to the ones surrounding it. But it does look tempting to squash in the diff below. The test still fails before and passes after. What do you think about this? Does this correctly capture your scenario?
Yeah, the test set up is literally a copy of the test immediately above, except modified not to use conflicting ref names.
It could be simplified, but in that case, the other tests should be simplified the same way, and I did not think it was appropriate to make such changes here.
Ok, fair enough. I could see the argument for doing a preparatory patch to simplify and use "test_commit". But I won't be the one to insist on it. This is a bugfix for maint, so it makes sense to minimize the rewriting.
There is a (minor) cognitive burden with "create contents, add it, commit it" compared to "test_commit", beyond the larger line count. Because "test_commit" adds a tag to each commit, seeing such an open-coded variant of "test_commit", one may wonder if this is deliberate exactly to avoid those tags interfering with what the test wants to do. Especially here, since this is about tags, that suspicion might be even more motivated. (Spoiler: In this test or the one it's modeled after, this is not the case. They might just as well use test_commit.) Anyway, all of that is just musing.
Thanks
Martin