RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index)

From: Elijah Newren <[email protected]>

In preparation for adding a non-trivial merge capability to merge-tree, move the existing merge logic for trivial merges into a new function.

Signed-off-by Elijah Newren <[email protected]>

---
 builtin/merge-tree.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

builtin/merge-tree.c +0 -0

index 06f9eee9f78..9fe5b99f623 100644

Quoted 18 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

In preparation for adding a non-trivial merge capability to merge-tree, move the existing merge logic for trivial merges into a new function.

Signed-off-by Elijah Newren <[email protected]>

---
 builtin/merge-tree.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

builtin/merge-tree.c +0 -0

index 06f9eee9f78..9fe5b99f623 100644
@@ -366,15 +366,11 @@ static void *get_tree_descriptor(struct repository *r,
      return buf;
 }
 

builtin/merge-tree.c static void *get_tree_descriptor(struct repository *r, · line 369

Quoted 3 lines

builtin/merge-tree.c +1 -2

-int cmd_merge_tree(int argc, const char **argv, const char *prefix)
-{
+static int trivial_merge(int argc, const char **argv) {

I guess the brace should probably stay on its own line

From: Elijah Newren <[email protected]>

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 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 $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 <[email protected]>

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

Documentation/git-merge-tree.txt +0 -0

index 58731c19422..5823938937f 100644
@@ -3,26 +3,34 @@ git-merge-tree(1)
 
 NAME

Quoted 46 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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 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 $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 <[email protected]>

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

Documentation/git-merge-tree.txt +1 -1

index 58731c19422..5823938937f 100644
@@ -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
 
 

Documentation/git-merge-tree.txt git-merge-tree(1) · line 13

Quoted 5 lines

Documentation/git-merge-tree.txt +1 -0

 SYNOPSIS
 --------
 [verse]
+'git merge-tree' --real <branch1> <branch2>
 'git merge-tree' <base-tree> <branch1> <branch2>

This is really exciting. It could replace the merge-machinery of git-revise (which is a "fast rebase" tool).
I think for cherry-pick/rebase we need to specify a custom merge base, would that suit the new form?

From: Elijah Newren <[email protected]>

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 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 $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 <[email protected]>

---

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

Documentation/git-merge-tree.txt +0 -0

index 58731c19422..5823938937f 100644
@@ -3,26 +3,34 @@ git-merge-tree(1)
 

NAME
----

Documentation/git-merge-tree.txt +1 -1

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

DESCRIPTION
-----------

Documentation/git-merge-tree.txt +16 -9

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

builtin/merge-tree.c +54 -1

index f04b1eaad0a..c5757bed5bb 100644
@@ -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"

@@ -391,7 +394,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?
+	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);
+	return result.clean ? 0 : 1;
 }
 
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)

t/t4301-merge-tree-real.sh new file +16 -0

new file mode 100755
index 00000000000..9fb617ccc7f
@@ -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
+
+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 &&

t/t4301-merge-tree-real.sh new file +46 -0

+'
+
+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) &&
+
+	git reset --hard &&
+	test_must_fail git merge-tree --real side1 side2 >RESULT &&
+	R_TREE=$(cat RESULT) &&
+
+	# 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
+'
+
+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

Signature · 1 line

gitgitgadget

Quoted 8 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

Quoted 10 lines

t/t4301-merge-tree-real.sh new file +10 -0

+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 &&
+
+      git branch side1 &&
+      git branch side2 &&
+

t/t4301-merge-tree-real.sh line 26

Quoted 6 lines

t/t4301-merge-tree-real.sh new file +6 -0

+      git checkout side1 &&
+      test_write_lines 1 2 3 4 5 6 >numbers &&
+      echo hi >greeting &&
+      echo bar >whatever &&
+      git add numbers greeting whatever &&
+      git commit -m rename-and-modify &&

The commit implies a rename as well which I think is missing.

t/t4301-merge-tree-real.sh line 35

Quoted 9 lines

t/t4301-merge-tree-real.sh new file +9 -0

+
+      git checkout side2 &&
+      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 remove-and-rename

And this looks more like a remove-and-modify.

From: Elijah Newren <[email protected]>

No functional changes included in this patch; it's just a preparatory step in anticipation of wanting to handle the printed messages differently in git merge-tree --real.

Signed-off-by Elijah Newren <[email protected]>

---
merge-ort.c | 69 ++++++++++++++++++++++++++++------------------------- merge-ort.h | 8 +++++++

2 files changed, 44 insertions(+), 33 deletions(-)

merge-ort.c +36 -33

index 0342f104836..6237e2fb7fe 100644
@@ -4197,6 +4197,42 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 	return errs;
 }
 
+void merge_display_update_messages(struct merge_options *opt,
+				   struct merge_result *result)
+{
+	struct merge_options_internal *opti = result->priv;
+	struct hashmap_iter iter;
+	struct strmap_entry *e;
+	struct string_list olist = STRING_LIST_INIT_NODUP;
+	int i;
+
+	trace2_region_enter("merge", "display messages", opt->repo);
+
+	/* Hack to pre-allocate olist to the desired size */
+	ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
+		   olist.alloc);
+
+	/* Put every entry from output into olist, then sort */
+	strmap_for_each_entry(&opti->output, &iter, e) {
+		string_list_append(&olist, e->key)->util = e->value;
+	}
+	string_list_sort(&olist);
+
+	/* Iterate over the items, printing them */
+	for (i = 0; i < olist.nr; ++i) {
+		struct strbuf *sb = olist.items[i].util;
+
+		printf("%s", sb->buf);
+	}
+	string_list_clear(&olist, 0);
+
+	/* Also include needed rename limit adjustment now */
+	diff_warn_rename_limit("merge.renamelimit",
+			       opti->renames.needed_limit, 0);
+
+	trace2_region_leave("merge", "display messages", opt->repo);
+}
+
 void merge_switch_to_result(struct merge_options *opt,
 			    struct tree *head,
 			    struct merge_result *result,

@@ -4235,39 +4271,6 @@ void merge_switch_to_result(struct merge_options *opt,
 		trace2_region_leave("merge", "write_auto_merge", opt->repo);
 	}
 
-	if (display_update_msgs) {
-		struct merge_options_internal *opti = result->priv;
-		struct hashmap_iter iter;
-		struct strmap_entry *e;
-		struct string_list olist = STRING_LIST_INIT_NODUP;
-		int i;
-
-		trace2_region_enter("merge", "display messages", opt->repo);
-
-		/* Hack to pre-allocate olist to the desired size */
-		ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
-			   olist.alloc);
-
-		/* Put every entry from output into olist, then sort */
-		strmap_for_each_entry(&opti->output, &iter, e) {
-			string_list_append(&olist, e->key)->util = e->value;
-		}
-		string_list_sort(&olist);
-
-		/* Iterate over the items, printing them */
-		for (i = 0; i < olist.nr; ++i) {
-			struct strbuf *sb = olist.items[i].util;
-
-			printf("%s", sb->buf);
-		}
-		string_list_clear(&olist, 0);
-
-		/* Also include needed rename limit adjustment now */
-		diff_warn_rename_limit("merge.renamelimit",
-				       opti->renames.needed_limit, 0);
-
-		trace2_region_leave("merge", "display messages", opt->repo);
-	}
 
 	merge_finalize(opt, result);
 }

merge-ort.h +8 -0

index c011864ffeb..1b93555a60b 100644
@@ -70,6 +70,14 @@ void merge_switch_to_result(struct merge_options *opt,
 			    int update_worktree_and_index,
 			    int display_update_msgs);
 
+/*
+ * Display messages about conflicts and which files were 3-way merged.
+ * Automatically called by merge_switch_to_result() with stream == stdout,
+ * so only call this when bypassing merge_switch_to_result().
+ */
+void merge_display_update_messages(struct merge_options *opt,
+				   struct merge_result *result);
+
 /* Do needed cleanup when not calling merge_switch_to_result() */
 void merge_finalize(struct merge_options *opt,
 		    struct merge_result *result);

Signature · 1 line

gitgitgadget

Quoted 5 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

No functional changes included in this patch; it's just a preparatory step in anticipation of wanting to handle the printed messages differently in git merge-tree --real.

Not quite. You are missing:

(quoted diff) +2 -0

+       if (display_update_msgs)
+               merge_display_update_messages(opt, result, stdout);

in merge_switch_to_result(), which you added in the next commit. Not really relevant for the series as a whole but this commit alone breaks some functionality.

Quoted 117 lines

Signed-off-by Elijah Newren <[email protected]>

---
merge-ort.c | 69 ++++++++++++++++++++++++++++------------------------- merge-ort.h | 8 +++++++

2 files changed, 44 insertions(+), 33 deletions(-)

merge-ort.c +36 -33

index 0342f104836..6237e2fb7fe 100644
@@ -4197,6 +4197,42 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 	return errs;
 }
 
+void merge_display_update_messages(struct merge_options *opt,
+				   struct merge_result *result)
+{
+	struct merge_options_internal *opti = result->priv;
+	struct hashmap_iter iter;
+	struct strmap_entry *e;
+	struct string_list olist = STRING_LIST_INIT_NODUP;
+	int i;
+
+	trace2_region_enter("merge", "display messages", opt->repo);
+
+	/* Hack to pre-allocate olist to the desired size */
+	ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
+		   olist.alloc);
+
+	/* Put every entry from output into olist, then sort */
+	strmap_for_each_entry(&opti->output, &iter, e) {
+		string_list_append(&olist, e->key)->util = e->value;
+	}
+	string_list_sort(&olist);
+
+	/* Iterate over the items, printing them */
+	for (i = 0; i < olist.nr; ++i) {
+		struct strbuf *sb = olist.items[i].util;
+
+		printf("%s", sb->buf);
+	}
+	string_list_clear(&olist, 0);
+
+	/* Also include needed rename limit adjustment now */
+	diff_warn_rename_limit("merge.renamelimit",
+			       opti->renames.needed_limit, 0);
+
+	trace2_region_leave("merge", "display messages", opt->repo);
+}
+
 void merge_switch_to_result(struct merge_options *opt,
 			    struct tree *head,
 			    struct merge_result *result,

@@ -4235,39 +4271,6 @@ void merge_switch_to_result(struct merge_options *opt,
 		trace2_region_leave("merge", "write_auto_merge", opt->repo);
 	}
 
-	if (display_update_msgs) {
-		struct merge_options_internal *opti = result->priv;
-		struct hashmap_iter iter;
-		struct strmap_entry *e;
-		struct string_list olist = STRING_LIST_INIT_NODUP;
-		int i;
-
-		trace2_region_enter("merge", "display messages", opt->repo);
-
-		/* Hack to pre-allocate olist to the desired size */
-		ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
-			   olist.alloc);
-
-		/* Put every entry from output into olist, then sort */
-		strmap_for_each_entry(&opti->output, &iter, e) {
-			string_list_append(&olist, e->key)->util = e->value;
-		}
-		string_list_sort(&olist);
-
-		/* Iterate over the items, printing them */
-		for (i = 0; i < olist.nr; ++i) {
-			struct strbuf *sb = olist.items[i].util;
-
-			printf("%s", sb->buf);
-		}
-		string_list_clear(&olist, 0);
-
-		/* Also include needed rename limit adjustment now */
-		diff_warn_rename_limit("merge.renamelimit",
-				       opti->renames.needed_limit, 0);
-
-		trace2_region_leave("merge", "display messages", opt->repo);
-	}
 
 	merge_finalize(opt, result);
 }

merge-ort.h +8 -0

index c011864ffeb..1b93555a60b 100644
@@ -70,6 +70,14 @@ void merge_switch_to_result(struct merge_options *opt,
 			    int update_worktree_and_index,
 			    int display_update_msgs);
 
+/*
+ * Display messages about conflicts and which files were 3-way merged.
+ * Automatically called by merge_switch_to_result() with stream == stdout,
+ * so only call this when bypassing merge_switch_to_result().
+ */
+void merge_display_update_messages(struct merge_options *opt,
+				   struct merge_result *result);
+
 /* Do needed cleanup when not calling merge_switch_to_result() */
 void merge_finalize(struct merge_options *opt,
 		    struct merge_result *result);

Signature · 1 line

gitgitgadget

From: Elijah Newren <[email protected]>

This modifies the new display_update_messages() function to allow printing to somewhere other than stdout.

Signed-off-by Elijah Newren <[email protected]>

---
 merge-ort.c | 7 +++++--
 merge-ort.h | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

merge-ort.c +0 -0

index 6237e2fb7fe..86eebf39166 100644
[...]

Quoted 20 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

This modifies the new display_update_messages() function to allow printing to somewhere other than stdout.

Signed-off-by Elijah Newren <[email protected]>

---
 merge-ort.c | 7 +++++--
 merge-ort.h | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

merge-ort.c +0 -0

index 6237e2fb7fe..86eebf39166 100644
[...]

@@ -4271,6 +4272,8 @@ void merge_switch_to_result(struct merge_options *opt,
              trace2_region_leave("merge", "write_auto_merge", opt->repo);
      }
 

merge-ort.c void merge_switch_to_result(struct merge_options *opt, · line 4276

Quoted 2 lines

merge-ort.c +2 -0

+     if (display_update_msgs)
+             merge_display_update_messages(opt, result, stdout);

is it intentional that the previous patch doesn't have this call?

From: Elijah Newren <[email protected]>

When running git merge-tree --real, we previously would only return an exit status reflecting the cleanness of a merge, and print out the toplevel tree of the resulting merge. Merges also have informational messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT (file/directory)", etc.) In fact, when non-content conflicts occur (such as file/directory, modify/delete, add/add with differing modes, rename/rename (1to2), etc.), these informational messages are often the only notification since these conflicts are not representable in the contents of the file.

Add a --messages option which names a file so that callers can request these messages be recorded somewhere.

Signed-off-by Elijah Newren <[email protected]>

---

Documentation/git-merge-tree.txt |  6 ++++--
builtin/merge-tree.c             | 18 ++++++++++++++++--
t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
3 files changed, 38 insertions(+), 4 deletions(-)

Documentation/git-merge-tree.txt +0 -0

index 5823938937f..4d5857b390b 100644
@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree

SYNOPSIS
--------
[verse]
-'git merge-tree' --real <branch1> <branch2>
+'git merge-tree' --real [--messages=<file>] <branch1> <branch2> 'git merge-tree' <base-tree> <branch1> <branch2>

DESCRIPTION

Documentation/git-merge-tree.txt +0 -0

@@ -21,7 +21,9 @@ 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

Documentation/git-merge-tree.txt +3 -1

-(which may have files including conflict markers).
+(which may have files including conflict markers).  With `--messages`,
+it will write any informational messages (such as "Auto-merging
+<path>" and conflict notices) to the given file.

The second form is meant for backward compatibility and will only do a trival merge. It reads three tree-ish, and outputs trivial merge

builtin/merge-tree.c +1 -0

index c5757bed5bb..47deef0b199 100644
@@ -389,6 +389,7 @@ static int trivial_merge(const char *base,
 
 struct merge_tree_options {
 	int real;
+	char *messages_file;
 };
 
 static int real_merge(struct merge_tree_options *o,

builtin/merge-tree.c +10 -2

+	}
 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
-	merge_switch_to_result(&opt, NULL, &result, 0, 0);
+
+	merge_finalize(&opt, &result);
 	return result.clean ? 0 : 1;
 }
 

@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
 	struct merge_tree_options o = { 0 };
 	int expected_remaining_argc;
+	int original_argc;
 
 	const char * const merge_tree_usage[] = {
-		N_("git merge-tree --real <branch1> <branch2>"),
+		N_("git merge-tree --real [<options>] <branch1> <branch2>"),
 		N_("git merge-tree <base-tree> <branch1> <branch2>"),
 		NULL
 	};
 	struct option mt_options[] = {
 		OPT_BOOL(0, "real", &o.real,
 			 N_("do a real merge instead of a trivial merge")),
+		OPT_STRING(0, "messages", &o.messages_file, N_("file"),
+			   N_("filename to write informational/conflict messages to")),
 		OPT_END()
 	};
 

@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 		usage_with_options(merge_tree_usage, mt_options);
 
 	/* Parse arguments */
+	original_argc = argc;
 	argc = parse_options(argc, argv, prefix, mt_options,
 			     merge_tree_usage, 0);
+	if (!o.real && original_argc < argc)
+		die(_("--real must be specified if any other options are"));
 	expected_remaining_argc = (o.real ? 2 : 3);
 	if (argc != expected_remaining_argc)
 		usage_with_options(merge_tree_usage, mt_options);

t/t4301-merge-tree-real.sh +0 -0

index 9fb617ccc7f..42218cdc019 100755
@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
 	grep "^usage: git merge-tree" expect

'

t/t4301-merge-tree-real.sh +18 -0

+test_expect_success '--messages gives us the conflict notices and such' '
+	test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&
+
+	# Expected results:
+	#   "greeting" should merge with conflicts
+	#   "numbers" should merge cleanly
+	#   "whatever" has *both* a modify/delete and a file/directory conflict
+	cat <<-EOF >expect &&
+	Auto-merging greeting
+	CONFLICT (content): Merge conflict in greeting
+	Auto-merging numbers
+	CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead.
+	CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1.  Version side1 of whatever~side1 left in tree.
+	EOF
+
+	test_cmp expect MSG_FILE
+'
+

test_done

Signature · 1 line

gitgitgadget

Quoted 61 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

When running git merge-tree --real, we previously would only return an exit status reflecting the cleanness of a merge, and print out the toplevel tree of the resulting merge. Merges also have informational messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT (file/directory)", etc.) In fact, when non-content conflicts occur (such as file/directory, modify/delete, add/add with differing modes, rename/rename (1to2), etc.), these informational messages are often the only notification since these conflicts are not representable in the contents of the file.

Add a --messages option which names a file so that callers can request these messages be recorded somewhere.

Signed-off-by Elijah Newren <[email protected]>

---

Documentation/git-merge-tree.txt |  6 ++++--
builtin/merge-tree.c             | 18 ++++++++++++++++--
t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
3 files changed, 38 insertions(+), 4 deletions(-)

Documentation/git-merge-tree.txt +0 -0

index 5823938937f..4d5857b390b 100644
@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree

SYNOPSIS
--------
[verse]
-'git merge-tree' --real <branch1> <branch2>
+'git merge-tree' --real [--messages=<file>] <branch1> <branch2> 'git merge-tree' <base-tree> <branch1> <branch2>

DESCRIPTION

Documentation/git-merge-tree.txt +0 -0

@@ -21,7 +21,9 @@ 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

Documentation/git-merge-tree.txt +3 -1

-(which may have files including conflict markers).
+(which may have files including conflict markers).  With `--messages`,
+it will write any informational messages (such as "Auto-merging
+<path>" and conflict notices) to the given file.

The second form is meant for backward compatibility and will only do a trival merge. It reads three tree-ish, and outputs trivial merge

builtin/merge-tree.c +1 -0

index c5757bed5bb..47deef0b199 100644
@@ -389,6 +389,7 @@ static int trivial_merge(const char *base,
 
 struct merge_tree_options {
      int real;
+      char *messages_file;
 };
 
 static int real_merge(struct merge_tree_options *o,

@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
       */
 

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 450

Quoted 6 lines

builtin/merge-tree.c +5 -0

      merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+
+      if (o->messages_file) {
+              FILE *fp = xfopen(o->messages_file, "w");
+              merge_display_update_messages(&opt, &result, fp);
+              fclose(fp);

I don't know enough about how merge-ort works internally, but it looks to me like at this point the merge already happened and we just didn't clean up (finalize) yet. It feels wrong to die() at this point just because we can't open messages_file.

Quoted 9 lines

builtin/merge-tree.c +3 -1

+      }
      printf("%s\n", oid_to_hex(&result.tree->object.oid));
-      merge_switch_to_result(&opt, NULL, &result, 0, 0);
+
+      merge_finalize(&opt, &result);
      return result.clean ? 0 : 1;
 }
 

Quoted 3 lines · Fabian Stelzer

Quoted 1 line · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Quoted 5 lines

Quoted 5 lines

static int real_merge(struct merge_tree_options *o,

(quoted diff) +0 -0

@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
       */
 

Quoted 11 lines

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 450

Quoted 6 lines

builtin/merge-tree.c +5 -0

      merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+
+      if (o->messages_file) {
+              FILE *fp = xfopen(o->messages_file, "w");
+              merge_display_update_messages(&opt, &result, fp);
+              fclose(fp);

I don't know enough about how merge-ort works internally, but it looks to me like at this point the merge already happened and we just didn't clean up (finalize) yet. It feels wrong to die() at this point just because we can't open messages_file.

Yes, the merge already happened; there now exists a new toplevel tree (that nothing references). I'm not sure I understand what's wrong with die'ing here, though. I can't tell if you want to defer the die-ing until later, or just avoid the die-ing and return some kind of success despite failing to complete what the user requested.

Quoted 4 lines · Elijah Newren

Quoted 3 lines · Fabian Stelzer

Quoted 1 line · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

[...]

Quoted 17 lines

Quoted 5 lines

Quoted 5 lines

static int real_merge(struct merge_tree_options *o,

(quoted diff) +0 -0

@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
       */
 

Quoted 11 lines

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 450

Quoted 6 lines

builtin/merge-tree.c +5 -0

      merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+
+      if (o->messages_file) {
+              FILE *fp = xfopen(o->messages_file, "w");
+              merge_display_update_messages(&opt, &result, fp);
+              fclose(fp);

I don't know enough about how merge-ort works internally, but it looks to me like at this point the merge already happened and we just didn't clean up (finalize) yet. It feels wrong to die() at this point just because we can't open messages_file.

Quoted 6 lines

Yes, the merge already happened; there now exists a new toplevel tree (that nothing references). I'm not sure I understand what's wrong with die'ing here, though. I can't tell if you want to defer the die-ing until later, or just avoid the die-ing and return some kind of success despite failing to complete what the user requested.

I think i would prefer the merge operation to abort before actually merging when not being able to write its logfile. Otherwise we possibly do a whole lot of work thats inaccessible afterwards isn't it? (since we dont print the hash)

Thanks for your work on this feature. I think this could open a lot of new possibilities.

Quoted 31 lines · Fabian Stelzer

Quoted 4 lines · Elijah Newren

Quoted 3 lines · Fabian Stelzer

Quoted 1 line · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

[...]

Quoted 17 lines

Quoted 5 lines

Quoted 5 lines

static int real_merge(struct merge_tree_options *o,

(quoted diff) +0 -0

@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
       */
 

Quoted 11 lines

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 450

Quoted 6 lines

builtin/merge-tree.c +5 -0

      merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+
+      if (o->messages_file) {
+              FILE *fp = xfopen(o->messages_file, "w");
+              merge_display_update_messages(&opt, &result, fp);
+              fclose(fp);

I don't know enough about how merge-ort works internally, but it looks to me like at this point the merge already happened and we just didn't clean up (finalize) yet. It feels wrong to die() at this point just because we can't open messages_file.

Quoted 6 lines

Yes, the merge already happened; there now exists a new toplevel tree (that nothing references). I'm not sure I understand what's wrong with die'ing here, though. I can't tell if you want to defer the die-ing until later, or just avoid the die-ing and return some kind of success despite failing to complete what the user requested.

Quoted 4 lines

I think i would prefer the merge operation to abort before actually merging when not being able to write its logfile. Otherwise we possibly do a whole lot of work thats inaccessible afterwards isn't it? (since we dont print the hash)

I see where you're coming from, but I don't see this as worth worrying about. For two reasons:

(1) I'm not sure I buy the "whole lot of work" concern.

merge-ort is pretty snappy. For a simple example of rebasing a single
patch in linux.git across a branch with 28000 renames, I get 176
milliseconds for merge_incore_nonrecursive(). Granted, linux.git is
pretty small in terms of number of files, but Stolee did some
measurements a while back on the Microsoft repos with millions of
files at HEAD. For those, for a trivial merge he saw
merge_incore_recursive() complete in 2 milliseconds, and for a trivial
rebase he saw merge_incore_nonrecursive() complete in 4 milliseconds
(See https://lore.kernel.org/git/CABPp-BHO7bZ3H7A=E9TudhvBoNfwPvRiDMm8S9kq3mYeSXrpXw@mail.gmail.com/).

So huge numbers of files pose much less of a problem than lots of interesting work like renames, and merge-ort is pretty fast in either case. Sure, if we were talking about traditional merge-recursive which would have taken 150000 milliseconds on the same single patch in linux.git testcase (due to the 28000 renames), then we might worry more about not letting work get tossed, but at only 176 milliseconds even with a crazy number of renames, it's just not worth worrying about.

(2) Even if there is a lot of computation, I don't see why this error path merits extra coding work to salvage the computation somehow

By way of comparison, a regular git merge will abort after completing the same amount of merge work (i.e. after creating a new tree) when the user has a dirty working tree involving a path that would need to be updated by the merge operation. And that is not a bug; it's a requirement -- we cannot first check if the user has dirtied such a path before performing the merge because it's impossible to do so accurately in the face of renames. merge-recursive tried to do that and had early aborts that fell in the false-positive category and some that fell in the false-negative category. It was impossible to fix the false-positives and false-negatives without either (a) disallowing ever doing a merge with a dirty working tree under any conditions (a backwards compatibility break), or (b) waiting to do the notification of dirty-files-in-the-way until after the merge tree has been computed. I wasn't about to break that feature, so merge-ort had to delay error notifications instead.

Now, the dirty-file-in-the-way condition is for a very common case (either for users who intentionally like keeping dirty changes around and doing merges but the branch they are merging happens to touch a file they didn't know about, or users who just forgot that they had local modifications). In contrast, this case here is for when we cannot open a file for writing -- with the filename explicitly just specified by the user.

So, I'd rather keep the code nice and simple as it currently stands.

Quoted 3 lines

Thanks for your work on this feature. I think this could open a lot of new possibilities.

I hope people do interesting things with it, and with the server-side commit replaying I'm working on as well.

From: Elijah Newren <[email protected]>

When running git merge-tree --real, we previously would only return an exit status reflecting the cleanness of a merge, and print out the toplevel tree of the resulting merge. Merges also have informational messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT (file/directory)", etc.) In fact, when non-content conflicts occur (such as file/directory, modify/delete, add/add with differing modes, rename/rename (1to2), etc.), these informational messages are often the only notification since these conflicts are not representable in the contents of the file.

Add a --messages option which names a file so that callers can request these messages be recorded somewhere.

Signed-off-by Elijah Newren <[email protected]>

---

Documentation/git-merge-tree.txt |  6 ++++--
builtin/merge-tree.c             | 18 ++++++++++++++++--
t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
3 files changed, 38 insertions(+), 4 deletions(-)

Documentation/git-merge-tree.txt +0 -0

index 5823938937f..4d5857b390b 100644
@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree

SYNOPSIS
--------
[verse]
-'git merge-tree' --real <branch1> <branch2>
+'git merge-tree' --real [--messages=<file>] <branch1> <branch2> 'git merge-tree' <base-tree> <branch1> <branch2>

DESCRIPTION

Documentation/git-merge-tree.txt +0 -0

@@ -21,7 +21,9 @@ 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

Documentation/git-merge-tree.txt +3 -1

-(which may have files including conflict markers).
+(which may have files including conflict markers).  With `--messages`,
+it will write any informational messages (such as "Auto-merging
+<path>" and conflict notices) to the given file.

The second form is meant for backward compatibility and will only do a trival merge. It reads three tree-ish, and outputs trivial merge

builtin/merge-tree.c +1 -0

index c5757bed5bb..47deef0b199 100644
@@ -389,6 +389,7 @@ static int trivial_merge(const char *base,
 
 struct merge_tree_options {
 	int real;
+	char *messages_file;
 };
 
 static int real_merge(struct merge_tree_options *o,

builtin/merge-tree.c +9 -2

 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
-	merge_switch_to_result(&opt, NULL, &result, 0, 0);
+
+	merge_finalize(&opt, &result);
 	return result.clean ? 0 : 1;
 }
 

@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
 	struct merge_tree_options o = { 0 };
 	int expected_remaining_argc;
+	int original_argc;
 
 	const char * const merge_tree_usage[] = {
-		N_("git merge-tree --real <branch1> <branch2>"),
+		N_("git merge-tree --real [<options>] <branch1> <branch2>"),
 		N_("git merge-tree <base-tree> <branch1> <branch2>"),
 		NULL
 	};
 	struct option mt_options[] = {
 		OPT_BOOL(0, "real", &o.real,
 			 N_("do a real merge instead of a trivial merge")),
+		OPT_STRING(0, "messages", &o.messages_file, N_("file"),
+			   N_("filename to write informational/conflict messages to")),
 		OPT_END()
 	};
 

@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 		usage_with_options(merge_tree_usage, mt_options);
 
 	/* Parse arguments */
+	original_argc = argc;
 	argc = parse_options(argc, argv, prefix, mt_options,
 			     merge_tree_usage, 0);
+	if (!o.real && original_argc < argc)
+		die(_("--real must be specified if any other options are"));
 	expected_remaining_argc = (o.real ? 2 : 3);
 	if (argc != expected_remaining_argc)
 		usage_with_options(merge_tree_usage, mt_options);

t/t4301-merge-tree-real.sh +0 -0

index 9fb617ccc7f..42218cdc019 100755
@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
 	grep "^usage: git merge-tree" expect

'

t/t4301-merge-tree-real.sh +18 -0

+test_expect_success '--messages gives us the conflict notices and such' '
+	test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&
+
+	# Expected results:
+	#   "greeting" should merge with conflicts
+	#   "numbers" should merge cleanly
+	#   "whatever" has *both* a modify/delete and a file/directory conflict
+	cat <<-EOF >expect &&
+	Auto-merging greeting
+	CONFLICT (content): Merge conflict in greeting
+	Auto-merging numbers
+	CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead.
+	CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1.  Version side1 of whatever~side1 left in tree.
+	EOF
+
+	test_cmp expect MSG_FILE
+'
+

test_done

Signature · 1 line

gitgitgadget

Quoted 61 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

When running git merge-tree --real, we previously would only return an exit status reflecting the cleanness of a merge, and print out the toplevel tree of the resulting merge. Merges also have informational messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT (file/directory)", etc.) In fact, when non-content conflicts occur (such as file/directory, modify/delete, add/add with differing modes, rename/rename (1to2), etc.), these informational messages are often the only notification since these conflicts are not representable in the contents of the file.

Add a --messages option which names a file so that callers can request these messages be recorded somewhere.

Signed-off-by Elijah Newren <[email protected]>

---

Documentation/git-merge-tree.txt |  6 ++++--
builtin/merge-tree.c             | 18 ++++++++++++++++--
t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
3 files changed, 38 insertions(+), 4 deletions(-)

Documentation/git-merge-tree.txt +0 -0

index 5823938937f..4d5857b390b 100644
@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree

SYNOPSIS
--------
[verse]
-'git merge-tree' --real <branch1> <branch2>
+'git merge-tree' --real [--messages=<file>] <branch1> <branch2> 'git merge-tree' <base-tree> <branch1> <branch2>

DESCRIPTION

Documentation/git-merge-tree.txt +0 -0

@@ -21,7 +21,9 @@ 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

Documentation/git-merge-tree.txt +3 -1

-(which may have files including conflict markers).
+(which may have files including conflict markers).  With `--messages`,
+it will write any informational messages (such as "Auto-merging
+<path>" and conflict notices) to the given file.

The second form is meant for backward compatibility and will only do a trival merge. It reads three tree-ish, and outputs trivial merge

builtin/merge-tree.c +1 -0

index c5757bed5bb..47deef0b199 100644
@@ -389,6 +389,7 @@ static int trivial_merge(const char *base,
 
 struct merge_tree_options {
      int real;
+      char *messages_file;
 };
 
 static int real_merge(struct merge_tree_options *o,

@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
       */
 

builtin/merge-tree.c static int real_merge(struct merge_tree_options *o, · line 451

Quoted 7 lines

builtin/merge-tree.c +6 -0

      merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+
+      if (o->messages_file) {
+              FILE *fp = xfopen(o->messages_file, "w");
+              merge_display_update_messages(&opt, &result, fp);
+              fclose(fp);
+      }

Something else I just wondered. Can the user differentiate between the die() in xfopen() and a failed/unclean merge?
Both just exit(1) don't they?

Quoted 70 lines

builtin/merge-tree.c +9 -2

      printf("%s\n", oid_to_hex(&result.tree->object.oid));
-      merge_switch_to_result(&opt, NULL, &result, 0, 0);
+
+      merge_finalize(&opt, &result);
      return result.clean ? 0 : 1;
 }
 

@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
      struct merge_tree_options o = { 0 };
      int expected_remaining_argc;
+      int original_argc;
 
      const char * const merge_tree_usage[] = {
-              N_("git merge-tree --real <branch1> <branch2>"),
+              N_("git merge-tree --real [<options>] <branch1> <branch2>"),
              N_("git merge-tree <base-tree> <branch1> <branch2>"),
              NULL
      };
      struct option mt_options[] = {
              OPT_BOOL(0, "real", &o.real,
                       N_("do a real merge instead of a trivial merge")),
+              OPT_STRING(0, "messages", &o.messages_file, N_("file"),
+                         N_("filename to write informational/conflict messages to")),
              OPT_END()
      };
 

@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
              usage_with_options(merge_tree_usage, mt_options);
 
      /* Parse arguments */
+      original_argc = argc;
      argc = parse_options(argc, argv, prefix, mt_options,
                           merge_tree_usage, 0);
+      if (!o.real && original_argc < argc)
+              die(_("--real must be specified if any other options are"));
      expected_remaining_argc = (o.real ? 2 : 3);
      if (argc != expected_remaining_argc)
              usage_with_options(merge_tree_usage, mt_options);

t/t4301-merge-tree-real.sh +0 -0

index 9fb617ccc7f..42218cdc019 100755
@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
      grep "^usage: git merge-tree" expect

'

t/t4301-merge-tree-real.sh +18 -0

+test_expect_success '--messages gives us the conflict notices and such' '
+      test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&
+
+      # Expected results:
+      #   "greeting" should merge with conflicts
+      #   "numbers" should merge cleanly
+      #   "whatever" has *both* a modify/delete and a file/directory conflict
+      cat <<-EOF >expect &&
+      Auto-merging greeting
+      CONFLICT (content): Merge conflict in greeting
+      Auto-merging numbers
+      CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead.
+      CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1.  Version side1 of whatever~side1 left in tree.
+      EOF
+
+      test_cmp expect MSG_FILE
+'
+

test_done
--
gitgitgadget

From: Elijah Newren <[email protected]>

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

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 <[email protected]>

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

Documentation/git-merge-tree.txt +0 -0

index 58731c19422..5823938937f 100644
@@ -3,26 +3,34 @@ git-merge-tree(1)
 
 NAME

Documentation/git-merge-tree.txt +16 -9

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

builtin/merge-tree.c +36 -1

index e1d2832c809..ac50f3d108b 100644
@@ -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? */
+

builtin/merge-tree.c +1 -0

+	return result.clean ? 0 : 1;
 }
 
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)

t/t4301-merge-tree-real.sh new file +0 -0

new file mode 100755
index 00000000000..f7aa310f8c1

t/t4301-merge-tree-real.sh new file +4 -0

+	test_write_lines 0 1 2 3 4 5 >numbers &&
+	echo yo >greeting &&
+	git rm whatever &&
+	mkdir whatever &&

t/t4301-merge-tree-real.sh new file +17 -0

+
+	# 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 &&

Hi Elijah,

Quoted 12 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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)

Quoted 1 line

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.

In my defense, I have more substantial suggestions below.

Quoted 33 lines

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 <[email protected]>

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

Documentation/git-merge-tree.txt +1 -1

index 58731c19422..5823938937f 100644
@@ -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
 
 

Documentation/git-merge-tree.txt git-merge-tree(1) · line 13

Quoted 5 lines

Documentation/git-merge-tree.txt +1 -0

 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?

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

Quoted 89 lines

Documentation/git-merge-tree.txt +16 -9

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

builtin/merge-tree.c +41 -1

index e1d2832c809..ac50f3d108b 100644
@@ -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);

builtin/merge-tree.c struct merge_tree_options { · line 440

Quoted 5 lines

builtin/merge-tree.c +5 -0

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

builtin/merge-tree.c struct merge_tree_options { · line 447

Quoted 7 lines

builtin/merge-tree.c +7 -0

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

Quoted 11 lines

builtin/merge-tree.c +1 -0

+     return result.clean ? 0 : 1;
 }
 
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)

t/t4301-merge-tree-real.sh new file +0 -0

new file mode 100755
index 00000000000..f7aa310f8c1

t/t4301-merge-tree-real.sh line 9

Quoted 9 lines

t/t4301-merge-tree-real.sh new file +9 -0

@@ -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
        }

t/t4301-merge-tree-real.sh line 16

Quoted 7 lines

t/t4301-merge-tree-real.sh new file +7 -0

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

t/t4301-merge-tree-real.sh line 21

Quoted 5 lines

t/t4301-merge-tree-real.sh new file +5 -0

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

t/t4301-merge-tree-real.sh line 28

Quoted 8 lines

t/t4301-merge-tree-real.sh new file +7 -0

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

Quoted 4 lines

t/t4301-merge-tree-real.sh new file +4 -0

+     test_write_lines 0 1 2 3 4 5 >numbers &&
+     echo yo >greeting &&
+     git rm whatever &&
+     mkdir whatever &&

t/t4301-merge-tree-real.sh line 42

Quoted 10 lines

t/t4301-merge-tree-real.sh new file +10 -0

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

t/t4301-merge-tree-real.sh line 44

Quoted 2 lines

t/t4301-merge-tree-real.sh new file +1 -0

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

t/t4301-merge-tree-real.sh line 46

Quoted 3 lines

t/t4301-merge-tree-real.sh new file +2 -0

+     test_must_fail git merge-tree --real side1 side2 >RESULT &&
+     R_TREE=$(cat RESULT) &&

How about actual_tree instead?

Quoted 22 lines

t/t4301-merge-tree-real.sh new file +22 -0

+
+     # 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' '

t/t4301-merge-tree-real.sh line 73

Quoted 5 lines

t/t4301-merge-tree-real.sh new file +5 -0

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

Quoted 5 lines

t/t4301-merge-tree-real.sh new file +5 -0

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

t/t4301-merge-tree-real.sh line 81

Quoted 3 lines

t/t4301-merge-tree-real.sh new file +3 -0

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

Quoted 9 lines · Johannes Schindelin

Quoted 5 lines · Elijah Newren via GitGitGadget

SYNOPSIS
--------
[verse]
+'git merge-tree' --real <branch1> <branch2>
'git merge-tree' <base-tree> <branch1> <branch2>

Quoted 2 lines

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

Quoted 19 lines

Quoted 6 lines

t/t4301-merge-tree-real.sh new file +0 -0

new file mode 100755
index 00000000000..f7aa310f8c1

t/t4301-merge-tree-real.sh line 9

Quoted 9 lines

t/t4301-merge-tree-real.sh new file +9 -0

@@ -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:

Quoted 4 lines

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

Quoted 10 lines

t/t4301-merge-tree-real.sh line 16

Quoted 6 lines

t/t4301-merge-tree-real.sh new file +6 -0

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

Quoted 8 lines

t/t4301-merge-tree-real.sh line 21

Quoted 5 lines

t/t4301-merge-tree-real.sh new file +5 -0

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

Quoted 9 lines

Quoted 8 lines

t/t4301-merge-tree-real.sh new file +7 -0

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

Quoted 2 lines

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.

Quoted 9 lines

t/t4301-merge-tree-real.sh line 73

Quoted 6 lines

t/t4301-merge-tree-real.sh new file +6 -0

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

Hi Elijah,

Quoted 11 lines · Elijah Newren

Quoted 9 lines · Johannes Schindelin

Quoted 5 lines · Elijah Newren via GitGitGadget

SYNOPSIS
--------
[verse]
+'git merge-tree' --real <branch1> <branch2>
'git merge-tree' <base-tree> <branch1> <branch2>

Quoted 4 lines

Quoted 2 lines

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

See? I know why I'm refraining from nitpicking. It's just not good for anyone involved.

Quoted 19 lines

Quoted 19 lines

Quoted 6 lines

t/t4301-merge-tree-real.sh new file +0 -0

new file mode 100755
index 00000000000..f7aa310f8c1

t/t4301-merge-tree-real.sh line 9

Quoted 9 lines

t/t4301-merge-tree-real.sh new file +9 -0

@@ -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:

Quoted 8 lines

Quoted 4 lines

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

Yes. Sorry.

Quoted 11 lines

Quoted 10 lines

t/t4301-merge-tree-real.sh line 16

Quoted 6 lines

t/t4301-merge-tree-real.sh new file +6 -0

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

Quoted 5 lines

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.

Nevertheless. To make comparing logs of two different test runs easier, it makes more sense to insist on consistency.

Quoted 21 lines

Quoted 8 lines

t/t4301-merge-tree-real.sh line 21

Quoted 5 lines

t/t4301-merge-tree-real.sh new file +5 -0

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

Quoted 9 lines

Quoted 8 lines

t/t4301-merge-tree-real.sh new file +7 -0

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

Quoted 6 lines

Quoted 2 lines

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.

Right. Different developers, different preferences. I wish we had a standard way in the test suite to initialize a test setup that _everybody_ could agree to be succinct and helpful. So far, we use shell scripted Git commands to recreate an initial commit topology, but especially when comparing to existing test suites with fixtures that are not only well-documented but also easy to wrap your head around, I find Git's test suite awfully lacking. Mind you, the code _I_ introduced isn't stellar in this respect, either, not by a very far stretch.

Quoted 10 lines

Quoted 9 lines

t/t4301-merge-tree-real.sh line 73

Quoted 6 lines

t/t4301-merge-tree-real.sh new file +6 -0

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

Quoted 4 lines

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

Fair enough.

Ciao,
Dscho

Quoted 5 lines

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

Quoted 7 lines · Elijah Newren

Quoted 5 lines

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

I wondered if we can _also_ extend the trivial-merge mode so that we do not have to call it "demo".

The internal result is expressed in this way:

    struct merge_list {
            struct merge_list *next;
            struct merge_list *link;	/* other stages for this object */

            unsigned int stage : 2;
            unsigned int mode;
            const char *path;
            struct blob *blob;
    };

because the command was not designed to resolve content level merges, but show the half-resolved state with the "stage" number. The "explanation" the command gives on the result is truly trivial, but there is no reason for it to stay that way.

The only output is:
- the toplevel resulting tree printed on stdout
- exit status of 0 (clean) or 1 (conflicts present)

Quoted 3 lines · Elijah Newren via GitGitGadget

The only output is:
- the toplevel resulting tree printed on stdout
- exit status of 0 (clean) or 1 (conflicts present)

I thought that the merge-ort API could (at least theoretically according to merge-ort.h) return something < 0 in case of internal error. In this case I would be interested in knowing what's the output of the command.

Documentation/git-merge-tree.txt git-merge-tree(1)

Quoted 4 lines

Documentation/git-merge-tree.txt +4 -0

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

No mention of what happens in case of an internal error in the merge-ort API.

builtin/merge-tree.c struct merge_tree_options { · line 448

Quoted 4 lines

builtin/merge-tree.c +4 -0

+       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);
+       return result.clean ? 0 : 1;

If result.clean can be < 0, this might pretend that the merge was clean.