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

[Elijah Newren <newren@gmail.com>]

On Fri, Jan 7, 2022 at 7:30 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > This adds the ability to perform real merges rather than just trivial
> > merges (meaning handling three way content merges, recursive ancestor
> > consolidation, renames, proper directory/file conflict handling, and so
> > forth).  However, unlike `git merge`, the working tree and index are
> > left alone and no branch is updated.
> >
> > The only output is:
> >   - the toplevel resulting tree printed on stdout
> >   - exit status of 0 (clean) or 1 (conflicts present)
> >
> > This output is mean to be used by some higher level script, perhaps in a
>                  ^^^^
>
> My apologies for pointing out a grammar issue: This probably intended to
> say "meant", as the word "mean" changes the sense of the sentence.

Oops.  Yeah, I'll correct that; thanks for pointing it out.

> In my defense, I have more substantial suggestions below.
>
> > sequence of steps like this:
> >
> >    NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
> >    test $? -eq 0 || die "There were conflicts..."
> >    NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 -p $BRANCH2)
> >    git update-ref $BRANCH1 $NEWCOMMIT
> >
> > Note that higher level scripts may also want to access the
> > conflict/warning messages normally output during a merge, or have quick
> > access to a list of files with conflicts.  That is not available in this
> > preliminary implementation, but subsequent commits will add that
> > ability.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  Documentation/git-merge-tree.txt | 28 +++++++----
> >  builtin/merge-tree.c             | 55 +++++++++++++++++++++-
> >  t/t4301-merge-tree-real.sh       | 81 ++++++++++++++++++++++++++++++++
> >  3 files changed, 153 insertions(+), 11 deletions(-)
> >  create mode 100755 t/t4301-merge-tree-real.sh
> >
> > diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> > index 58731c19422..5823938937f 100644
> > --- a/Documentation/git-merge-tree.txt
> > +++ b/Documentation/git-merge-tree.txt
> > @@ -3,26 +3,34 @@ git-merge-tree(1)
> >
> >  NAME
> >  ----
> > -git-merge-tree - Show three-way merge without touching index
> > +git-merge-tree - Perform merge without touching index or working tree
> >
> >
> >  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/.   :-)

>
>    'git merge-tree' [--write-tree] <branch1> <branch2>
>    'git merge-tree' [--demo-trivial-merge] <base-tree> <branch1> <branch2>
>
> That way, the old mode can still function, and can even at some stage be
> deprecated and eventually removed.

Ooh, interesting.

> >
> >  DESCRIPTION
> >  -----------
> > -Reads three tree-ish, and output trivial merge results and
> > -conflicting stages to the standard output.  This is similar to
> > -what three-way 'git read-tree -m' does, but instead of storing the
> > -results in the index, the command outputs the entries to the
> > -standard output.
> > +Performs a merge, but does not make any new commits and does not read
> > +from or write to either the working tree or index.
> >
> > -This is meant to be used by higher level scripts to compute
> > -merge results outside of the index, and stuff the results back into the
> > -index.  For this reason, the output from the command omits
> > -entries that match the <branch1> tree.
> > +The first form will merge the two branches, doing a full recursive
> > +merge with rename detection.  If the merge is clean, the exit status
> > +will be `0`, and if the merge has conflicts, the exit status will be
> > +`1`.  The output will consist solely of the resulting toplevel tree
> > +(which may have files including conflict markers).
> > +
> > +The second form is meant for backward compatibility and will only do a
> > +trival merge.  It reads three tree-ish, and outputs trivial merge
> > +results and conflicting stages to the standard output in a semi-diff
> > +format.  Since this was designed for higher level scripts to consume
> > +and merge the results back into the index, it omits entries that match
> > +<branch1>.  The result of this second form is is similar to what
> > +three-way 'git read-tree -m' does, but instead of storing the results
> > +in the index, the command outputs the entries to the standard output.
> >
> >  GIT
> >  ---
> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > index e1d2832c809..ac50f3d108b 100644
> > --- a/builtin/merge-tree.c
> > +++ b/builtin/merge-tree.c
> > @@ -2,6 +2,9 @@
> >  #include "builtin.h"
> >  #include "tree-walk.h"
> >  #include "xdiff-interface.h"
> > +#include "help.h"
> > +#include "commit-reach.h"
> > +#include "merge-ort.h"
> >  #include "object-store.h"
> >  #include "parse-options.h"
> >  #include "repository.h"
> > @@ -392,7 +395,57 @@ struct merge_tree_options {
> >  static int real_merge(struct merge_tree_options *o,
> >                     const char *branch1, const char *branch2)
> >  {
> > -     die(_("real merges are not yet implemented"));
> > +     struct commit *parent1, *parent2;
> > +     struct commit_list *common;
> > +     struct commit_list *merge_bases = NULL;
> > +     struct commit_list *j;
> > +     struct merge_options opt;
> > +     struct merge_result result = { 0 };
> > +
> > +     parent1 = get_merge_parent(branch1);
> > +     if (!parent1)
> > +             help_unknown_ref(branch1, "merge",
> > +                              _("not something we can merge"));
> > +
> > +     parent2 = get_merge_parent(branch2);
> > +     if (!parent2)
> > +             help_unknown_ref(branch2, "merge",
> > +                              _("not something we can merge"));
> > +
> > +     init_merge_options(&opt, the_repository);
> > +     /*
> > +      * TODO: Support subtree and other -X options?
> > +     if (use_strategies_nr == 1 &&
> > +         !strcmp(use_strategies[0]->name, "subtree"))
> > +             opt.subtree_shift = "";
> > +     for (x = 0; x < xopts_nr; x++)
> > +             if (parse_merge_opt(&opt, xopts[x]))
> > +                     die(_("Unknown strategy option: -X%s"), xopts[x]);
> > +     */
> > +
> > +     opt.show_rename_progress = 0;
> > +
> > +     opt.branch1 = merge_remote_util(parent1)->name; /* or just branch1? */
> > +     opt.branch2 = merge_remote_util(parent2)->name; /* or just branch2? */
> > +
> > +     /*
> > +      * Get the merge bases, in reverse order; see comment above
> > +      * merge_incore_recursive in merge-ort.h
> > +      */
> > +     common = get_merge_bases(parent1, parent2);
> > +     for (j = common; j; j = j->next)
> > +             commit_list_insert(j->item, &merge_bases);
> > +
> > +     /*
> > +      * TODO: notify if merging unrelated histories?
>
> I guess that it would make most sense to add a flag whether this is
> allowed or not, and I would suggest the default to be `off`.

Sounds fair.  Thanks for commenting on one of the TODOs that I was unsure about.

> > +     if (!common)
> > +             fprintf(stderr, _("merging unrelated histories"));
> > +      */
> > +
> > +     merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> > +     printf("%s\n", oid_to_hex(&result.tree->object.oid));
> > +     merge_switch_to_result(&opt, NULL, &result, 0, 0);
>
> This looks to be idempotent to `merge_finalize(&opt, &result)`, so maybe
> use that instead?

Yeah, and add a TODO about the display messages (that'll be addressed
in a later patch, unlike the above TODOs).

>
> > +     return result.clean ? 0 : 1;
> >  }
> >
> >  int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> > 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_write_lines 0 1 2 3 4 5 >numbers &&
> > +     echo yo >greeting &&
> > +     git rm whatever &&
> > +     mkdir whatever &&
> > +     >whatever/empty &&
> > +     git add numbers greeting whatever/empty &&
> > +     git commit -m other-modifications
> > +'
> > +
> > +test_expect_success 'Content merge and a few conflicts' '
> > +     git checkout side1^0 &&
> > +     test_must_fail git merge side2 &&
> > +     cp .git/AUTO_MERGE EXPECT &&
> > +     E_TREE=$(cat EXPECT) &&
>
> The file `EXPECT` is not used below. And can we use a more obvious name?
> SOmething like:
>
>         expected_tree=$(cat .git/AUTO_MERGE)

There go my beautiful <80 character lines below.  :-(

But on a more serious note, yeah this is probably better.  I'll change it.  :-)

>
> > +     git reset --hard &&
>
> For an extra bonus, we could delay this via `test_when_finished`, to prove
> that `git merge-tree --real` works even in a dirty worktree _with
> conflicts_.

Ooh, good thought.  I like that.

>
> > +     test_must_fail git merge-tree --real side1 side2 >RESULT &&
> > +     R_TREE=$(cat RESULT) &&
>
> How about `actual_tree` instead?

But my 80-characters rev-parse lines....waaah.  Just kidding, yeah
this would be better.

> > +
> > +     # Due to differences of e.g. "HEAD" vs "side1", the results will not
> > +     # exactly match.  Dig into individual files.
> > +
> > +     # Numbers should have three-way merged cleanly
> > +     test_write_lines 0 1 2 3 4 5 6 >expect &&
> > +     git show ${R_TREE}:numbers >actual &&
> > +     test_cmp expect actual &&
> > +
> > +     # whatever and whatever~<branch> should have same HASHES
> > +     git rev-parse ${E_TREE}:whatever ${E_TREE}:whatever~HEAD >expect &&
> > +     git rev-parse ${R_TREE}:whatever ${R_TREE}:whatever~side1 >actual &&
> > +     test_cmp expect actual &&
> > +
> > +     # greeting should have a merge conflict
> > +     git show ${E_TREE}:greeting >tmp &&
> > +     cat tmp | sed -e s/HEAD/side1/ >expect &&
> > +     git show ${R_TREE}:greeting >actual &&
> > +     test_cmp expect actual
> > +'
> > +
> > +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).  That
said, I did a bad job mentioning that in the test description; I'll
fix it up.

> > +
> > +test_expect_success 'Barf on too many arguments' '
> > +     test_expect_code 129 git merge-tree --real side1 side2 side3 2>expect &&
> > +
> > +     grep "^usage: git merge-tree" expect
> > +'
> > +
> > +test_done
>
> The rest looks awesome. Thank you for working on it! I will definitely
> come back to review the rest (have to take a break now), and then probably
> add quite a bit of food for thought based on my experience _actually_
> using `merge-ort` on the server-side. Stay tuned.

Ooh, I'm intrigued.  And thanks for reviewing!