RFC: In-core git merge-tree ("Server side merges")

From: Elijah Newren <[email protected]>

Let merge-tree accept a --write-tree parameter for choosing real merges instead of trivial merges, and accept an optional --trivial-merge option to get the traditional behavior. Note that these accept different numbers of arguments, though, so these names need not actually be used.

Note that real merges differ from trivial merges in that they handle:
- three way content merges
- recursive ancestor consolidation
- renames
- proper directory/file conflict handling
- etc.
Basically all the stuff you'd expect from git merge, just without updating the index and working tree. The initial shell added here does nothing more than die with "real merges are not yet implemented", but that will be fixed in subsequent commits.

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

---
 builtin/merge-tree.c | 67 ++++++++++++++++++++++++++++++++++++++------
 git.c                |  2 +-
 2 files changed, 59 insertions(+), 10 deletions(-)

builtin/merge-tree.c +27 -9

index 914ec960b7e..33e47cc1534 100644
@@ -3,13 +3,12 @@
 #include "tree-walk.h"
 #include "xdiff-interface.h"
 #include "object-store.h"
+#include "parse-options.h"
 #include "repository.h"
 #include "blob.h"
 #include "exec-cmd.h"
 #include "merge-blobs.h"
 
-static const char merge_tree_usage[] = "git merge-tree <base-tree> <branch1> <branch2>";
-
 struct merge_list {
 	struct merge_list *next;
 	struct merge_list *link;	/* other stages for this object */

@@ -366,15 +365,17 @@ static void *get_tree_descriptor(struct repository *r,
 	return buf;
 }
 
-static int trivial_merge(int argc, const char **argv)
+static int trivial_merge(const char *base,
+			 const char *branch1,
+			 const char *branch2)
 {
 	struct repository *r = the_repository;
 	struct tree_desc t[3];
 	void *buf1, *buf2, *buf3;
 
-	buf1 = get_tree_descriptor(r, t+0, argv[1]);
-	buf2 = get_tree_descriptor(r, t+1, argv[2]);
-	buf3 = get_tree_descriptor(r, t+2, argv[3]);
+	buf1 = get_tree_descriptor(r, t+0, base);
+	buf2 = get_tree_descriptor(r, t+1, branch1);
+	buf3 = get_tree_descriptor(r, t+2, branch2);
 	trivial_merge_trees(t, "");
 	free(buf1);
 	free(buf2);

@@ -384,9 +385,57 @@ static int trivial_merge(int argc, const char **argv)
 	return 0;
 }
 
+struct merge_tree_options {
+	int real;
+	int trivial;
+};
+
+static int real_merge(struct merge_tree_options *o,
+		      const char *branch1, const char *branch2)
+{
+	die(_("real merges are not yet implemented"));
+}
+
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
-	if (argc != 4)
-		usage(merge_tree_usage);
-	return trivial_merge(argc, argv);
+	struct merge_tree_options o = { 0 };
+	int expected_remaining_argc;
+
+	const char * const merge_tree_usage[] = {
+		N_("git merge-tree [--write-tree] <branch1> <branch2>"),
+		N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
+		NULL
+	};
+	struct option mt_options[] = {

builtin/merge-tree.c +15 -0

+	if (o.real || o.trivial) {
+		expected_remaining_argc = (o.real ? 2 : 3);
+		if (argc != expected_remaining_argc)
+			usage_with_options(merge_tree_usage, mt_options);
+	} else {
+		if (argc < 2 || argc > 3)
+			usage_with_options(merge_tree_usage, mt_options);
+		o.real = (argc == 2);
+	}
+
+	/* Do the relevant type of merge */
+	if (o.real)
+		return real_merge(&o, argv[0], argv[1]);
+	else
+		return trivial_merge(argv[0], argv[1], argv[2]);
 }

git.c +1 -1

index 5ff21be21f3..6090a1289db 100644
@@ -558,7 +558,7 @@ static struct cmd_struct commands[] = {
 	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-tree", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },
+	{ "merge-tree", cmd_merge_tree, RUN_SETUP },
 	{ "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
 	{ "mktree", cmd_mktree, RUN_SETUP },
 	{ "multi-pack-index", cmd_multi_pack_index, RUN_SETUP },

From: Elijah Newren <[email protected]>

Let merge-tree accept a --write-tree parameter for choosing real merges instead of trivial merges, and accept an optional --trivial-merge option to get the traditional behavior. Note that these accept different numbers of arguments, though, so these names need not actually be used.

Note that real merges differ from trivial merges in that they handle:
- three way content merges
- recursive ancestor consolidation
- renames
- proper directory/file conflict handling
- etc.
Basically all the stuff you'd expect from git merge, just without updating the index and working tree. The initial shell added here does nothing more than die with "real merges are not yet implemented", but that will be fixed in subsequent commits.

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

---
 builtin/merge-tree.c | 67 ++++++++++++++++++++++++++++++++++++++------
 git.c                |  2 +-
 2 files changed, 59 insertions(+), 10 deletions(-)

builtin/merge-tree.c +27 -9

index 914ec960b7e..33e47cc1534 100644
@@ -3,13 +3,12 @@
 #include "tree-walk.h"
 #include "xdiff-interface.h"
 #include "object-store.h"
+#include "parse-options.h"
 #include "repository.h"
 #include "blob.h"
 #include "exec-cmd.h"
 #include "merge-blobs.h"
 
-static const char merge_tree_usage[] = "git merge-tree <base-tree> <branch1> <branch2>";
-
 struct merge_list {
 	struct merge_list *next;
 	struct merge_list *link;	/* other stages for this object */

@@ -366,15 +365,17 @@ static void *get_tree_descriptor(struct repository *r,
 	return buf;
 }
 
-static int trivial_merge(int argc, const char **argv)
+static int trivial_merge(const char *base,
+			 const char *branch1,
+			 const char *branch2)
 {
 	struct repository *r = the_repository;
 	struct tree_desc t[3];
 	void *buf1, *buf2, *buf3;
 
-	buf1 = get_tree_descriptor(r, t+0, argv[1]);
-	buf2 = get_tree_descriptor(r, t+1, argv[2]);
-	buf3 = get_tree_descriptor(r, t+2, argv[3]);
+	buf1 = get_tree_descriptor(r, t+0, base);
+	buf2 = get_tree_descriptor(r, t+1, branch1);
+	buf3 = get_tree_descriptor(r, t+2, branch2);
 	trivial_merge_trees(t, "");
 	free(buf1);
 	free(buf2);

@@ -384,9 +385,57 @@ static int trivial_merge(int argc, const char **argv)
 	return 0;
 }
 
+struct merge_tree_options {
+	int real;
+	int trivial;
+};
+
+static int real_merge(struct merge_tree_options *o,
+		      const char *branch1, const char *branch2)
+{
+	die(_("real merges are not yet implemented"));
+}
+
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
-	if (argc != 4)
-		usage(merge_tree_usage);
-	return trivial_merge(argc, argv);
+	struct merge_tree_options o = { 0 };
+	int expected_remaining_argc;
+
+	const char * const merge_tree_usage[] = {
+		N_("git merge-tree [--write-tree] <branch1> <branch2>"),
+		N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
+		NULL
+	};
+	struct option mt_options[] = {

builtin/merge-tree.c +5 -0

+	/* Do the relevant type of merge */
+	if (o.real)
+		return real_merge(&o, argv[0], argv[1]);
+	else
+		return trivial_merge(argv[0], argv[1], argv[2]);
 }

git.c +1 -1

index 5ff21be21f3..6090a1289db 100644
@@ -558,7 +558,7 @@ static struct cmd_struct commands[] = {
 	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-tree", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },
+	{ "merge-tree", cmd_merge_tree, RUN_SETUP },
 	{ "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
 	{ "mktree", cmd_mktree, RUN_SETUP },
 	{ "multi-pack-index", cmd_multi_pack_index, RUN_SETUP },

merge-ort.c +0 -19

-	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;
-
-		if (opt->record_conflict_msgs_as_headers)
-			BUG("Either display conflict messages or record them as headers, not both");
-
-		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;

From: Elijah Newren <[email protected]>

Callers of git merge-tree --write-tree will often want to know which files had conflicts. While they could potentially attempt to parse the CONFLICT notices printed, those messages are not meant to be machine readable. Provide a simpler mechanism of just printing the files (in the same format as git ls-files with quoting, but restricted to [...]

builtin/merge-tree.c +0 -0

index 560640ad911..d8eeeb3f306 100644

builtin/merge-tree.c +4 -1

 struct merge_list {
 	struct merge_list *next;

@@ -395,7 +398,8 @@ struct merge_tree_options {
 };
 
 static int real_merge(struct merge_tree_options *o,
-		      const char *branch1, const char *branch2)
+		      const char *branch1, const char *branch2,
+		      const char *prefix)
 {
 	struct commit *parent1, *parent2;
 	struct commit_list *common;

@@ -449,6 +453,22 @@ static int real_merge(struct merge_tree_options *o,
 		o->show_messages = !result.clean;
 
 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
+	if (!result.clean) {
+		struct string_list conflicted_files = STRING_LIST_INIT_NODUP;

From: Elijah Newren <[email protected]>

Much like git merge updates the index with information of the form
(mode, oid, stage, name)
provide this output for conflicted files for merge-tree as well. Provide an --exclude-oids-and-modes option for users to exclude the mode, oid, and stage and only get the list of conflicted filenames. [...]
+--exclude-oids-and-modes::

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

+	Instead of writing a list of (mode, oid, stage, path) tuples
+	to output for conflicted files, just provide a list of
+	filenames with conflicts.
+
[...]
-This is a sequence of lines containing a filename on each line, quoted
-as explained for the configuration variable `core.quotePath` (see
-linkgit:git-config[1]).
+This is a sequence of lines with the format
+
+	<mode> <object> <stage> <filename>
+
+The filename will be quoted as explained for the configuration
+variable `core.quotePath` (see linkgit:git-config[1]).  However, if
+the `--exclude-oids-and-modes` option is passed, the mode, object, and
+stage will be omitted.
 
 Informational messages
 ~~~~~~~~~~~~~~~~~~~~~~
 
 This always starts with a blank line to separate it from the previous
-section, and then has free-form messages about the merge, such as:
+sections, and then has free-form messages about the merge, such as:
 
   * "Auto-merging <file>"
   * "CONFLICT (rename/delete): <oldfile> renamed...but deleted in..."

@@ -113,6 +123,14 @@ plumbing commands since the possibility of merge conflicts give it a
 much higher chance of the command not succeeding (and NEWTREE containing
 a bunch of stuff other than just a toplevel tree).
 
+git-merge-tree was written to provide users with the same information
+that they'd have access to if using `git merge`:
+  * what would be written to the working tree (the <OID of toplevel tree>)
+  * the higher order stages that would be written to the index (the
+    <Conflicted file info>)
+  * any messages that would have been printed to stdout (the <Informational
+    messages>)
+
 GIT
 ---
 Part of the linkgit:git[1] suite

builtin/merge-tree.c +6 -1

index d8eeeb3f306..7aa7f9fd54a 100644
@@ -395,6 +395,7 @@ struct merge_tree_options {
 	int real;
 	int trivial;
 	int show_messages;
+	int exclude_oids_and_modes;
 };
 
 static int real_merge(struct merge_tree_options *o,

@@ -461,7 +462,11 @@ static int real_merge(struct merge_tree_options *o,
 		merge_get_conflicted_files(&result, &conflicted_files);
 		for (i = 0; i < conflicted_files.nr; i++) {
 			const char *name = conflicted_files.items[i].string;
-			if (last && !strcmp(last, name))
+			struct stage_info *c = conflicted_files.items[i].util;
+			if (!o->exclude_oids_and_modes)
+				printf("%06o %s %d\t",
+				       c->mode, oid_to_hex(&c->oid), c->stage);
+			else if (last && !strcmp(last, name))
 				continue;
 			write_name_quoted_relative(
 				name, prefix, stdout, line_termination);

Am 22.01.22 um 22:55 schrieb Elijah Newren via GitGitGadget:

builtin/merge-tree.c static int trivial_merge(int argc, const char **argv) · line 419

Quoted 3 lines

builtin/merge-tree.c +3 -0

+     /* Check for a request for basic help */
+     if (argc == 2 && !strcmp(argv[1], "-h"))
+             usage_with_options(merge_tree_usage, mt_options);

This is unnecessary; parse_options() handles -h already.

builtin/merge-tree.c static int trivial_merge(int argc, const char **argv) · line 425

Quoted 6 lines

builtin/merge-tree.c +6 -0

+
+     /* Parse arguments */
+     argc = parse_options(argc, argv, prefix, mt_options,
+                          merge_tree_usage, 0);
+     if (o.real && o.trivial)
+             die(_("--write-tree and --trivial-merge are incompatible"));

12909b6b8a (i18n: turn "options are incompatible" into "cannot be used together", 2022-01-05) standardized messages of that kind; let's stick to that to simplify translation:

die(_("options '%s' and '%s' cannot be used together"),
"--write-tree", "--trivial-merge");

On Sat, Jan 22 2022, Elijah Newren via GitGitGadget wrote:

builtin/merge-tree.c static int trivial_merge(int argc, const char **argv) · line 419

Quoted 3 lines

builtin/merge-tree.c +3 -0

+     /* Check for a request for basic help */
+     if (argc == 2 && !strcmp(argv[1], "-h"))
+             usage_with_options(merge_tree_usage, mt_options);

Is this bit cargo-culted from something else, perhaps non-parse-options.c usage? I don't think this is needed, the parse_options() below intercepts "-h" by default.

builtin/merge-tree.c static int trivial_merge(int argc, const char **argv) · line 425

Quoted 5 lines

builtin/merge-tree.c +5 -0

+     /* Parse arguments */
+     argc = parse_options(argc, argv, prefix, mt_options,
+                          merge_tree_usage, 0);
+     if (o.real && o.trivial)
+             die(_("--write-tree and --trivial-merge are incompatible"));

Shouldn't those two just be OPT_CMDMODE()? Then you get this incompatibility checking for free. See 485fd2c3dae (cat-file: make --batch-all-objects a CMDMODE, 2021-12-28).

builtin/merge-tree.c static int trivial_merge(int argc, const char **argv) · line 434

Quoted 9 lines

builtin/merge-tree.c +9 -0

+     if (o.real || o.trivial) {
+             expected_remaining_argc = (o.real ? 2 : 3);
+             if (argc != expected_remaining_argc)
+                     usage_with_options(merge_tree_usage, mt_options);
+     } else {
+             if (argc < 2 || argc > 3)
+                     usage_with_options(merge_tree_usage, mt_options);
+             o.real = (argc == 2);
+     }

And this can also be done like this, but I wonder if using PARSE_OPT_STOP_AT_NON_OPTION and then routing to a sub-function wouldn't be better, i.e. to treat these like sub-commands if they've got different arity etc.

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

Quoted 7 lines · Elijah Newren via GitGitGadget

builtin/merge-tree.c +7 -0

+     /*
+      * 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]))

Better omitted WIP code in a non-RFC series?

Quoted 1 line

+ die(_("Unknown strategy option: -X%s"), xopts[x]);

As a general issue with this series, die(), BUG() etc. messages should start with a non-capital letter.

Quoted 1 line

+ printf("%s\n", oid_to_hex(&result.tree->object.oid));

And for both this...

Quoted 1 line

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

... and this we can just use puts(). For the former it's just less code, but for the latter translators also don't need to see the always-there \n in the translated message.

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

Quoted 2 lines

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

+# This test is ort-specific
+test "${GIT_TEST_MERGE_ALGORITHM:-ort}" = ort || {

Is this ${} trickery really needed? We're not testing with "set -u". So just:

        if test "$GIT_..." != "ort"
        then
                ...
        fi

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

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 --write-tree side1 side2 side3 2>expect &&
+
+     grep "^usage: git merge-tree" expect
+'

Nit: In most other tests these usage assertions are at the top of the test, and for those we also make do with just testing the 129 exit code, which is probably enough here too...

Quoted 13 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Callers of git merge-tree --write-tree will often want to know which files had conflicts. While they could potentially attempt to parse the CONFLICT notices printed, those messages are not meant to be machine readable. Provide a simpler mechanism of just printing the files (in the same format as git ls-files with quoting, but restricted to [...]

builtin/merge-tree.c +0 -0

index 560640ad911..d8eeeb3f306 100644

builtin/merge-tree.c line 16

Quoted 6 lines

builtin/merge-tree.c +3 -0

@@ -11,6 +11,9 @@
 #include "blob.h"
 #include "exec-cmd.h"
 #include "merge-blobs.h"
+#include "quote.h"
+
+static int line_termination = '\n';

But unlike ls-files we don't do anything with line_termination as a != '\n', maybe in a later commit?

Quoted 15 lines

builtin/merge-tree.c +2 -1

 struct merge_list {
      struct merge_list *next;

@@ -395,7 +398,8 @@ struct merge_tree_options {
 };
 
 static int real_merge(struct merge_tree_options *o,
-                   const char *branch1, const char *branch2)
+                   const char *branch1, const char *branch2,
+                   const char *prefix)
 {
      struct commit *parent1, *parent2;
      struct commit_list *common;

@@ -449,6 +453,22 @@ static int real_merge(struct merge_tree_options *o,
              o->show_messages = !result.clean;
 

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

Quoted 13 lines

builtin/merge-tree.c +12 -0

      printf("%s\n", oid_to_hex(&result.tree->object.oid));
+     if (!result.clean) {
+             struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
+             const char *last = NULL;
+             int i;
+
+             merge_get_conflicted_files(&result, &conflicted_files);
+             for (i = 0; i < conflicted_files.nr; i++) {
+                     const char *name = conflicted_files.items[i].string;
+                     if (last && !strcmp(last, name))
+                             continue;
+                     write_name_quoted_relative(
+                             name, prefix, stdout, line_termination);

But here it's never \0 or whatever.

Quoted 82 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Much like git merge updates the index with information of the form
(mode, oid, stage, name)
provide this output for conflicted files for merge-tree as well. Provide an --exclude-oids-and-modes option for users to exclude the mode, oid, and stage and only get the list of conflicted filenames. [...]
+--exclude-oids-and-modes::

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

+     Instead of writing a list of (mode, oid, stage, path) tuples
+     to output for conflicted files, just provide a list of
+     filenames with conflicts.
+
[...]
-This is a sequence of lines containing a filename on each line, quoted
-as explained for the configuration variable `core.quotePath` (see
-linkgit:git-config[1]).
+This is a sequence of lines with the format
+
+     <mode> <object> <stage> <filename>
+
+The filename will be quoted as explained for the configuration
+variable `core.quotePath` (see linkgit:git-config[1]).  However, if
+the `--exclude-oids-and-modes` option is passed, the mode, object, and
+stage will be omitted.
 
 Informational messages
 ~~~~~~~~~~~~~~~~~~~~~~
 
 This always starts with a blank line to separate it from the previous
-section, and then has free-form messages about the merge, such as:
+sections, and then has free-form messages about the merge, such as:
 
   * "Auto-merging <file>"
   * "CONFLICT (rename/delete): <oldfile> renamed...but deleted in..."

@@ -113,6 +123,14 @@ plumbing commands since the possibility of merge conflicts give it a
 much higher chance of the command not succeeding (and NEWTREE containing
 a bunch of stuff other than just a toplevel tree).
 
+git-merge-tree was written to provide users with the same information
+that they'd have access to if using `git merge`:
+  * what would be written to the working tree (the <OID of toplevel tree>)
+  * the higher order stages that would be written to the index (the
+    <Conflicted file info>)
+  * any messages that would have been printed to stdout (the <Informational
+    messages>)
+
 GIT
 ---
 Part of the linkgit:git[1] suite

builtin/merge-tree.c +9 -1

index d8eeeb3f306..7aa7f9fd54a 100644
@@ -395,6 +395,7 @@ struct merge_tree_options {
      int real;
      int trivial;
      int show_messages;
+     int exclude_oids_and_modes;
 };
 
 static int real_merge(struct merge_tree_options *o,

@@ -461,7 +462,11 @@ static int real_merge(struct merge_tree_options *o,
              merge_get_conflicted_files(&result, &conflicted_files);
              for (i = 0; i < conflicted_files.nr; i++) {
                      const char *name = conflicted_files.items[i].string;
-                     if (last && !strcmp(last, name))
+                     struct stage_info *c = conflicted_files.items[i].util;
+                     if (!o->exclude_oids_and_modes)
+                             printf("%06o %s %d\t",
+                                    c->mode, oid_to_hex(&c->oid), c->stage);
+                     else if (last && !strcmp(last, name))
                              continue;
                      write_name_quoted_relative(
                              name, prefix, stdout, line_termination);

@@ -495,6 +500,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
                       N_("do a trivial merge only")),
              OPT_BOOL(0, "messages", &o.show_messages,
                       N_("also show informational/conflict messages")),
+             OPT_BOOL_F(0, "exclude-oids-and-modes",
+                        &o.exclude_oids_and_modes,
+                        N_("list conflicted files without oids and modes"),

builtin/merge-tree.c int cmd_merge_tree(int argc, const char **argv, const char *prefix) · line 508

Quoted 3 lines

builtin/merge-tree.c +1 -0

+                        PARSE_OPT_NONEG),
              OPT_END()
      };

Perhaps this really is the last formatting information anyone will want, but with a default of "<mode> <object> <stage> <filename>" being made "<stage> <filename>" with --exclude-oids-and-modes perhaps we'll want --exclude-all-except-filename etc. later.

It seems a lot simpler for new code to just support a --conflict-format option, lifting some code from the in-flight
https://lore.kernel.org/git/db058bf670c5668fc5b95baf83667cc282cb739b.1641978175.git.dyroneteng@gmail.com/

I.e. that inner loop becomes a slightly more verbose strbuf_expand(), but it's all easy boilerplate code.

Then we just feed "%(objectmode) %(objectname) %(objectstage) %(filename)" into it by default, and allow the user to customize it.

merge-ort.c static int record_conflicted_index_entries(struct merge_options *opt) · line 4254

Quoted 5 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>
[...]

merge-ort.c +3 -0

+     /* Hack to pre-allocate olist to the desired size */
+     ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
+                olist.alloc);

Perhaps just add a string_list_grow()? But I wonder if this is really needed v.s. just using the default growing pattern here.

merge-ort.c static int record_conflicted_index_entries(struct merge_options *opt) · line 4267

Quoted 13 lines

merge-ort.c +13 -0

+
+     /* 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);
+     }

Shorter/nicer:

        for_each_string_list_item(item, &olist) {
                struct strbuf *sb = item->util;
                puts(sb->buf);
        }

Quoted 24 lines

merge-ort.c +0 -24

-     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;
-
-             if (opt->record_conflict_msgs_as_headers)
-                     BUG("Either display conflict messages or record them as headers, not both");
-
-             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) {

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

Quoted 5 lines

merge-ort.c +0 -5

-                     struct strbuf *sb = olist.items[i].util;
-
-                     printf("%s", sb->buf);
-             }
-             string_list_clear(&olist, 0);

Ah, at this point I see you're just moving code around :) Sending this anyway in case it's useful :)

Updates since v2 (thanks to Christian, Dscho, Ramsay, and René for suggestions and comments on v2):

* Significant changes to output format:
* Flags no longer take a filename for additional output; they write to stdout instead.
* More information included by default when there are conflicts (no need to request it with additional flags, instead flags can be used to suppress it).
* Provide (mode, oid, stage, file) tuples -- i.e. ls-files -u style of information -- when there are conflicts. Add a flag to only list conflicted files if that's preferred.

* Much more thorough manual for git-merge-tree.txt
* Renamed option from --real to --write-tree
* Accept an optional --trivial-merge option to get old style merge-tree behavior
* Allow both --write-tree and --trivial-merge to be omitted since we can deduce which from number of arguments

* Document exit code when the merge cannot be run (so we can distinguish other error cases from conflicts)
* testcase cleanups: test_tick, early skip of test when using recursive backend, variable renames, etc.
* various minor code cleanups
* Add a new --allow-unrelated-histories option (with same meaning as the one used in git merge)

Stuff intentionally NOT included, but which others seemed to feel strongly about; they'd need to convince me more on these:

* Any form of diff output[1]

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)

The rest of this manual (other than the

builtin/merge-tree.c +3 -1

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

After a merge, this function allows the user to extract the same information that would be printed by ls-files -u -- conflicted

files with their mode, oid, and stage.

merge-ort.c +11 -0

+void merge_get_conflicted_files(struct merge_result *result,
+                               struct string_list *conflicted_files)
+{
+       struct hashmap_iter iter;
+       struct strmap_entry *e;
+       struct merge_options_internal *opti = result->priv;
+
+       strmap_for_each_entry(&opti->conflicted, &iter, e) {
+               const char *path = e->key;
+               struct conflict_info *ci = e->value;
+               int i;

merge-ort.c +9 -0

+                       si->stage = i+1;
+                       si->mode = ci->stages[i].mode;
+                       oidcpy(&si->oid, &ci->stages[i].oid);
+                       string_list_append(conflicted_files, path)->util = si;
+               }
+       }
+       /* string_list_sort() uses a stable sort, so we're good */
+       string_list_sort(conflicted_files);
+}

Quoted 4 lines · Elijah Newren via GitGitGadget

* Accept an optional --trivial-merge option to get old style merge-tree behavior
* Allow both --write-tree and --trivial-merge to be omitted since we can deduce which from number of arguments

I still think that it might be simpler and cleaner to leave 'git merge-tree' alone for now, and just add a new command named for example 'git write-merge-tree'.

Later we can always add flags to 'git merge-tree' or add 'git trivial-merge-tree' as an alias for 'git merge-tree', and eventually slowly switch 'git merge-tree' to mean only 'git write-merge-tree' if that's where we want to go.

Quoted 4 lines

Stuff intentionally NOT included, but which others seemed to feel strongly about; they'd need to convince me more on these:

* Any form of diff output[1]

It's not a big issue for me to not include them right now as long as it's possible to add cli options later that add them.

The reason is that I think in many cases when there are conflicts, the conflicts will be small and the user will want to see them. So it would be simpler to just have an option to show any conflict right away, rather than have the user launch another command (a diff-tree against which tree and with which options?).

Hi Christian,

Quoted 11 lines · Christian Couder

Quoted 4 lines · Elijah Newren via GitGitGadget

* Accept an optional --trivial-merge option to get old style merge-tree behavior
* Allow both --write-tree and --trivial-merge to be omitted since we can deduce which from number of arguments

I still think that it might be simpler and cleaner to leave 'git merge-tree' alone for now, and just add a new command named for example 'git write-merge-tree'.

That would assume that the original git merge-tree implementation was useful. That notion has been thoroughly refuted in the meantime, though.

I am really opposed to introducing a new command here. Elijah took the best approach we can take here: save the merge-tree command by teaching it to do something useful.

Quoted 4 lines

Later we can always add flags to 'git merge-tree' or add 'git trivial-merge-tree' as an alias for 'git merge-tree', and eventually slowly switch 'git merge-tree' to mean only 'git write-merge-tree' if that's where we want to go.

I suggested before, and seem to need to repeat again, that we need to let ourselves be guided less by hypothetical scenarios, and more by actual, concrete use cases where the revamped merge-tree command is useful.

And since I already provided some feedback based on my work from working on a server-side backend, I am fairly certain that we already have a pretty good idea where we want to go.

Quoted 7 lines

Quoted 4 lines

Stuff intentionally NOT included, but which others seemed to feel strongly about; they'd need to convince me more on these:

* Any form of diff output[1]

It's not a big issue for me to not include them right now as long as it's possible to add cli options later that add them.

But why? That _so_ smells like a hypothetical scenario.

We do not need the diffs. It is highly unlikely that the server-side wants to have diffs, and if a user does want the diffs, it is very, very easy to generate them by chaining low-level commands.

So there is absolutely no need for git merge-tree to produce diffs.

Quoted 5 lines

The reason is that I think in many cases when there are conflicts, the conflicts will be small and the user will want to see them. So it would be simpler to just have an option to show any conflict right away, rather than have the user launch another command (a diff-tree against which tree and with which options?).

That assumes that server-side merge UIs will present merge conflicts in the form of diffs containing merge conflict markers. Which I don't think will happen, like, ever.

In short: I completely disagree that we should introduce a new command, and I also completely disagree that the merge-tree command should output any diffs.

I do agree that we need to be mindful of what we actually need, and in that regard, I reiterate that we need to let concrete use cases guide us. As part of GitLab, you might be in an excellent position to look at GitLab's concrete server-side needs when it comes to use git merge-tree to perform merges.

Quoted 9 lines · Johannes Schindelin

Quoted 5 lines · Christian Couder

The reason is that I think in many cases when there are conflicts, the conflicts will be small and the user will want to see them. So it would be simpler to just have an option to show any conflict right away, rather than have the user launch another command (a diff-tree against which tree and with which options?).

Quoted 3 lines

That assumes that server-side merge UIs will present merge conflicts in the form of diffs containing merge conflict markers. Which I don't think will happen, like, ever.

Please take a look at:

https://docs.gitlab.com/ee/user/project/merge_requests/conflicts.html#resolve-conflicts-in-the-inline-editor

As you can see in the image there are conflict markers in the file displayed by the server UI.

Quoted 9 lines

In short: I completely disagree that we should introduce a new command, and I also completely disagree that the merge-tree command should output any diffs.

I do agree that we need to be mindful of what we actually need, and in that regard, I reiterate that we need to let concrete use cases guide us. As part of GitLab, you might be in an excellent position to look at GitLab's concrete server-side needs when it comes to use git merge-tree to perform merges.

I hope I provided a concrete use case with the link above.

Hi Christian,

Quoted 15 lines · Christian Couder

Quoted 9 lines · Johannes Schindelin

Quoted 5 lines · Christian Couder

The reason is that I think in many cases when there are conflicts, the conflicts will be small and the user will want to see them. So it would be simpler to just have an option to show any conflict right away, rather than have the user launch another command (a diff-tree against which tree and with which options?).

Quoted 3 lines

That assumes that server-side merge UIs will present merge conflicts in the form of diffs containing merge conflict markers. Which I don't think will happen, like, ever.

Quoted 6 lines

Please take a look at:

https://docs.gitlab.com/ee/user/project/merge_requests/conflicts.html#resolve-conflicts-in-the-inline-editor

As you can see in the image there are conflict markers in the file displayed by the server UI.

Please note the difference between what I wrote above (present merge conflicts in the form of diffs containing merge conflict markers) and what is shown in the document you linked to (present a file annotated with merge conflict markers).

There is no diff in that page.

What's more: there are not only conflict markers in the editor,

there is
clearly a visual marker next to the line number that indicates that the editor has a fundamental understanding where the conflict markers are.

Which means that the conflict markers must have been generated independently of Git rather than parsed in some random diff that was produced by Git.

In other words: you are making my case for me that git merge-tree should not generate diff output because it would not even be used.

Quoted 10 lines

Quoted 9 lines

In short: I completely disagree that we should introduce a new command, and I also completely disagree that the merge-tree command should output any diffs.

I do agree that we need to be mindful of what we actually need, and in that regard, I reiterate that we need to let concrete use cases guide us. As part of GitLab, you might be in an excellent position to look at GitLab's concrete server-side needs when it comes to use git merge-tree to perform merges.

Quoted 1 line

I hope I provided a concrete use case with the link above.

Sorry, I apparently was a bit unclear.

In the context of discussing git merge-tree, a low-level Git command, when I talk about a user, I do not mean a human being, but a program that calls that command and parses its output.

Of course, I am still left guessing what the server-side needs concretely, because that is not at all obvious from the user-facing web site to which you sent me. What is needed is a good, hard look at the actual _code_, the code that calls into libgit2 to perform a merge, and that could instead spawn a git merge-tree process to accomplish the same thing.

We need to get away from hypothetical scenarios. They're not helping.

Quoted 30 lines · Johannes Schindelin

Quoted 15 lines · Christian Couder

Quoted 9 lines · Johannes Schindelin

Quoted 5 lines · Christian Couder

The reason is that I think in many cases when there are conflicts, the conflicts will be small and the user will want to see them. So it would be simpler to just have an option to show any conflict right away, rather than have the user launch another command (a diff-tree against which tree and with which options?).

Quoted 3 lines

That assumes that server-side merge UIs will present merge conflicts in the form of diffs containing merge conflict markers. Which I don't think will happen, like, ever.

Quoted 6 lines

Please take a look at:

https://docs.gitlab.com/ee/user/project/merge_requests/conflicts.html#resolve-conflicts-in-the-inline-editor

As you can see in the image there are conflict markers in the file displayed by the server UI.

Please note the difference between what I wrote above (present merge conflicts in the form of diffs containing merge conflict markers) and what is shown in the document you linked to (present a file annotated with merge conflict markers).

Quoted 1 line

There is no diff in that page.

The server UI could just get a diff with the conflicts inside instead of the full files with conflict inside, as the diff would be smaller to parse than the full files. So even if it's not shown, the diff could still be useful.

Also just above the section of the link I sent, there is this section

https://docs.gitlab.com/ee/user/project/merge_requests/conflicts.html#resolve-conflicts-in-interactive-mode

where one can see diff markers in the image.

Quoted 1 line

What's more: there are not only conflict markers in the editor,

You don't see the ">>>>>>>"?

Quoted 3 lines

there is clearly a visual marker next to the line number that indicates that the editor has a fundamental understanding where the conflict markers are.

Yeah, so this shows that those markers can be important for the editor.

Quoted 2 lines

In other words: you are making my case for me that git merge-tree should not generate diff output because it would not even be used.

The other link above in this email actually shows that diffs are used right now to resolve conflicts.

From: Elijah Newren <[email protected]>

No functional changes included in this patch; it's just a preparatory step to allow the printed messages to be handled differently by other callers, such as in git merge-tree --write-tree.

merge-ort.c +2 -1

index f9e35b0f96b..b78dde55ad9 100644
@@ -4236,7 +4236,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 }
 
 void merge_display_update_messages(struct merge_options *opt,
-				   struct merge_result *result)
+				   struct merge_result *result,
+				   FILE *stream)
 {
 	struct merge_options_internal *opti = result->priv;
 	struct hashmap_iter iter;

merge-ort.c +1 -1

@@ -4313,7 +4314,7 @@ void merge_switch_to_result(struct merge_options *opt,
 	}
 
 	if (display_update_msgs)
-		merge_display_update_messages(opt, result);
+		merge_display_update_messages(opt, result, stdout);
 
 	merge_finalize(opt, result);
 }

merge-ort.h +2 -2

index e5aec45b18f..d643b47cb7c 100644
@@ -86,7 +86,8 @@ void merge_switch_to_result(struct merge_options *opt,
  * so only call this when bypassing merge_switch_to_result().
  */
 void merge_display_update_messages(struct merge_options *opt,
-				   struct merge_result *result);
+				   struct merge_result *result,
+				   FILE *stream);
 
 /* Do needed cleanup when not calling merge_switch_to_result() */
 void merge_finalize(struct merge_options *opt,
--

gitgitgadget

From: Elijah Newren <[email protected]>

When running git merge-tree --write-tree, 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 --[no-]messages option so that callers can request these messages be included at the end of the output. Include such messages by default when there are conflicts, and omit them by default when the merge is clean.

builtin/merge-tree.c +1 -2

index 0c19639594d..560640ad911 100644
@@ -440,22 +441,30 @@ static int real_merge(struct merge_tree_options *o,
 		commit_list_insert(j->item, &merge_bases);
 
 	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
-	printf("%s\n", oid_to_hex(&result.tree->object.oid));
+
 	if (result.clean < 0)
 		die(_("failure to merge"));
-	else if (!result.clean)

builtin/merge-tree.c +5 -2

 	merge_finalize(&opt, &result);
 	return !result.clean; /* result.clean < 0 handled above */
 }
 
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
-	struct merge_tree_options o = { 0 };
+	struct merge_tree_options o = { .show_messages = -1 };
 	int expected_remaining_argc;
+	int original_argc;
 
 	const char * const merge_tree_usage[] = {
-		N_("git merge-tree [--write-tree] <branch1> <branch2>"),
+		N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
 		N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
 		NULL
 	};

@@ -464,6 +473,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 			 N_("do a real merge instead of a trivial merge")),
 		OPT_BOOL(0, "trivial-merge", &o.trivial,
 			 N_("do a trivial merge only")),
+		OPT_BOOL(0, "messages", &o.show_messages,
+			 N_("also show informational/conflict messages")),
 		OPT_END()
 	};
 

builtin/merge-tree.c +0 -0

 	if (o.real || o.trivial) {
 		expected_remaining_argc = (o.real ? 2 : 3);
 		if (argc != expected_remaining_argc)

t/t4301-merge-tree-real.sh +21 -1

index e03688515c5..c34f8e6c1ed 100755
@@ -84,4 +84,25 @@ test_expect_success 'Barf on too many arguments' '
 	grep "^usage: git merge-tree" expect
 '
 
+test_expect_success 'test conflict notices and such' '
+	test_expect_code 1 git merge-tree --write-tree side1 side2 >out &&
+	sed -e "s/[0-9a-f]\{40,\}/HASH/g" out >actual &&
+
+	# 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 &&
+	HASH
+
+	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 actual
+'
+
 test_done
--

gitgitgadget

From: Elijah Newren <[email protected]>

After a merge, this function allows the user to extract the same information that would be printed by ls-files -u -- conflicted files with their mode, oid, and stage.

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

---
 merge-ort.c | 31 +++++++++++++++++++++++++++++++
 merge-ort.h | 21 +++++++++++++++++++++
 2 files changed, 52 insertions(+)

merge-ort.c +31 -0

index b78dde55ad9..5e7cea6cc8f 100644
@@ -4275,6 +4275,37 @@ void merge_display_update_messages(struct merge_options *opt,
 	trace2_region_leave("merge", "display messages", opt->repo);
 }
 
+void merge_get_conflicted_files(struct merge_result *result,
+				struct string_list *conflicted_files)
+{
+	struct hashmap_iter iter;
+	struct strmap_entry *e;
+	struct merge_options_internal *opti = result->priv;
+
+	strmap_for_each_entry(&opti->conflicted, &iter, e) {
+		const char *path = e->key;
+		struct conflict_info *ci = e->value;
+		int i;
+
+		VERIFY_CI(ci);
+
+		for (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {
+			struct stage_info *si;
+
+			if (!(ci->filemask & (1ul << i)))
+				continue;
+
+			si = xmalloc(sizeof(*si));
+			si->stage = i+1;
+			si->mode = ci->stages[i].mode;
+			oidcpy(&si->oid, &ci->stages[i].oid);
+			string_list_append(conflicted_files, path)->util = si;
+		}
+	}
+	/* string_list_sort() uses a stable sort, so we're good */
+	string_list_sort(conflicted_files);
+}
+
 void merge_switch_to_result(struct merge_options *opt,
 			    struct tree *head,
 			    struct merge_result *result,

merge-ort.h +1 -0

index d643b47cb7c..e635a294ea8 100644
@@ -2,6 +2,7 @@
 #define MERGE_ORT_H
 
 #include "merge-recursive.h"
+#include "hash.h"
 
 struct commit;
 struct tree;

merge-ort.h +15 -1

+
+/*
+ * Provide a list of path -> {struct stage_info*} mappings for
+ * all conflicted files.  Note that each path could appear up to three
+ * times in the list, corresponding to 3 different stage entries.  In short,
+ * this basically provides the info that would be printed by `ls-files -u`.
+ *
+ * result should have been populated by a call to
+ * one of the merge_incore_[non]recursive() functions.
+ *
+ * conflicted_files should be empty before calling this function.
+ */
+void merge_get_conflicted_files(struct merge_result *result,
+				struct string_list *conflicted_files);
+
 /* Do needed cleanup when not calling merge_switch_to_result() */
 void merge_finalize(struct merge_options *opt,
 		    struct merge_result *result);
--

gitgitgadget

From: Elijah Newren <[email protected]>

Callers of git merge-tree --write-tree will often want to know which files had conflicts. While they could potentially attempt to parse the CONFLICT notices printed, those messages are not meant to be machine readable. Provide a simpler mechanism of just printing the files (in the same format as git ls-files with quoting, but restricted to unmerged files) in the output before the free-form messages.

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

---
 Documentation/git-merge-tree.txt |  8 ++++++++
 builtin/merge-tree.c             | 24 ++++++++++++++++++++++--
 t/t4301-merge-tree-real.sh       | 11 +++++++++++
 3 files changed, 41 insertions(+), 2 deletions(-)

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

index fd7a867de60..041a4ac2785 100644

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

 These are discussed individually below.

@@ -69,6 +70,13 @@ This is a tree object that represents what would be checked out in the
 working tree at the end of `git merge`.  If there were conflicts, then
 files within this tree may have embedded conflict markers.
 
+Conflicted file list
+~~~~~~~~~~~~~~~~~~~~
+
+This is a sequence of lines containing a filename on each line, quoted
+as explained for the configuration variable `core.quotePath` (see
+linkgit:git-config[1]).
+
 Informational messages
 ~~~~~~~~~~~~~~~~~~~~~~
 

builtin/merge-tree.c +13 -1

index 560640ad911..d8eeeb3f306 100644
@@ -11,6 +11,9 @@
 #include "blob.h"
 #include "exec-cmd.h"
 #include "merge-blobs.h"
+#include "quote.h"
+
+static int line_termination = '\n';
 
 struct merge_list {
 	struct merge_list *next;

@@ -395,7 +398,8 @@ struct merge_tree_options {
 };
 
 static int real_merge(struct merge_tree_options *o,
-		      const char *branch1, const char *branch2)
+		      const char *branch1, const char *branch2,
+		      const char *prefix)
 {
 	struct commit *parent1, *parent2;
 	struct commit_list *common;

@@ -449,6 +453,22 @@ static int real_merge(struct merge_tree_options *o,
 		o->show_messages = !result.clean;
 
 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
+	if (!result.clean) {
+		struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
+		const char *last = NULL;
+		int i;
+
+		merge_get_conflicted_files(&result, &conflicted_files);
+		for (i = 0; i < conflicted_files.nr; i++) {
+			const char *name = conflicted_files.items[i].string;

builtin/merge-tree.c +1 -1

 		merge_display_update_messages(&opt, &result, stdout);

@@ -502,7 +522,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 
 	/* Do the relevant type of merge */
 	if (o.real)
-		return real_merge(&o, argv[0], argv[1]);
+		return real_merge(&o, argv[0], argv[1], prefix);
 	else
 		return trivial_merge(argv[0], argv[1], argv[2]);
 }

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

index c34f8e6c1ed..43c9950dedb 100755

t/t4301-merge-tree-real.sh +9 -1

@@ -105,4 +107,13 @@ test_expect_success 'test conflict notices and such' '
 	test_cmp expect actual
 '
 
+test_expect_success 'Just the conflicted files without the messages' '
+	test_expect_code 1 git merge-tree --write-tree --no-messages side1 side2 >out &&
+	sed -e "s/[0-9a-f]\{40,\}/HASH/g" out >actual &&
+
+	test_write_lines HASH greeting whatever~side1 >expect &&
+
+	test_cmp expect actual
+'
+
 test_done
--

gitgitgadget

Note1: Depends on en/remerge-diff (but no need to pick it up; it's still RFC).

== Updates Log ==

Updates since v2 (thanks to Christian, Dscho, Ramsay, and René for suggestions and comments on v2):

* Significant changes to output format:
* Flags no longer take a filename for additional output; they write to stdout instead.
* More information included by default when there are conflicts (no need to request it with additional flags, instead flags can be used to suppress it).
* Provide (mode, oid, stage, file) tuples -- i.e. ls-files -u style of information -- when there are conflicts. Add a flag to only list conflicted files if that's preferred.
* Much more thorough manual for git-merge-tree.txt
* Renamed option from --real to --write-tree
* Accept an optional --trivial-merge option to get old style merge-tree behavior
* Allow both --write-tree and --trivial-merge to be omitted since we can deduce which from number of arguments

* Document exit code when the merge cannot be run (so we can distinguish other error cases from conflicts)
* testcase cleanups: test_tick, early skip of test when using recursive backend, variable renames, etc.
* various minor code cleanups
* Add a new --allow-unrelated-histories option (with same meaning as the one used in git merge)
* Rebased on top of en/remerge-diff to avoid a small conflict

Quoted 9 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 3 lines

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.

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

Quoted 5 lines

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

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

Quoted 9 lines

The rest of this manual (other than the

builtin/merge-tree.c +3 -1

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

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

Quoted 10 lines

builtin/merge-tree.c +10 -0

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

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

Quoted 5 lines

builtin/merge-tree.c +4 -0

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

idem

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

Quoted 4 lines

builtin/merge-tree.c +4 -0

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

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

Quoted 2 lines

builtin/merge-tree.c +2 -0

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

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

Quoted 3 lines

builtin/merge-tree.c +3 -0

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

The "else" is not necessary above.

Quoted 1 line

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

t/t4301-merge-tree-real.sh

Quoted 9 lines

builtin/merge-tree.c +2 -0

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

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

new file mode 100755
index 00000000000..e03688515c5

I wonder if it would be better named 't4301-merge-tree-write-tree.sh'...

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

Quoted 4 lines

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

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

... especially given this description.

Hi Elijah,

Quoted 18 lines · Elijah Newren via GitGitGadget

merge-ort.c +2 -1

index f9e35b0f96b..b78dde55ad9 100644
@@ -4236,7 +4236,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 }
 
 void merge_display_update_messages(struct merge_options *opt,
-                                struct merge_result *result)
+                                struct merge_result *result,
+                                FILE *stream)
 {
      struct merge_options_internal *opti = result->priv;
      struct hashmap_iter iter;

@@ -4263,7 +4264,7 @@ void merge_display_update_messages(struct merge_options *opt,
      for (i = 0; i < olist.nr; ++i) {
              struct strbuf *sb = olist.items[i].util;
 

merge-ort.c void merge_display_update_messages(struct merge_options *opt, · line 4267

Quoted 2 lines

merge-ort.c +1 -1

-             printf("%s", sb->buf);
+             fprintf(stream, "%s", sb->buf);

Maybe strbuf_write(sb, stream); instead? Whenever I see a "%s", I feel like it's unnecessary churn...

merge-ort.c void merge_display_update_messages(struct merge_options *opt, · line 4270

Quoted 3 lines

merge-ort.c +0 -0

      }
      string_list_clear(&olist, 0);
 

Missing from this hunk:

        /* Also include needed rename limit adjustment now */
        diff_warn_rename_limit("merge.renamelimit",
                               opti->renames.needed_limit, 0);

This function explicitly writes to stdout, and will need to be adjusted, I think, before we can include an adjustment to this call in this patch.

Unless we override warn_routine() (which is used inside that function), that is. Which is hacky, and we would not have addressed the fflush(stdout) in diff_warn_rename_limit(). So I would much prefer something like this:

-- snip --

diff.c +14 -6

index 861282db1c3..87966602cbd 100644
@@ -6312,17 +6312,25 @@ static const char rename_limit_advice[] =
 N_("you may want to set your %s variable to at least "
    "%d and retry the command.");
 
-void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
+void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
+                           FILE *out)
 {
-       fflush(stdout);
+       const char *fmt = NULL;
+
        if (degraded_cc)
-               warning(_(degrade_cc_to_c_warning));
+               fmt = _(degrade_cc_to_c_warning);
        else if (needed)
-               warning(_(rename_limit_warning));
+               fmt = _(rename_limit_warning);
        else
                return;
        if (0 < needed)
-               warning(_(rename_limit_advice), varname, needed);
+               fmt = _(rename_limit_advice);
+
+       fflush(out);
+       if (out == stdout)
+               warning(fmt, varname, needed);
+       else
+               fprintf(out, fmt, varname, needed);
 }
 
 static void diff_flush_patch_all_file_pairs(struct diff_options *o)

@@ -6754,7 +6762,7 @@ int diff_result_code(struct diff_options *opt, int status)
 
        diff_warn_rename_limit("diff.renameLimit",
                               opt->needed_rename_limit,
-                              opt->degraded_cc_to_c);
+                              opt->degraded_cc_to_c, stdout);
        if (!opt->flags.exit_with_status &&
            !(opt->output_format & DIFF_FORMAT_CHECKDIFF))
                return status;

diff.h +2 -1

index 8ba85c5e605..be4ee68c0a2 100644
@@ -596,7 +596,8 @@ void diffcore_fix_diff_index(void);
 int diff_queue_is_empty(void);
 void diff_flush(struct diff_options*);
 void diff_free(struct diff_options*);
-void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
+void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
+                           FILE *out);
 
 /* diff-raw status letters */
 #define DIFF_STATUS_ADDED              'A'

merge-ort.c +1 -1

index 0342f104836..e6b5a0e7c64 100644
@@ -4264,7 +4264,7 @@ void merge_switch_to_result(struct merge_options *opt,
 
                /* Also include needed rename limit adjustment now */
                diff_warn_rename_limit("merge.renamelimit",
-                                      opti->renames.needed_limit, 0);
+                                      opti->renames.needed_limit, 0, stdout);
 
                trace2_region_leave("merge", "display messages", opt->repo);
        }

merge-recursive.c +0 -0

index d9457797dbb..10b2948678c 100644

The rest of the patch looks good to me.

Hi Elijah,

Quoted 12 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

When running git merge-tree --write-tree, 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.

Quoted 4 lines

Add a --[no-]messages option so that callers can request these messages be included at the end of the output. Include such messages by default when there are conflicts, and omit them by default when the merge is clean.

Makes sense.

Quoted 13 lines

builtin/merge-tree.c +1 -2

index 0c19639594d..560640ad911 100644
@@ -440,22 +441,30 @@ static int real_merge(struct merge_tree_options *o,
              commit_list_insert(j->item, &merge_bases);
 
      merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
-     printf("%s\n", oid_to_hex(&result.tree->object.oid));
+
      if (result.clean < 0)
              die(_("failure to merge"));
-     else if (!result.clean)

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

Quoted 10 lines

builtin/merge-tree.c +9 -1

-             printf(_("Conflicts!\n"));
+
+     if (o->show_messages == -1)
+             o->show_messages = !result.clean;
+
+     printf("%s\n", oid_to_hex(&result.tree->object.oid));
+     if (o->show_messages) {
+             printf("\n");
+             merge_display_update_messages(&opt, &result, stdout);
+     }

Excellent.

Quoted 27 lines

builtin/merge-tree.c +5 -2

      merge_finalize(&opt, &result);
      return !result.clean; /* result.clean < 0 handled above */
 }
 
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
-     struct merge_tree_options o = { 0 };
+     struct merge_tree_options o = { .show_messages = -1 };
      int expected_remaining_argc;
+     int original_argc;
 
      const char * const merge_tree_usage[] = {
-             N_("git merge-tree [--write-tree] <branch1> <branch2>"),
+             N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
              N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
              NULL
      };

@@ -464,6 +473,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
                       N_("do a real merge instead of a trivial merge")),
              OPT_BOOL(0, "trivial-merge", &o.trivial,
                       N_("do a trivial merge only")),
+             OPT_BOOL(0, "messages", &o.show_messages,
+                      N_("also show informational/conflict messages")),
              OPT_END()
      };
 

builtin/merge-tree.c int cmd_merge_tree(int argc, const char **argv, const char *prefix) · line 492

Quoted 10 lines

builtin/merge-tree.c +3 -0

@@ -472,10 +483,13 @@ 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 && o.trivial)
              die(_("--write-tree and --trivial-merge are incompatible"));
+     if (!o.real && original_argc < argc)
+             die(_("--write-tree must be specified if any other options are"));

Hmm. Well. Hmm.