Re: [PATCH v2 4/8] merge-tree: implement real merges

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Elijah,


On Fri, 7 Jan 2022, Elijah Newren wrote:

> On Fri, Jan 7, 2022 at 7:30 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:
> >
> > >  SYNOPSIS
> > >  --------
> > >  [verse]
> > > +'git merge-tree' --real <branch1> <branch2>
> > >  'git merge-tree' <base-tree> <branch1> <branch2>
> >
> > Here is an idea: How about aiming for this synopsis instead, exploiting
> > the fact that the "real" mode takes a different amount of arguments?
>
> My turn on the grammar thing: s/amount/number/.   :-)

See? I know why I'm refraining from nitpicking. It's just not good for
anyone involved.

> > > diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> > > new file mode 100755
> > > index 00000000000..f7aa310f8c1
> > > --- /dev/null
> > > +++ b/t/t4301-merge-tree-real.sh
> > > @@ -0,0 +1,81 @@
> > > +#!/bin/sh
> > > +
> > > +test_description='git merge-tree --real'
> > > +
> > > +. ./test-lib.sh
> > > +
> > > +# This test is ort-specific
> > > +GIT_TEST_MERGE_ALGORITHM=ort
> > > +export GIT_TEST_MERGE_ALGORITHM
> >
> > It might make sense to skip the entire test if the user asked for
> > `recursive` to be tested:
> >
> >         test "${GIT_TEST_MERGE_ALGORITHM:-ort}" = ort ||
> >                 skip_all="GIT_TEST_MERGE_ALGORITHM != ort"
> >                 test_done
> >         }
>
> The idea makes sense, but it took me a bit to understand this code
> block.  I think you're just missing an opening left curly brace right
> after the '||'?

Yes. Sorry.

> > > +test_expect_success setup '
> > > +     test_write_lines 1 2 3 4 5 >numbers &&
> > > +     echo hello >greeting &&
> > > +     echo foo >whatever &&
> > > +     git add numbers greeting whatever &&
> > > +     git commit -m initial &&
> >
> > I would really like to encourage the use of `test_tick`. It makes the
> > commit consistent, just in case you run into an issue that depends on some
> > hash order.
>
> I've used test_tick before, but I already know this test can't depend
> on hash order.  Further, the hashes in the output are also replaced
> before comparing in order to make the tests also work as-is under
> sha256.  So the tests are explicitly ignoring precise hashes.  As
> such, I'm not sure I see the value of test_tick here.

Nevertheless. To make comparing logs of two different test runs easier, it
makes more sense to insist on consistency.

> > > +
> > > +     git branch side1 &&
> > > +     git branch side2 &&
> > > +
> > > +     git checkout side1 &&
> >
> > Please use `git switch -c side1` or `git checkout -b side1`: it is more
> > compact than `git branch ... && git checkout ...`.
>
> Yes, but less forgiving to later modification where I go and add
> additional commits on one of the sides, because...
>
> >
> > > +     test_write_lines 1 2 3 4 5 6 >numbers &&
> > > +     echo hi >greeting &&
> > > +     echo bar >whatever &&
> > > +     git add numbers greeting whatever &&
> > > +     git commit -m modify-stuff &&
> > > +
> > > +     git checkout side2 &&
> >
> > This could be written as `git checkout -b side2 HEAD^`, to make the setup
> > more succinct.
>
> ...the presumption of HEAD^ is hardcoded and has to be parsed by
> readers to understand the test.  It felt like more cognitive overhead
> to me, in addition to being less malleable.

Right. Different developers, different preferences. I wish we had a
standard way in the test suite to initialize a test setup that _everybody_
could agree to be succinct and helpful. So far, we use shell scripted Git
commands to recreate an initial commit topology, but especially when
comparing to existing test suites with fixtures that are not only
well-documented but also easy to wrap your head around, I find Git's test
suite awfully lacking. Mind you, the code _I_ introduced isn't stellar in
this respect, either, not by a very far stretch.

> > > +test_expect_success 'Barf on misspelled option' '
> > > +     # Mis-spell with single "s" instead of double "s"
> > > +     test_expect_code 129 git merge-tree --real --mesages FOOBAR side1 side2 2>expect &&
> > > +
> > > +     grep "error: unknown option.*mesages" expect
> > > +'
> >
> > I do not think that this test case adds much, and we already test the
> > `parse_options()` machinery elsewhere.
>
> It's more about verifying that exit codes of 0 & 1 are reserved for
> "completed with no conflicts" and "completed with conflicts".  The 129
> bit in this test is the important bit (and perhaps is well-known to
> lots of other folks, but I thought it was worth highlighting).

Fair enough.

Ciao,
Dscho