{"thread":{"id":"52919","subject":"[PATCH] show_one_mergetag: print non-parent in hex form.","startedAt":"2020-02-29T13:08:04Z","lastAt":"2020-03-02T19:17:57Z","messageCount":5,"participants":["Harald van Dijk via GitGitGadget","brian m. carlson","Martin Ågren","Harald van Dijk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"392708","messageId":"pull.568.git.1582981677312.gitgitgadget@gmail.com","threadId":"52919","inReplyTo":null,"subject":"[PATCH] show_one_mergetag: print non-parent in hex form.","fromName":"Harald van Dijk via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-29T13:07:57Z","receivedAt":"2020-02-29T13:08:04Z","isPatch":true,"sender":{"key":"harald@gigawatt.nl","avatar":null},"body":"From: Harald van Dijk <harald@gigawatt.nl>\n\nWhen a mergetag names a non-parent, which can occur after a shallow\nclone, its hash was previously printed as raw data. Print it in hex form\ninstead.\n\nSigned-off-by: Harald van Dijk <harald@gigawatt.nl>\n---\n    show_one_mergetag: print non-parent in hex form.\n    \n    When a mergetag names a non-parent, which can occur after a shallow\n    clone, its hash was previously printed as raw data. Print it in hex form\n    instead.\n    \n    Signed-off-by: Harald van Dijk harald@gigawatt.nl [harald@gigawatt.nl]\n    \n    Before, after a shallow clone of \n    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/:\n    \n    $ git show -s --show-signature\n    commit a2f0b878c3ca531a1706cb2a8b079cea3b17bafc (grafted, HEAD -> master, origin/master, origin/HEAD)\n    tag kbuild-fixes-v5.6-2 names a non-parent 꼋<CB>)/<B9><A5>u{^L<8A><B7>u^_A<B0><A1>^D<F8>\n    No signature\n    Author: Linus Torvalds <torvalds@linux-foundation.org>\n    Date:   Thu Feb 27 11:26:33 2020 -0800\n    [...]\n    \n    After:\n    \n    $ git show -s --show-signature\n    commit a2f0b878c3ca531a1706cb2a8b079cea3b17bafc (grafted, HEAD -> master, origin/master, origin/HEAD)\n    tag kbuild-fixes-v5.6-2 names a non-parent eabc8bcb292fb9a5757b0c8ab7751f41b0a104f8\n    No signature\n    Author: Linus Torvalds <torvalds@linux-foundation.org>\n    Date:   Thu Feb 27 11:26:33 2020 -0800\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-568%2Fhvdijk%2Fmergetag-hex-form-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-568/hvdijk/mergetag-hex-form-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/568\n\n log-tree.c     |  2 +-\n t/t4202-log.sh | 20 ++++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex cae38dcc662..2a2db96d5ec 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -517,7 +517,7 @@ static int show_one_mergetag(struct commit *commit,\n \t\t\t    \"merged tag '%s'\\n\", tag->tag);\n \telse if ((nth = which_parent(&tag->tagged->oid, commit)) < 0)\n \t\tstrbuf_addf(&verify_message, \"tag %s names a non-parent %s\\n\",\n-\t\t\t\t    tag->tag, tag->tagged->oid.hash);\n+\t\t\t\t    tag->tag, oid_to_hex(&tag->tagged->oid));\n \telse\n \t\tstrbuf_addf(&verify_message,\n \t\t\t    \"parent #%d, tagged '%s'\\n\", nth + 1, tag->tag);\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 192347a3e1f..20cb436c433 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1634,6 +1634,26 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \tgrep \"^| | gpg: Good signature\" actual\n '\n \n+test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit checkout -b plain-shallow master &&\n+\techo aaa >bar &&\n+\tgit add bar &&\n+\tgit commit -m bar_commit &&\n+\tgit checkout --detach master &&\n+\techo bbb >baz &&\n+\tgit add baz &&\n+\tgit commit -m baz_commit &&\n+\tgit tag -s -m signed_tag_msg signed_tag_shallow &&\n+\thash=$(git rev-parse HEAD) &&\n+\tgit checkout plain-shallow &&\n+\tgit merge --no-ff -m msg signed_tag_shallow &&\n+\tgit clone --depth 1 --no-local . shallow &&\n+\ttest_when_finished \"rm -rf shallow\" &&\n+\tgit -C shallow log --graph --show-signature -n1 plain-shallow >actual &&\n+\tgrep \"tag signed_tag_shallow names a non-parent $hash\" actual\n+'\n+\n test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' '\n \ttest_when_finished \"git reset --hard && git checkout master\" &&\n \ttest_config gpg.format x509 &&\n\nbase-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc\n-- \ngitgitgadget\n"},{"id":"392722","messageId":"20200301020119.GE9290@camp.crustytoothpaste.net","threadId":"52919","inReplyTo":"pull.568.git.1582981677312.gitgitgadget@gmail.com","subject":"Re: [PATCH] show_one_mergetag: print non-parent in hex form.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-03-01T02:01:19Z","receivedAt":"2020-03-01T02:01:31Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-29 at 13:07:57, Harald van Dijk via GitGitGadget wrote:\n> From: Harald van Dijk <harald@gigawatt.nl>\n> \n> When a mergetag names a non-parent, which can occur after a shallow\n> clone, its hash was previously printed as raw data. Print it in hex form\n> instead.\n\nSeems like a good idea.\n\n> +test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' '\n> +\ttest_when_finished \"git reset --hard && git checkout master\" &&\n> +\tgit checkout -b plain-shallow master &&\n> +\techo aaa >bar &&\n> +\tgit add bar &&\n> +\tgit commit -m bar_commit &&\n> +\tgit checkout --detach master &&\n> +\techo bbb >baz &&\n> +\tgit add baz &&\n> +\tgit commit -m baz_commit &&\n> +\tgit tag -s -m signed_tag_msg signed_tag_shallow &&\n> +\thash=$(git rev-parse HEAD) &&\n> +\tgit checkout plain-shallow &&\n> +\tgit merge --no-ff -m msg signed_tag_shallow &&\n> +\tgit clone --depth 1 --no-local . shallow &&\n> +\ttest_when_finished \"rm -rf shallow\" &&\n> +\tgit -C shallow log --graph --show-signature -n1 plain-shallow >actual &&\n> +\tgrep \"tag signed_tag_shallow names a non-parent $hash\" actual\n\nI appreciate you computing this value with git rev-parse.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"392729","messageId":"20200301155937.7168-1-martin.agren@gmail.com","threadId":"52919","inReplyTo":"pull.568.git.1582981677312.gitgitgadget@gmail.com","subject":"Re: [PATCH] show_one_mergetag: print non-parent in hex form.","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-03-01T15:59:37Z","receivedAt":"2020-03-01T15:59:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Harald,\n\nOn Sat, 29 Feb 2020 at 14:11, Harald van Dijk via GitGitGadget <gitgitgadget@gmail.com> wrote:\n> When a mergetag names a non-parent, which can occur after a shallow\n> clone, its hash was previously printed as raw data. Print it in hex form\n> instead.\n\nMinor nit: \"..., its hash is printed as raw data. ...\". (It *is* indeed\nbeing printed as raw data as you set out to write this patch.)\n\n>         else if ((nth = which_parent(&tag->tagged->oid, commit)) < 0)\n>                 strbuf_addf(&verify_message, \"tag %s names a non-parent %s\\n\",\n> -                                   tag->tag, tag->tagged->oid.hash);\n> +                                   tag->tag, oid_to_hex(&tag->tagged->oid));\n\nLooks obviously correct.\n\n> +test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' '\n> +       test_when_finished \"git reset --hard && git checkout master\" &&\n> +       git checkout -b plain-shallow master &&\n> +       echo aaa >bar &&\n> +       git add bar &&\n> +       git commit -m bar_commit &&\n\nThese last three lines could be \"test_commit bar\".\n\n> +       git checkout --detach master &&\n> +       echo bbb >baz &&\n> +       git add baz &&\n> +       git commit -m baz_commit &&\n\nSimilarly here.\n\n> +       git tag -s -m signed_tag_msg signed_tag_shallow &&\n> +       hash=$(git rev-parse HEAD) &&\n> +       git checkout plain-shallow &&\n> +       git merge --no-ff -m msg signed_tag_shallow &&\n> +       git clone --depth 1 --no-local . shallow &&\n> +       test_when_finished \"rm -rf shallow\" &&\n> +       git -C shallow log --graph --show-signature -n1 plain-shallow >actual &&\n> +       grep \"tag signed_tag_shallow names a non-parent $hash\" actual\n> +'\n\nBut I also wonder if we even need the \"bar\" commit. Similarly,\n\"--detach\"-ing when checking out master seems unnecessary, unless we're\nafraid of \"messing up\" master, by modifying the expectations of later\ntests? Was that something you were concerned about?\n\nI realize the test you add is similar to the ones surrounding it. But it\ndoes look tempting to squash in the diff below. The test still fails\nbefore and passes after. What do you think about this? Does this\ncorrectly capture your scenario?\n\n(I suppose even the \"test_commit baz\" could be dropped, but then this\ntest needs to assume that \"master\" already contains a commit.)\n\nMartin\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 20cb436c43..7441485e73 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1636,17 +1636,11 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \n test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' '\n \ttest_when_finished \"git reset --hard && git checkout master\" &&\n-\tgit checkout -b plain-shallow master &&\n-\techo aaa >bar &&\n-\tgit add bar &&\n-\tgit commit -m bar_commit &&\n-\tgit checkout --detach master &&\n-\techo bbb >baz &&\n-\tgit add baz &&\n-\tgit commit -m baz_commit &&\n+\tgit checkout master &&\n+\ttest_commit baz &&\n \tgit tag -s -m signed_tag_msg signed_tag_shallow &&\n \thash=$(git rev-parse HEAD) &&\n-\tgit checkout plain-shallow &&\n+\tgit checkout -b plain-shallow HEAD~ &&\n \tgit merge --no-ff -m msg signed_tag_shallow &&\n \tgit clone --depth 1 --no-local . shallow &&\n \ttest_when_finished \"rm -rf shallow\" &&\n-- \n2.25.1\n\n"},{"id":"392731","messageId":"7330146a-5a4f-17cc-cb26-f90d4060c0ee@gigawatt.nl","threadId":"52919","inReplyTo":"20200301155937.7168-1-martin.agren@gmail.com","subject":"Re: [PATCH] show_one_mergetag: print non-parent in hex form.","fromName":"Harald van Dijk","fromEmail":"harald@gigawatt.nl","sentAt":"2020-03-01T20:25:03Z","receivedAt":"2020-03-01T20:33:19Z","isPatch":true,"sender":{"key":"harald@gigawatt.nl","avatar":null},"body":"Hi Martin,\n\nOn 01/03/2020 15:59, Martin Ågren wrote:\n> But I also wonder if we even need the \"bar\" commit. Similarly,\n> \"--detach\"-ing when checking out master seems unnecessary, unless we're\n> afraid of \"messing up\" master, by modifying the expectations of later\n> tests? Was that something you were concerned about?\n\nThe \"bar\" commit was present in the test I based mine on, where it was \nequally unnecessary except possibly for making the test easier to \nunderstand. I have no feelings whether it is better to have it in or \nleave it out, other than that it should be consistent across tests.\n\nNot messing up the master branch is what multiple tests in this file, \nspecifically the test I based mine on, do. If the test I had based mine \non and my test had both done\n\n   echo aaa >bar &&\n   git add bar &&\n   git commit -m bar_commit\n\non the master branch, whichever test appeared second would fail, because \ngit commit would not detect any modifications. This seems needlessly \nfragile to me, so I agree with the pattern used by the existing tests.\n\n> I realize the test you add is similar to the ones surrounding it. But it\n> does look tempting to squash in the diff below. The test still fails\n> before and passes after. What do you think about this? Does this\n> correctly capture your scenario?\n\nYeah, the test set up is literally a copy of the test immediately above, \nexcept modified not to use conflicting ref names.\n\nIt could be simplified, but in that case, the other tests should be \nsimplified the same way, and I did not think it was appropriate to make \nsuch changes here.\n\nCheers,\nHarald van Dijk\n"},{"id":"392750","messageId":"CAN0heSpvyTtLouSvocBVXKz-67nvb_58mXJ8jpXr=zE8YsX=1g@mail.gmail.com","threadId":"52919","inReplyTo":"7330146a-5a4f-17cc-cb26-f90d4060c0ee@gigawatt.nl","subject":"Re: [PATCH] show_one_mergetag: print non-parent in hex form.","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-03-02T19:17:42Z","receivedAt":"2020-03-02T19:17:57Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Harald,\n\nOn Sun, 1 Mar 2020 at 21:27, Harald van Dijk <harald@gigawatt.nl> wrote:\n> On 01/03/2020 15:59, Martin Ågren wrote:\n> > But I also wonder if we even need the \"bar\" commit. Similarly,\n> > \"--detach\"-ing when checking out master seems unnecessary, unless we're\n> > afraid of \"messing up\" master, by modifying the expectations of later\n> > tests? Was that something you were concerned about?\n>\n> The \"bar\" commit was present in the test I based mine on, where it was\n> equally unnecessary except possibly for making the test easier to\n> understand. I have no feelings whether it is better to have it in or\n> leave it out, other than that it should be consistent across tests.\n>\n> Not messing up the master branch is what multiple tests in this file,\n> specifically the test I based mine on, do. If the test I had based mine\n> on and my test had both done\n>\n>    echo aaa >bar &&\n>    git add bar &&\n>    git commit -m bar_commit\n>\n> on the master branch, whichever test appeared second would fail, because\n> git commit would not detect any modifications. This seems needlessly\n> fragile to me, so I agree with the pattern used by the existing tests.\n\nYeah, that makes sense. Thanks for explaining.\n\n> > I realize the test you add is similar to the ones surrounding it. But it\n> > does look tempting to squash in the diff below. The test still fails\n> > before and passes after. What do you think about this? Does this\n> > correctly capture your scenario?\n>\n> Yeah, the test set up is literally a copy of the test immediately above,\n> except modified not to use conflicting ref names.\n>\n> It could be simplified, but in that case, the other tests should be\n> simplified the same way, and I did not think it was appropriate to make\n> such changes here.\n\nOk, fair enough. I could see the argument for doing a preparatory patch\nto simplify and use \"test_commit\". But I won't be the one to insist on\nit. This is a bugfix for maint, so it makes sense to minimize the\nrewriting.\n\nThere is a (minor) cognitive burden with \"create contents, add it,\ncommit it\" compared to \"test_commit\", beyond the larger line count.\nBecause \"test_commit\" adds a tag to each commit, seeing such an\nopen-coded variant of \"test_commit\", one may wonder if this is\ndeliberate exactly to avoid those tags interfering with what the test\nwants to do. Especially here, since this is about tags, that suspicion\nmight be even more motivated. (Spoiler: In this test or the one it's\nmodeled after, this is not the case. They might just as well use\ntest_commit.) Anyway, all of that is just musing.\n\nThanks\nMartin\n"}]}