Re: [PATCH 04/12] merge-tree: implement real merges

From: Elijah Newren <newren@gmail.com>
Date: Jan 29, 2022, 04:09 UTC
Message-ID: <CABPp-BHrjpEr=Pmea1Em+cntNavd_juO1+jV-OYCEs7G1qDGSw@mail.gmail.com>
In-Reply-To: <CAP8UFD3_Dx2aZObbUp7dHkFCp5w9mJG-s03Soz=9YDU=yE2NoQ@mail.gmail.com>

On Wed, Jan 26, 2022 at 1:45 AM Christian Couder <christian.couder@gmail.com> wrote:

Show 18 quoted lines

>
> On Sat, Jan 22, 2022 at 10:56 PM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> 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)
>
> The exit status can now actually be something other than 0 and 1
> according to the doc and code below.

Thanks for catching; will fix.

Show 11 quoted lines

> > +Performs a merge, but does not make any new commits and does not read
> > +from or write to either the working tree or index.
> > +
> > +The first form will merge the two branches, doing a full recursive
> > +merge with rename detection.
>
> Maybe this could already tell that the first form will also write a
> tree with the result of the merge (even in case of conflict) as this
> could help understand the reason why the associated option is called
> '--write-tree'. It could also help to say that we call such a merge a
> 'real' merge.

Makes sense.

Show 22 quoted lines

> > The rest of this manual (other than the
> > +next paragraph) describes the first form in more detail -- including
> > +options, output format, exit status, and usage notes.
>
> >  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"));
>
> The second argument is supposed to be the command (it's called "cmd"),
> so maybe "merge-tree" instead of "merge".

Oh, good catch; thanks for pointing this out.

Show 20 quoted lines

>
> > +       parent2 = get_merge_parent(branch2);
> > +       if (!parent2)
> > +               help_unknown_ref(branch2, "merge",
> > +                                _("not something we can merge"));
>
> idem
>
> > +       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? */
>
> I think just:
>
>        opt.branch1 = branch1
>        opt.branch2 = branch2
>
> might be better for users as it should show the name as it was passed
> to the command.

After digging for a bit, I think in this case there actually isn't a difference to users because both will give the same result. But, if that's the case, the simpler code is warranted.

Show 6 quoted lines

> > +       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> > +       printf("%s\n", oid_to_hex(&result.tree->object.oid));
>
> I wonder if we can actually always output a valid tree when
> result.clean < 0. In case we might not, the printing should go a few
> lines below.

Yeah, I caught that and fixed it, but got it squashed into a later commit. I'll fix it up.

Show 7 quoted lines

> > +       if (result.clean < 0)
> > +               die(_("failure to merge"));
> > +       else if (!result.clean)
>
> The "else" is not necessary above.
>
> > +               printf(_("Conflicts!\n"));

Yes, and the else clause should just be ripped out.

Show 18 quoted lines

> > +       merge_finalize(&opt, &result);
> > +       return !result.clean; /* result.clean < 0 handled above */
> >  }
>
> > diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> > new file mode 100755
> > index 00000000000..e03688515c5
> > --- /dev/null
> > +++ b/t/t4301-merge-tree-real.sh
>
> I wonder if it would be better named 't4301-merge-tree-write-tree.sh'...
>
> > @@ -0,0 +1,87 @@
> > +#!/bin/sh
> > +
> > +test_description='git merge-tree --write-tree'
>
> ... especially given this description.

Makes sense; will rename.