On Wed, Jan 26, 2022 at 1:45 AM Christian Couder
[off-list ref] wrote:

On Sat, Jan 22, 2022 at 10:56 PM Elijah Newren via GitGitGadget
[off-list ref] wrote:

quoted

From: Elijah Newren <redacted>

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.

quoted

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

quoted

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.

quoted

 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.

quoted

+       parent2 = get_merge_parent(branch2);
+       if (!parent2)
+               help_unknown_ref(branch2, "merge",
+                                _("not something we can merge"));

idem

quoted

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

quoted

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

quoted

+       if (result.clean < 0)
+               die(_("failure to merge"));
+       else if (!result.clean)

The "else" is not necessary above.

quoted

+               printf(_("Conflicts!\n"));

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

quoted

+       merge_finalize(&opt, &result);
+       return !result.clean; /* result.clean < 0 handled above */
 }

quoted

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

quoted

@@ -0,0 +1,87 @@
+#!/bin/sh
+
+test_description='git merge-tree --write-tree'

... especially given this description.

Makes sense; will rename.