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

From: Martin Ågren <martin.agren@gmail.com>
Date: Mar 2, 2020, 19:17 UTC
Message-ID: <CAN0heSpvyTtLouSvocBVXKz-67nvb_58mXJ8jpXr=zE8YsX=1g@mail.gmail.com>
In-Reply-To: <7330146a-5a4f-17cc-cb26-f90d4060c0ee@gigawatt.nl>

Hi Harald,

On Sun, 1 Mar 2020 at 21:27, Harald van Dijk <harald@gigawatt.nl> wrote:

Show 22 quoted lines

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

Show 11 quoted 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