[PATCH] show_one_mergetag: print non-parent in hex form.

[PATCH] show_one_mergetag: print non-parent in hex form.

[Harald van Dijk via GitGitGadget]

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.

Signed-off-by: Harald van Dijk <harald@gigawatt.nl>
---
    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 harald@gigawatt.nl [harald@gigawatt.nl]
    
    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 <torvalds@linux-foundation.org>
    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 <torvalds@linux-foundation.org>
    Date:   Thu Feb 27 11:26:33 2020 -0800

Published-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(-)

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 &&

base-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc
-- 
gitgitgadget

Re: [PATCH] show_one_mergetag: print non-parent in hex form.

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] show_one_mergetag: print non-parent in hex form.

[Martin Ågren]

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" &&
-- 
2.25.1

Re: [PATCH] show_one_mergetag: print non-parent in hex form.

[Harald van Dijk]

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

Re: [PATCH] show_one_mergetag: print non-parent in hex form.

[Martin Ågren]

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