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

> 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/.   :-)

> > > 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 '||'?

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

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

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