| Message ID | pull.568.git.1582981677312.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | show_one_mergetag: print non-parent in hex form. | expand |
On 2020-02-29 at 13:07:57, Harald van Dijk via GitGitGadget wrote: > From: Harald van Dijk <harald@gigawatt.nl> > > 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. > +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 I appreciate you computing this value with git rev-parse.
Hi Harald, On Sat, 29 Feb 2020 at 14:11, Harald van Dijk via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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.) > 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)); Looks obviously correct. > +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". > + git checkout --detach master && > + echo bbb >baz && > + git add baz && > + git commit -m baz_commit && Similarly here. > + 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 diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 20cb436c43..7441485e73 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -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" &&
Hi Martin, On 01/03/2020 15:59, Martin Ågren wrote: > 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. > 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, On Sun, 1 Mar 2020 at 21:27, Harald van Dijk <harald@gigawatt.nl> wrote: > On 01/03/2020 15:59, Martin Ågren wrote: > > 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. Yeah, that makes sense. Thanks for explaining. > > 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
diff --git a/log-tree.c b/log-tree.c index cae38dcc662..2a2db96d5ec 100644 --- a/log-tree.c +++ b/log-tree.c @@ -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); diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 192347a3e1f..20cb436c433 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -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 &&