show_one_mergetag: print non-parent in hex form.

[hvdijk]

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]

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

[gitgitgadget]

Welcome to GitGitGadget

Hi @hvdijk, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[dscho]

/allow

[gitgitgadget]

User hvdijk is now allowed to use GitGitGadget.

[hvdijk]

/submit

[gitgitgadget]

Submitted as [email protected]

[gitgitgadget]

On the Git mailing list, "brian m. carlson" wrote (reply to this):

--tmoQ0UElFV5VgXgH
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On 2020-02-29 at 13:07:57, Harald van Dijk via GitGitGadget wrote:
> From: Harald van Dijk <[email protected]>
>=20
> 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=3D$(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.
--=20
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

--tmoQ0UElFV5VgXgH
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.2.19 (GNU/Linux)

iHUEABYKAB0WIQQILOaKnbxl+4PRw5F8DEliiIeigQUCXlsXbgAKCRB8DEliiIei
gZ0PAQCuHopz3NbTm5DCd7Rrl87Y9G/QIqxhXi5Ex/3eeLc+4gD/ZQBOwnY6v5DX
h4FaApla9N5ydtyRxglkCYFSFc9SVwg=
=+KTj
-----END PGP SIGNATURE-----

--tmoQ0UElFV5VgXgH--

[gitgitgadget]

On the Git mailing list, Martin Ågren wrote (reply to this):

Hi Harald,

On Sat, 29 Feb 2020 at 14:11, Harald van Dijk via GitGitGadget <[email protected]> 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

[gitgitgadget]

On the Git mailing list, Harald van Dijk wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Martin Ågren wrote (reply to this):

Hi Harald,

On Sun, 1 Mar 2020 at 21:27, Harald van Dijk <[email protected]> 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

[gitgitgadget]

This branch is now known as hd/show-one-mergetag-fix.

[gitgitgadget]

This patch series was integrated into pu via git@90a5186.

[gitgitgadget]

This patch series was integrated into pu via git@3cd381a.

[gitgitgadget]

This patch series was integrated into next via git@12fe907.

[gitgitgadget]

This patch series was integrated into pu via git@5b2e646.

[gitgitgadget]

This patch series was integrated into pu via git@5b67c37.

[gitgitgadget]

This patch series was integrated into pu via git@41aeb8f.

[gitgitgadget]

This patch series was integrated into pu via git@e85a596.

[gitgitgadget]

This patch series was integrated into pu via git@3658d77.

[gitgitgadget]

This patch series was integrated into next via git@3658d77.

[gitgitgadget]

This patch series was integrated into master via git@3658d77.

[gitgitgadget]

Closed via 3658d77.