On Wed, Jan 26, 2022 at 1:45 AM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
>
> On Sat, Jan 22, 2022 at 10:56 PM Elijah Newren via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
> >
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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.
> > +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.
> > 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.
>
> > + 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.
> > + 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.
> > + 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.
> > + 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.
