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

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");

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

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

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;

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.

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;

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

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

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.

Quoted 4 lines · Elijah Newren via GitGitGadget

EXIT STATUS
-----------

(quoted diff) +0 -0

@@ -72,7 +102,8 @@ be used as a part of a series of steps such as
 

Documentation/git-merge-tree.txt be used as a part of a series of steps such as · line 106

Quoted 5 lines

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

 However, it does not quite fit into the same category of low-level
 plumbing commands since the possibility of merge conflicts give it a
-much higher chance of the command not succeeding.
+much higher chance of the command not succeeding (and NEWTREE containing
+a bunch of stuff other than just a toplevel tree).

Is this hunk really related to this commit or should it go into a previous commit?

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

Quoted 8 lines

builtin/merge-tree.c +1 -1

@@ -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"));

So this addresses the comment I made in a previous commit related to the fact that if result.clean < 0 we might not have a valid tree that we can print. I think though that it would be better if that was addressed in a previous commit.

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

Quoted 2 lines

builtin/merge-tree.c +0 -2

-       else if (!result.clean)
-               printf(_("Conflicts!\n"));

Ok, so we don't print "Conflicts!\n" now, which makes me wonder if we should have printed it in the first place in previous commits.

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

Quoted 4 lines

builtin/merge-tree.c +2 -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"));

Is this necessary? It looks to me like another thing that would be simplified if we were just adding a new command...

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

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.

I'd rather keep --write-tree neat and optional. What's wrong with allowing

        git merge-tree --no-messages HEAD MERGE_HEAD

?

To be clear, I think we need this instead:

        if (o.trivial && o.show_messages >= 0)
                die(_("--trivial-merge is incompatible with additional options"));

I like the rest of the patch very much!

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.

On Sat, Jan 22, 2022 at 10:56 PM Elijah Newren via GitGitGadget <[email protected]> wrote:

Quoted 2 lines

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

This made me wonder if "-- conflicted" should be part of the ls-files -u command. Maybe "by ls-files -u, which means" would make things a bit clearer.

Quoted 1 line

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 19 lines · Elijah Newren via GitGitGadget

merge-ort.c +19 -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;
+
+               VERIFY_CI(ci);
+
+               for (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {
+                       struct stage_info *si;
+
+                       if (!(ci->filemask & (1ul << i)))
+                               continue;

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+                       si = xmalloc(sizeof(*si));

It's probably a premature optimization, so feel free to ignore, but as MERGE_BASE and MERGE_SIDE2 are constants, and ci->filemask is constant inside the 'for' loop, we could compute before the 'for' loop how many 'struct stage_info' we will need and allocate them all at once before the 'for' loop.

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

Hi Elijah,

Quoted 5 lines · Elijah Newren via 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.

Hmm. Okay.

I am not really a fan of that output where we use a variable number of lines per file. As in: in the regular case, where a file was modified in divergent ways, we get three lines. But if it was deleted in one branch, or if it was created in both branches, we have only two lines.

Frankly, I'd much rather have 3 lines for each and every conflict.

Granted, we currently only have three stages...

...and we can pretty much
guarantee that at least two of these stages are non-empty (otherwise where would be the conflict?).

Meaning: Even if stage 3 is missing from the first conflict and stage 1 is missing from the second conflict, in the output we would see stages 1, 2, 2, 3, i.e. a duplicate stage 2, signifying that we're talking about two different conflicts.

But still. It makes me uneasy to have that much variability in machine-consumable output.

And who knows, maybe if we're ever implementing more sophisticated merge strategies for octopus merges, we might end up with more stages...

In other words...

Quoted 61 lines

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 void merge_display_update_messages(struct merge_options *opt, · line 97

Quoted 8 lines

merge-ort.h +5 -0

@@ -89,6 +90,26 @@ void merge_display_update_messages(struct merge_options *opt,
                                 struct merge_result *result,
                                 FILE *stream);
 
+struct stage_info {
+     struct object_id oid;
+     int mode;
+     int stage;
+};

... I'd rather not tack this onto a string_list but instead have something like:

        struct conflict_info {

                struct {
                        struct object_id oid;
                        int mode;
                        const char *path;
                } stages[3]
        };

where path can be NULL to indicate that this stage is missing.

Quoted 4 lines · Johannes Schindelin

Meaning: Even if stage 3 is missing from the first conflict and stage 1 is missing from the second conflict, in the output we would see stages 1, 2, 2, 3, i.e. a duplicate stage 2, signifying that we're talking about two different conflicts.

I don't understand why you're fixating on the stage here. Why would you want to group all the stage 2s together, count them up, and then determine there are N conflicting files because there are N stage 2's?

Just a heckling from the peanut gallery...

Am 29.01.22 um 07:08 schrieb Elijah Newren:

Quoted 10 lines

Quoted 4 lines · Johannes Schindelin

Meaning: Even if stage 3 is missing from the first conflict and stage 1 is missing from the second conflict, in the output we would see stages 1, 2, 2, 3, i.e. a duplicate stage 2, signifying that we're talking about two different conflicts.

I don't understand why you're fixating on the stage here. Why would you want to group all the stage 2s together, count them up, and then determine there are N conflicting files because there are N stage 2's?

Looks like you are misunderstanding Dscho's point: When you have two conflicts, the first with stages 1 and 2, the second with stages 2 and 3, then the 2s occur lumped together when the 4 lines are printed in a row, and that is the cue to the parser where the new conflict begins. Dscho did not mean that all N 2s of should be listed together.

Quoted 15 lines · Johannes Sixt

Just a heckling from the peanut gallery...

Am 29.01.22 um 07:08 schrieb Elijah Newren:

Quoted 10 lines

Quoted 4 lines · Johannes Schindelin

Meaning: Even if stage 3 is missing from the first conflict and stage 1 is missing from the second conflict, in the output we would see stages 1, 2, 2, 3, i.e. a duplicate stage 2, signifying that we're talking about two different conflicts.

I don't understand why you're fixating on the stage here. Why would you want to group all the stage 2s together, count them up, and then determine there are N conflicting files because there are N stage 2's?

Quoted 5 lines

Looks like you are misunderstanding Dscho's point: When you have two conflicts, the first with stages 1 and 2, the second with stages 2 and 3, then the 2s occur lumped together when the 4 lines are printed in a row, and that is the cue to the parser where the new conflict begins. Dscho did not mean that all N 2s of should be listed together.

Ah, so...I didn't understand his misunderstanding? Using stages as a cue to the parser where the new conflict begins is broken; you should instead check for when the filename listed on a line does not match the filename on the previous line.

In particular, if one conflict has stages 1 and 2, and the next conflict has only stage 3, then looking at stages only might cause you to accidentally lump unrelated conflicts together.

Hi Elijah,

Quoted 22 lines · Elijah Newren

Quoted 15 lines · Johannes Sixt

Just a heckling from the peanut gallery...

Am 29.01.22 um 07:08 schrieb Elijah Newren:

Quoted 10 lines

Quoted 4 lines · Johannes Schindelin

Meaning: Even if stage 3 is missing from the first conflict and stage 1 is missing from the second conflict, in the output we would see stages 1, 2, 2, 3, i.e. a duplicate stage 2, signifying that we're talking about two different conflicts.

I don't understand why you're fixating on the stage here. Why would you want to group all the stage 2s together, count them up, and then determine there are N conflicting files because there are N stage 2's?

Quoted 5 lines

Looks like you are misunderstanding Dscho's point: When you have two conflicts, the first with stages 1 and 2, the second with stages 2 and 3, then the 2s occur lumped together when the 4 lines are printed in a row, and that is the cue to the parser where the new conflict begins. Dscho did not mean that all N 2s of should be listed together.

Quoted 4 lines

Ah, so...I didn't understand his misunderstanding? Using stages as a cue to the parser where the new conflict begins is broken; you should instead check for when the filename listed on a line does not match the filename on the previous line.

But that would break down in case of rename/rename conflicts, right?

Quoted 3 lines

In particular, if one conflict has stages 1 and 2, and the next conflict has only stage 3, then looking at stages only might cause you to accidentally lump unrelated conflicts together.

Precisely. That's why I would love to have a way to deviate from the output of ls-files -u's format, and have a reliable way to indicate stages that belong to the same merge conflict.

Quoted 32 lines · Johannes Schindelin

Quoted 22 lines · Elijah Newren

Quoted 15 lines · Johannes Sixt

Just a heckling from the peanut gallery...

Am 29.01.22 um 07:08 schrieb Elijah Newren:

Quoted 10 lines

Quoted 4 lines · Johannes Schindelin

Meaning: Even if stage 3 is missing from the first conflict and stage 1 is missing from the second conflict, in the output we would see stages 1, 2, 2, 3, i.e. a duplicate stage 2, signifying that we're talking about two different conflicts.

I don't understand why you're fixating on the stage here. Why would you want to group all the stage 2s together, count them up, and then determine there are N conflicting files because there are N stage 2's?

Quoted 5 lines

Looks like you are misunderstanding Dscho's point: When you have two conflicts, the first with stages 1 and 2, the second with stages 2 and 3, then the 2s occur lumped together when the 4 lines are printed in a row, and that is the cue to the parser where the new conflict begins. Dscho did not mean that all N 2s of should be listed together.

Quoted 4 lines

Ah, so...I didn't understand his misunderstanding? Using stages as a cue to the parser where the new conflict begins is broken; you should instead check for when the filename listed on a line does not match the filename on the previous line.

But that would break down in case of rename/rename conflicts, right?

Quoted 7 lines

Quoted 3 lines

In particular, if one conflict has stages 1 and 2, and the next conflict has only stage 3, then looking at stages only might cause you to accidentally lump unrelated conflicts together.

Precisely. That's why I would love to have a way to deviate from the output of ls-files -u's format, and have a reliable way to indicate stages that belong to the same merge conflict.

Ah, attempting to somehow identify and present logical separate conflicts? That could be awesome, but I'm not sure it's technically possible. It certainly isn't with today's merge-ort.

Let me ask some questions first...

If I understand you correctly then in the event of a rename/rename, i.e. foo->bar & foo->baz, then you want foo's, bar's, & baz's stages all listed together. Right? And in some way that you can identify them as related?

If we do so, how do we mark the beginning and the end of what you call "the same merge conflict"? If you say it's always 3 stages (with the possibility of all-zero modes/oids), then what about the rename/rename case above modified so that the side that did foo->baz also added a different 'bar'? That'd be 4 non-zero modes/oids, all of them relevant. Or what if the side that did foo->bar also renamed something else to 'baz', giving us even more non-zero stages for these three paths? Perhaps you consider these different conflicts and want them listed separately -- if so, where does one conflict begin and another start and which stages are parts of which conflict?

If you are attempting to somehow present the stuff that "belongs to the same merge conflict" are you also trying to identify what kind of merge conflict it is? If so, do you want each type of merge conflict listed? For example, let's switch from the example above of logically disjoint paths coming together to result in more than 3 stages, and instead pick an example with a single logical path with less than three stages. And now let's say that path has multiple conflicts associated with it; let's use an example with 3: rename/delete + modify/delete + directory/file (one side renames foo->bar while modifying the contents, the other side deletes foo and adds the directory 'bar/'). In this case, there is a target file 'bar' that has two non-zero modes/oids in the ls-files-u output. If all three types of conflicts need to be listed, does each need to be listed with the two non-zero modes/oids (and perhaps one zero mode/oid), resulting in six listings for 'bar'? Or would the duplication be confusing enough that we instead decide to list some merge conflicts with no stages associated with them?

Thinking about both sets of questions in the last two paragraphs from a higher level -- should we focus on and group the higher order stages by the individual conflicts that happen, or should we group them by the paths that they happen to (which is what ls-files -u happens to do), or should we not bother grouping them and instead duplicate the higher order stages for each logical conflict it is part of?

As an alternative to duplicating higher order stages, do we sometimes decide to "lump" separate conflicts together and treat them as one conflict? If so, what are the rules on how we decide to lump conflicts and when not to? Is there a bright line boundary? And can it be done without sucking in arbitrarily more stages for a single conflict?

Some testcases that might be useful while considering the above
questions: take a look at the "rad", "rrdd", and "mod6" tests of t6422. How many "same merge conflicts" are there for each of those, and what's the boundary between them? And can you give the answer in the form of rules that generically handle all cases, rather than just answering these three specific cases?

I've thought about this problem long and hard before (in part because of some conversations I had with Edward Thompson about libgit2 and

merging at Git Merge 2020). It wasn't at all clear to me that libgit2 had considered anything beyond simple rename cases. The only rules I ever figured out that made sense to me was "group the stages by target filename rather than by logical conflict" (so we get ls -files -u populated) and print a meant-for-human message for each logical conflict (found in the <Informational Messages> section for merge-tree), and make NO attempt to connect stages by conflict type.

I'm sure that's not what you wanted to hear, and maybe doesn't even play nicely with your design. But short of ignoring the edge and corner cases, I don't see how to solve that problem. If you do just want to ignore edge and corner cases, then just ignore the rename/rename case you brought up in the first place and just use ls-files -u-type output as-is within your design. If you don't want to ignore edge cases and want something that works with a specific design that somehow groups conflicted file stages by conflict type, then we're going to have to dig into all these questions above and do some big replumbing within merge-ort.

Hi Elijah,

Quoted 46 lines · Elijah Newren

Quoted 32 lines · Johannes Schindelin

Quoted 22 lines · Elijah Newren

Quoted 15 lines · Johannes Sixt

Just a heckling from the peanut gallery...

Am 29.01.22 um 07:08 schrieb Elijah Newren:

Quoted 10 lines

Quoted 4 lines · Johannes Schindelin

Meaning: Even if stage 3 is missing from the first conflict and stage 1 is missing from the second conflict, in the output we would see stages 1, 2, 2, 3, i.e. a duplicate stage 2, signifying that we're talking about two different conflicts.

I don't understand why you're fixating on the stage here. Why would you want to group all the stage 2s together, count them up, and then determine there are N conflicting files because there are N stage 2's?

Quoted 5 lines

Looks like you are misunderstanding Dscho's point: When you have two conflicts, the first with stages 1 and 2, the second with stages 2 and 3, then the 2s occur lumped together when the 4 lines are printed in a row, and that is the cue to the parser where the new conflict begins. Dscho did not mean that all N 2s of should be listed together.

Quoted 4 lines

Ah, so...I didn't understand his misunderstanding? Using stages as a cue to the parser where the new conflict begins is broken; you should instead check for when the filename listed on a line does not match the filename on the previous line.

But that would break down in case of rename/rename conflicts, right?

Quoted 7 lines

Quoted 3 lines

In particular, if one conflict has stages 1 and 2, and the next conflict has only stage 3, then looking at stages only might cause you to accidentally lump unrelated conflicts together.

Precisely. That's why I would love to have a way to deviate from the output of ls-files -u's format, and have a reliable way to indicate stages that belong to the same merge conflict.

Ah, attempting to somehow identify and present logical separate conflicts? That could be awesome, but I'm not sure it's technically possible. It certainly isn't with today's merge-ort.

Quoted 6 lines

Let me ask some questions first...

If I understand you correctly then in the event of a rename/rename, i.e. foo->bar & foo->baz, then you want foo's, bar's, & baz's stages all listed together. Right? And in some way that you can identify them as related?

Yes, that was kind of my idea ;-)

Quoted 10 lines

If we do so, how do we mark the beginning and the end of what you call "the same merge conflict"? If you say it's always 3 stages (with the possibility of all-zero modes/oids), then what about the rename/rename case above modified so that the side that did foo->baz also added a different 'bar'? That'd be 4 non-zero modes/oids, all of them relevant. Or what if the side that did foo->bar also renamed something else to 'baz', giving us even more non-zero stages for these three paths? Perhaps you consider these different conflicts and want them listed separately -- if so, where does one conflict begin and another start and which stages are parts of which conflict?

Thank you for calling me out on an only half-finished thought.

To be quite honest, I previously did not have much time to think of the non-trivial nature of representing merge conflicts in a web UI when performing merges with rename detection, in particular when performing _recursive_ merges (where, as you point out so correctly, an arbitrary number of rename conflicts can happen _for the same file_).

Of course, this gets even more complicated when we're also thinking about type changes (file -> symlink, file -> directory, and vice versa).

Quoted 41 lines

If you are attempting to somehow present the stuff that "belongs to the same merge conflict" are you also trying to identify what kind of merge conflict it is? If so, do you want each type of merge conflict listed? For example, let's switch from the example above of logically disjoint paths coming together to result in more than 3 stages, and instead pick an example with a single logical path with less than three stages. And now let's say that path has multiple conflicts associated with it; let's use an example with 3: rename/delete + modify/delete + directory/file (one side renames foo->bar while modifying the contents, the other side deletes foo and adds the directory 'bar/'). In this case, there is a target file 'bar' that has two non-zero modes/oids in the ls-files-u output. If all three types of conflicts need to be listed, does each need to be listed with the two non-zero modes/oids (and perhaps one zero mode/oid), resulting in six listings for 'bar'? Or would the duplication be confusing enough that we instead decide to list some merge conflicts with no stages associated with them?

Thinking about both sets of questions in the last two paragraphs from a higher level -- should we focus on and group the higher order stages by the individual conflicts that happen, or should we group them by the paths that they happen to (which is what ls-files -u happens to do), or should we not bother grouping them and instead duplicate the higher order stages for each logical conflict it is part of?

As an alternative to duplicating higher order stages, do we sometimes decide to "lump" separate conflicts together and treat them as one conflict? If so, what are the rules on how we decide to lump conflicts and when not to? Is there a bright line boundary? And can it be done without sucking in arbitrarily more stages for a single conflict?

Some testcases that might be useful while considering the above
questions: take a look at the "rad", "rrdd", and "mod6" tests of t6422. How many "same merge conflicts" are there for each of those, and what's the boundary between them? And can you give the answer in the form of rules that generically handle all cases, rather than just answering these three specific cases?

Quoted 2 lines

I've thought about this problem long and hard before (in part because of some conversations I had with Edward Thompson about libgit2 and

Not a big deal for _me_, but I seem to remember that Ed cared a lot about having no p in their surname ;-)

Quoted 8 lines

merging at Git Merge 2020). It wasn't at all clear to me that libgit2 had considered anything beyond simple rename cases. The only rules I ever figured out that made sense to me was "group the stages by target filename rather than by logical conflict" (so we get ls -files -u populated) and print a meant-for-human message for each logical conflict (found in the <Informational Messages> section for merge-tree), and make NO attempt to connect stages by conflict type.

Quoted 10 lines

I'm sure that's not what you wanted to hear, and maybe doesn't even play nicely with your design. But short of ignoring the edge and corner cases, I don't see how to solve that problem. If you do just want to ignore edge and corner cases, then just ignore the rename/rename case you brought up in the first place and just use ls-files -u-type output as-is within your design. If you don't want to ignore edge cases and want something that works with a specific design that somehow groups conflicted file stages by conflict type, then we're going to have to dig into all these questions above and do some big replumbing within merge-ort.

There is sometimes a big difference between what I want to hear and what I need to hear. Thank you for providing so many details that I needed to hear.

So let's take a step back and look at my goal here, as in: the over-arching goal: to use merge-ort on the server side.

From what you said above, it becomes very clear to me that there is very little chance to resolve such conflicts on the server side.

For example, if a topic branch renames a file differently than the main branch, there is a really good chance that the user tasked with merging the topic branch will have to do a whole lot more than just click a few buttons to perform that task. There might very well be the need to edit files that do not contain merge conflict markers (I like to call those cases "non-semantic merge conflicts"), and almost certainly local testing will be necessary.

So I guess the best we can do in those complicated cases is to give a comprehensive overview of the problems in the web UI, with the note that this merge conflict has to be resolved on the local side.

Which brings me to the next concern: since merge-tree is a low-level tool meant to be called by programs rather than humans, we need to make sure that those messages remain machine-parseable, even if they contain file names.

Concretely: while I am not currently aware of any web UI that allows to resolve simple rename/rename conflicts, it is easily conceivable how to implement such a thing. When that happens, we will need to be able to teach the server-side code to discern between the cases that can be handled in the web UI (trivial merge conflicts, trivial rename/rename conflicts) as compared to scenarios where the conflicts are just too complex.

Here's an excerpt from t4301:

-- snip --
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. -- snap --

This is the complete set of messages provided in the test conflict notices and such test case.

I immediately notice that every line contains at least one file name. Looking at https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1899, it does not seem as if the file names are quoted:

		path_msg(opt, path, 1, _("Auto-merging %s"), path);

(where path is used verbatim in a call to merge_3way() before that, i.e. it must not have been quoted)

I would like to register a wish to ensure that file names with special characters (such as most notably line-feed characters) are quoted in these messages, so that a simple server-side parser can handle messages starting with Auto-merging and with CONFLICT (content): Merge conflict in , and "throw the hands up in the air" if any other message prefix is seen.

Do you think we can switch to sq_quote_buf_pretty() for these messages? For the Auto-merging one, it would be trivial, but I fear that we will have to work a bit on the path_msg() function
(https://github.com/git/git/blob/v2.35.1/merge-ort.c#L630-L649) because it accepts a variable list of arguments without any clue whether the arguments refer to paths or not. (And I would be loathe to switch _all_ callers to do the quoting themselves.)

I see 28 calls to that function, and at least a couple that pass not only a path but also an OID (e.g.
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1611-L1613).

We could of course be sloppy and pass even OIDs through sq_quote_buf_pretty() in path_msg(), knowing that there won't be any special characters in them, but it gets more complicated e.g. in
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1648-L1651, where we pass an strbuf that contains a somewhat free-form commit message.

I guess we could still pass those through sq_quote_buf_pretty(), even if they are not paths, to ensure that there are no special characters in the machine-parseable lines.

What do you think?

Ciao,
Dscho

[...]
Which brings me to the next concern: since merge-tree is a low-level tool meant to be called by programs rather than humans, we need to make sure that those messages remain machine-parseable, even if they contain file names.

Concretely: while I am not currently aware of any web UI that allows to resolve simple rename/rename conflicts, it is easily conceivable how to implement such a thing. When that happens, we will need to be able to teach the server-side code to discern between the cases that can be handled in the web UI (trivial merge conflicts, trivial rename/rename conflicts) as compared to scenarios where the conflicts are just too complex.

Here's an excerpt from t4301:

-- snip --
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. -- snap --

This is the complete set of messages provided in the test conflict notices and such test case.

I immediately notice that every line contains at least one file name. Looking at https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1899, it does not seem as if the file names are quoted:

		path_msg(opt, path, 1, _("Auto-merging %s"), path);

(where path is used verbatim in a call to merge_3way() before that, i.e. it must not have been quoted)

I would like to register a wish to ensure that file names with special characters (such as most notably line-feed characters) are quoted in these messages, so that a simple server-side parser can handle messages starting with Auto-merging and with CONFLICT (content): Merge conflict in , and "throw the hands up in the air" if any other message prefix is seen.

Do you think we can switch to sq_quote_buf_pretty() for these messages? For the Auto-merging one, it would be trivial, but I fear that we will have to work a bit on the path_msg() function
(https://github.com/git/git/blob/v2.35.1/merge-ort.c#L630-L649) because it accepts a variable list of arguments without any clue whether the arguments refer to paths or not. (And I would be loathe to switch _all_ callers to do the quoting themselves.)

I see 28 calls to that function, and at least a couple that pass not only a path but also an OID (e.g.
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1611-L1613).

We could of course be sloppy and pass even OIDs through sq_quote_buf_pretty() in path_msg(), knowing that there won't be any special characters in them, but it gets more complicated e.g. in
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1648-L1651, where we pass an strbuf that contains a somewhat free-form commit message.

I guess we could still pass those through sq_quote_buf_pretty(), even if they are not paths, to ensure that there are no special characters in the machine-parseable lines.

What do you think?

On Mon, Feb 21 2022, Johannes Schindelin wrote:

[I sent out an empty reply to this earlier by mistake, sorry about that]

Quoted 60 lines

[...]
Which brings me to the next concern: since merge-tree is a low-level tool meant to be called by programs rather than humans, we need to make sure that those messages remain machine-parseable, even if they contain file names.

Concretely: while I am not currently aware of any web UI that allows to resolve simple rename/rename conflicts, it is easily conceivable how to implement such a thing. When that happens, we will need to be able to teach the server-side code to discern between the cases that can be handled in the web UI (trivial merge conflicts, trivial rename/rename conflicts) as compared to scenarios where the conflicts are just too complex.

Here's an excerpt from t4301:

-- snip --
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. -- snap --

This is the complete set of messages provided in the test conflict notices and such test case.

I immediately notice that every line contains at least one file name. Looking at https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1899, it does not seem as if the file names are quoted:

              path_msg(opt, path, 1, _("Auto-merging %s"), path);

(where path is used verbatim in a call to merge_3way() before that, i.e. it must not have been quoted)

I would like to register a wish to ensure that file names with special characters (such as most notably line-feed characters) are quoted in these messages, so that a simple server-side parser can handle messages starting with Auto-merging and with CONFLICT (content): Merge conflict in , and "throw the hands up in the air" if any other message prefix is seen.

Do you think we can switch to sq_quote_buf_pretty() for these messages? For the Auto-merging one, it would be trivial, but I fear that we will have to work a bit on the path_msg() function
(https://github.com/git/git/blob/v2.35.1/merge-ort.c#L630-L649) because it accepts a variable list of arguments without any clue whether the arguments refer to paths or not. (And I would be loathe to switch _all_ callers to do the quoting themselves.)

I see 28 calls to that function, and at least a couple that pass not only a path but also an OID (e.g.
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1611-L1613).

We could of course be sloppy and pass even OIDs through sq_quote_buf_pretty() in path_msg(), knowing that there won't be any special characters in them, but it gets more complicated e.g. in
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1648-L1651, where we pass an strbuf that contains a somewhat free-form commit message.

Quoted 5 lines

I guess we could still pass those through sq_quote_buf_pretty(), even if they are not paths, to ensure that there are no special characters in the machine-parseable lines.

What do you think?

That sounds like a rather nasty hack, this is too, but demonstrates that we can pretty easily extract this in a machine-readable format with just a few lines now):

merge-ort.c +12 -1

index 8a5f201d190..a906881f9b3 100644
@@ -633,7 +633,7 @@ static void path_msg(struct merge_options *opt,
                     int omittable_hint, /* skippable under --remerge-diff */
                     const char *fmt, ...)
 {
-       va_list ap;
+       va_list ap, cp;
        struct strbuf *sb, *dest;
        struct strbuf tmp = STRBUF_INIT;
 

@@ -650,7 +650,9 @@ static void path_msg(struct merge_options *opt,
 
        dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb);
 
+       va_copy(cp, ap);
        va_start(ap, fmt);
+
        if (opt->priv->call_depth) {
                strbuf_addchars(dest, ' ', 2);
                strbuf_addstr(dest, "From inner merge:");

@@ -659,6 +661,15 @@ static void path_msg(struct merge_options *opt,
        strbuf_vaddf(dest, fmt, ap);
        va_end(ap);
 
+       va_start(cp, fmt);
+       trace2_region_enter_printf("merge", "conflict/path", opt->repo, "%s", path);
+       trace2_region_leave("merge", "conflict/path", opt->repo);
+       trace2_region_enter_printf("merge", "conflict/fmt", opt->repo, "%s", fmt);
+       trace2_region_leave("merge", "conflict/fmt", opt->repo);
+       trace2_region_enter_printf_va("merge", "conflict/msg", opt->repo, fmt, cp);
+       trace2_region_leave("merge", "conflict/msg", opt->repo);
+       va_end(cp);
+
        if (opt->record_conflict_msgs_as_headers) {
                int i_sb = 0, i_tmp = 0;
 

You can run that with one of the tests added in this series to get the output as JSON, e.g.:

GIT_TRACE2_EVENT=/dev/stderr GIT_TRACE2_EVENT_NESTING=10 ~/g/git/git merge-tree --write-tree --no-messages --name-only --messages side1 side2 2>&1|jq -r .| grep '"msg"'
"msg": "whatever~side1"
"msg": "CONFLICT (file/directory): directory in the way of %s from %s; moving it to %s instead."
"msg": "CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead."
"msg": "whatever~side1"
"msg": "CONFLICT (modify/delete): %s deleted in %s and modified in %s. Version %s of %s left in tree."
"msg": "CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1. Version side1 of whatever~side1 left in tree." "msg": "numbers"
"msg": "Auto-merging %s"
"msg": "Auto-merging numbers"
"msg": "greeting"
"msg": "Auto-merging %s"
"msg": "Auto-merging greeting"
"msg": "greeting"
"msg": "CONFLICT (%s): Merge conflict in %s"
"msg": "CONFLICT (content): Merge conflict in greeting"

A "proper" fix for this doesn't sound too hard, we'd just instrument the path_msg() function to pass along some "message category", see e.g. unpack_plumbing_errors in unpack-trees.c for one example of such a thing, or the "enum fsck_msg_id".

Then we'd just allow you to emit any of the sprintf() format itself, or the expanded version, the path, or an id like "CONFLICT:file/directory" or "auto-merging" etc.

I think that would be particularly useful in conjuction with the --format changes I was proposing for this (and hacked up a WIP patch for). You could just have a similar --format-messages or whatever.

Then you could pick \0\0 as a delimiter for your "main" --format, and "\0" as the delimiter for your --format-messages, and thus be able to parse N-nested \0-delimited content.

Quoted 134 lines · Ævar Arnfjörð Bjarmason

On Mon, Feb 21 2022, Johannes Schindelin wrote:

[I sent out an empty reply to this earlier by mistake, sorry about that]

Quoted 60 lines

[...]
Which brings me to the next concern: since merge-tree is a low-level tool meant to be called by programs rather than humans, we need to make sure that those messages remain machine-parseable, even if they contain file names.

Concretely: while I am not currently aware of any web UI that allows to resolve simple rename/rename conflicts, it is easily conceivable how to implement such a thing. When that happens, we will need to be able to teach the server-side code to discern between the cases that can be handled in the web UI (trivial merge conflicts, trivial rename/rename conflicts) as compared to scenarios where the conflicts are just too complex.

Here's an excerpt from t4301:

-- snip --
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. -- snap --

This is the complete set of messages provided in the test conflict notices and such test case.

I immediately notice that every line contains at least one file name. Looking at https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1899, it does not seem as if the file names are quoted:

              path_msg(opt, path, 1, _("Auto-merging %s"), path);

(where path is used verbatim in a call to merge_3way() before that, i.e. it must not have been quoted)

I would like to register a wish to ensure that file names with special characters (such as most notably line-feed characters) are quoted in these messages, so that a simple server-side parser can handle messages starting with Auto-merging and with CONFLICT (content): Merge conflict in , and "throw the hands up in the air" if any other message prefix is seen.

Do you think we can switch to sq_quote_buf_pretty() for these messages? For the Auto-merging one, it would be trivial, but I fear that we will have to work a bit on the path_msg() function
(https://github.com/git/git/blob/v2.35.1/merge-ort.c#L630-L649) because it accepts a variable list of arguments without any clue whether the arguments refer to paths or not. (And I would be loathe to switch _all_ callers to do the quoting themselves.)

I see 28 calls to that function, and at least a couple that pass not only a path but also an OID (e.g.
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1611-L1613).

We could of course be sloppy and pass even OIDs through sq_quote_buf_pretty() in path_msg(), knowing that there won't be any special characters in them, but it gets more complicated e.g. in
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1648-L1651, where we pass an strbuf that contains a somewhat free-form commit message.

Quoted 5 lines

I guess we could still pass those through sq_quote_buf_pretty(), even if they are not paths, to ensure that there are no special characters in the machine-parseable lines.

What do you think?

That sounds like a rather nasty hack, this is too, but demonstrates that we can pretty easily extract this in a machine-readable format with just a few lines now):

merge-ort.c +12 -1

index 8a5f201d190..a906881f9b3 100644
@@ -633,7 +633,7 @@ static void path_msg(struct merge_options *opt,
                     int omittable_hint, /* skippable under --remerge-diff */
                     const char *fmt, ...)
 {
-       va_list ap;
+       va_list ap, cp;
        struct strbuf *sb, *dest;
        struct strbuf tmp = STRBUF_INIT;
 

@@ -650,7 +650,9 @@ static void path_msg(struct merge_options *opt,
 
        dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb);
 
+       va_copy(cp, ap);
        va_start(ap, fmt);
+
        if (opt->priv->call_depth) {
                strbuf_addchars(dest, ' ', 2);
                strbuf_addstr(dest, "From inner merge:");

@@ -659,6 +661,15 @@ static void path_msg(struct merge_options *opt,
        strbuf_vaddf(dest, fmt, ap);
        va_end(ap);
 
+       va_start(cp, fmt);
+       trace2_region_enter_printf("merge", "conflict/path", opt->repo, "%s", path);
+       trace2_region_leave("merge", "conflict/path", opt->repo);
+       trace2_region_enter_printf("merge", "conflict/fmt", opt->repo, "%s", fmt);
+       trace2_region_leave("merge", "conflict/fmt", opt->repo);
+       trace2_region_enter_printf_va("merge", "conflict/msg", opt->repo, fmt, cp);
+       trace2_region_leave("merge", "conflict/msg", opt->repo);
+       va_end(cp);
+
        if (opt->record_conflict_msgs_as_headers) {
                int i_sb = 0, i_tmp = 0;
 

You can run that with one of the tests added in this series to get the output as JSON, e.g.:

GIT_TRACE2_EVENT=/dev/stderr GIT_TRACE2_EVENT_NESTING=10 ~/g/git/git merge-tree --write-tree --no-messages --name-only --messages side1 side2 2>&1|jq -r .| grep '"msg"'
"msg": "whatever~side1"
"msg": "CONFLICT (file/directory): directory in the way of %s from %s; moving it to %s instead."
"msg": "CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead."
"msg": "whatever~side1"
"msg": "CONFLICT (modify/delete): %s deleted in %s and modified in %s. Version %s of %s left in tree."
"msg": "CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1. Version side1 of whatever~side1 left in tree." "msg": "numbers"
"msg": "Auto-merging %s"
"msg": "Auto-merging numbers"
"msg": "greeting"
"msg": "Auto-merging %s"
"msg": "Auto-merging greeting"
"msg": "greeting"
"msg": "CONFLICT (%s): Merge conflict in %s"
"msg": "CONFLICT (content): Merge conflict in greeting"

Quoted 8 lines

A "proper" fix for this doesn't sound too hard, we'd just instrument the path_msg() function to pass along some "message category", see e.g. unpack_plumbing_errors in unpack-trees.c for one example of such a thing, or the "enum fsck_msg_id".

Then we'd just allow you to emit any of the sprintf() format itself, or the expanded version, the path, or an id like "CONFLICT:file/directory" or "auto-merging" etc.

I don't see how this helps solve the problem Dscho was bringing up at all. Your reference to "the path" means you've missed his whole complaint -- that with more complex conflicts (renames, directory/file conflicts resolved via moving the file out of the way, mode conflicts resolved by moving both files out of the way, etc) there are multiple paths involved and he's trying to determine what those paths are. He's particularly focusing on rename/rename cases where a single path was renamed differently by the two sides of history (which results in a conflict message only being associated with the path from the merge base in order to avoid repeating the same message 2-3 times, but that one message has three distinct paths embedded in the string).

Also, the additional paths is not part of the API to path_msg; it's merely embedded in a string. (And, in case it bears repeating: as mentioned elsewhere, we cannot assume there will only be one path_msg() call per path, and we at least currently can't assume that each path_msg() call is for a separate logical conflict; there might be two for a single "conflict".)

I agree that parsing these meant-for-human-consumption (and not promised to be stable) messages is not a good way to go, but pretending the current API has enough info to answer his questions isn't right either.

Quoted 7 lines

I think that would be particularly useful in conjuction with the --format changes I was proposing for this (and hacked up a WIP patch for). You could just have a similar --format-messages or whatever.

Then you could pick \0\0 as a delimiter for your "main" --format, and "\0" as the delimiter for your --format-messages, and thus be able to parse N-nested \0-delimited content.

To be honest, the --format stuff is sounding a little bit like a solution in search of a problem.

On Tue, Feb 22 2022, Elijah Newren wrote:

Quoted 19 lines

I don't see how this helps solve the problem Dscho was bringing up at all. Your reference to "the path" means you've missed his whole complaint -- that with more complex conflicts (renames, directory/file conflicts resolved via moving the file out of the way, mode conflicts resolved by moving both files out of the way, etc) there are multiple paths involved and he's trying to determine what those paths are. He's particularly focusing on rename/rename cases where a single path was renamed differently by the two sides of history (which results in a conflict message only being associated with the path from the merge base in order to avoid repeating the same message 2-3 times, but that one message has three distinct paths embedded in the string).

Also, the additional paths is not part of the API to path_msg; it's merely embedded in a string. (And, in case it bears repeating: as mentioned elsewhere, we cannot assume there will only be one path_msg() call per path, and we at least currently can't assume that each path_msg() call is for a separate logical conflict; there might be two for a single "conflict".)

Quoted 4 lines

I agree that parsing these meant-for-human-consumption (and not promised to be stable) messages is not a good way to go, but pretending the current API has enough info to answer his questions isn't right either.

The intent here wasn't to present a complete solution, but to reply to the part of Johannes's E-Mail that e.g. mention "and I would be loathe to switch _all_ callers to do the quoting themselves.".

I.e. it's a POC for passing this data further up the stack. The issue you mention with the renaming case could/should be handled by having whatever handles the vargs accept those N arguments, the POC doesn't handle it.

But in any case, needing to convert "28 calls to [path_msg()]" doesn't seem like it's required.

But obviously we wouldn't want to use trace2 as a plumbing layer for message passing, but could format the same data in a similar way, especially in the context of a discussion about filenames with odd characters in them (some of which JSON is inherently incapable of encoding).

Quoted 8 lines

Quoted 7 lines

I think that would be particularly useful in conjuction with the --format changes I was proposing for this (and hacked up a WIP patch for). You could just have a similar --format-messages or whatever.

Then you could pick \0\0 as a delimiter for your "main" --format, and "\0" as the delimiter for your --format-messages, and thus be able to parse N-nested \0-delimited content.

Quoted 2 lines

To be honest, the --format stuff is sounding a little bit like a solution in search of a problem.

Opinions on this obviously differ, and I'm not going to pick this as my particular hill to die on :)

But I do think it's the other way around, in that hardcoded output formats are a problem requiring solutions.

[...] since merge-tree is a low-level tool meant to be called by programs rather than humans, we need to make sure that those messages remain machine-parseable, even if they contain file names.

[...]

Do you think we can switch to sq_quote_buf_pretty() for these messages?

Hi Elijah,

Quoted 4 lines · Johannes Schindelin

[...] since merge-tree is a low-level tool meant to be called by programs rather than humans, we need to make sure that those messages remain machine-parseable, even if they contain file names.

Quoted 3 lines

[...]

Do you think we can switch to sq_quote_buf_pretty() for these messages?

Or maybe much better: use NUL to separate those messages if -z is passed to merge-tree? That would address the issue in one elegant diff.

What do you think?

Quoted 7 lines · Johannes Schindelin

Quoted 4 lines · Johannes Schindelin

[...] since merge-tree is a low-level tool meant to be called by programs rather than humans, we need to make sure that those messages remain machine-parseable, even if they contain file names.

Quoted 8 lines

Quoted 3 lines

[...]

Do you think we can switch to sq_quote_buf_pretty() for these messages?

Or maybe much better: use NUL to separate those messages if -z is passed to merge-tree? That would address the issue in one elegant diff.

What do you think?

Separating the combination of messages for a single target path from the combination of messages for a different target path by a NUL character may make sense. Would we also want the messages for a path to be prepended by the pathname and a NUL character, in this case, to make it easier to determine which path the group of messages are for?

I'm not sure if that does exactly what you are asking, though.

The thing that is stored (in opt->priv->output) is a strbuf per path, not an array of strbufs per path. So, if we have a rename/delete and a rename/add and a mode conflict for the same "path" (A->B on one side, other side deletes A and adds a symlink B, resulting in three messages for path "B" that are all appended into a single strbuf), then we'll have a single "message" which has three newlines. We can add a NUL character at the end of that, but not between the messages without restructuring things a bit.

There's also at least one example, with submodules, where there are two path_msg() calls for the same individual conflict in order to split conflict info from resolution advice, and those really shouldn't be thought of as messages for different conflicts. (I'm starting to wonder if the resolution advice should just be tossed; I kept it because merge-recursive had it, but it might not make sense with merge-ort being used by server side merges. But even if we toss that one, I'm not sure I want to commit to one path_msg() call per "logical conflict".)

But...maybe this would be good enough for some kind of use you have? Because if you only want to care about "simple" cases, you could potentially define those as ones with only one newline in them.

Hi Elijah,

Quoted 4 lines · Elijah Newren

Quoted 2 lines · Johannes Schindelin

On Sat, 29 Jan 2022, Elijah Newren wrote:

Quoted 2 lines

I've thought about this problem long and hard before (in part because of some conversations I had with Edward Thompson about libgit2 and

Not a big deal for _me_, but I seem to remember that Ed cared a lot about having no p in their surname ;-)

Quoted 8 lines

merging at Git Merge 2020). It wasn't at all clear to me that libgit2 had considered anything beyond simple rename cases. The only rules I ever figured out that made sense to me was "group the stages by target filename rather than by logical conflict" (so we get ls -files -u populated) and print a meant-for-human message for each logical conflict (found in the <Informational Messages> section for merge-tree), and make NO attempt to connect stages by conflict type.

Quoted 10 lines

I'm sure that's not what you wanted to hear, and maybe doesn't even play nicely with your design. But short of ignoring the edge and corner cases, I don't see how to solve that problem. If you do just want to ignore edge and corner cases, then just ignore the rename/rename case you brought up in the first place and just use ls-files -u-type output as-is within your design. If you don't want to ignore edge cases and want something that works with a specific design that somehow groups conflicted file stages by conflict type, then we're going to have to dig into all these questions above and do some big replumbing within merge-ort.

There is sometimes a big difference between what I want to hear and what I need to hear. Thank you for providing so many details that I needed to hear.

So let's take a step back and look at my goal here, as in: the over-arching goal: to use merge-ort on the server side.

From what you said above, it becomes very clear to me that there is very little chance to resolve such conflicts on the server side.

For example, if a topic branch renames a file differently than the main branch, there is a really good chance that the user tasked with merging the topic branch will have to do a whole lot more than just click a few buttons to perform that task. There might very well be the need to edit files that do not contain merge conflict markers (I like to call those cases "non-semantic merge conflicts"), and almost certainly local testing will be necessary.

So I guess the best we can do in those complicated cases is to give a comprehensive overview of the problems in the web UI, with the note that this merge conflict has to be resolved on the local side.

Which brings me to the next concern: since merge-tree is a low-level tool meant to be called by programs rather than humans, we need to make sure that those messages remain machine-parseable, even if they contain file names.

Concretely: while I am not currently aware of any web UI that allows to resolve simple rename/rename conflicts, it is easily conceivable how to implement such a thing. When that happens, we will need to be able to teach the server-side code to discern between the cases that can be handled in the web UI (trivial merge conflicts, trivial rename/rename conflicts) as compared to scenarios where the conflicts are just too complex.

Here's an excerpt from t4301:

-- snip --
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. -- snap --

This is the complete set of messages provided in the test conflict notices and such test case.

I immediately notice that every line contains at least one file name. Looking at https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1899, it does not seem as if the file names are quoted:

                path_msg(opt, path, 1, _("Auto-merging %s"), path);

(where path is used verbatim in a call to merge_3way() before that, i.e. it must not have been quoted)

I would like to register a wish to ensure that file names with special characters (such as most notably line-feed characters) are quoted in these messages, so that a simple server-side parser can handle messages starting with Auto-merging and with CONFLICT (content): Merge conflict in , and "throw the hands up in the air" if any other message prefix is seen.

Do you think we can switch to sq_quote_buf_pretty() for these messages? For the Auto-merging one, it would be trivial, but I fear that we will have to work a bit on the path_msg() function
(https://github.com/git/git/blob/v2.35.1/merge-ort.c#L630-L649) because it accepts a variable list of arguments without any clue whether the arguments refer to paths or not. (And I would be loathe to switch _all_ callers to do the quoting themselves.)

I see 28 calls to that function, and at least a couple that pass not only a path but also an OID (e.g.
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1611-L1613).

We could of course be sloppy and pass even OIDs through sq_quote_buf_pretty() in path_msg(), knowing that there won't be any special characters in them, but it gets more complicated e.g. in
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1648-L1651, where we pass an strbuf that contains a somewhat free-form commit message.

I guess we could still pass those through sq_quote_buf_pretty(), even if they are not paths, to ensure that there are no special characters in the machine-parseable lines.

What do you think?

Sidenote: Do you lump in binary merge conflicts with "non-semantic merge conflicts"? You would by your definition, but I'm not sure it matches.

I tend to call things either content-based conflicts or path-based conflicts, where content-based usually means textual-based but also includes merges of binaries.

Quoted 8 lines · Johannes Schindelin

Concretely: while I am not currently aware of any web UI that allows to resolve simple rename/rename conflicts, it is easily conceivable how to implement such a thing. When that happens, we will need to be able to teach the server-side code to discern between the cases that can be handled in the web UI (trivial merge conflicts, trivial rename/rename conflicts) as compared to scenarios where the conflicts are just too complex.

Um, I'm really worried about attempting to make the conflict notices machine parseable. I don't like that idea at all, and I even tried to rule that out already with my wording:
"""
In all cases, the
<Informational messages> section has the necessary info, though it is not designed to be machine parseable.
"""
though maybe I should have been even more explicit. The restrictions that those messages be stable is too rigid, I think. I also think they're a poor way to communicate information to a higher level tool. I would much rather us add some kind of additional return data structures from merge ort and use them if we want extra info.

Quoted 50 lines

Here's an excerpt from t4301:

-- snip --
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. -- snap --

This is the complete set of messages provided in the test conflict notices and such test case.

I immediately notice that every line contains at least one file name. Looking at https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1899, it does not seem as if the file names are quoted:

                path_msg(opt, path, 1, _("Auto-merging %s"), path);

(where path is used verbatim in a call to merge_3way() before that, i.e. it must not have been quoted)

I would like to register a wish to ensure that file names with special characters (such as most notably line-feed characters) are quoted in these messages, so that a simple server-side parser can handle messages starting with Auto-merging and with CONFLICT (content): Merge conflict in , and "throw the hands up in the air" if any other message prefix is seen.

Do you think we can switch to sq_quote_buf_pretty() for these messages? For the Auto-merging one, it would be trivial, but I fear that we will have to work a bit on the path_msg() function
(https://github.com/git/git/blob/v2.35.1/merge-ort.c#L630-L649) because it accepts a variable list of arguments without any clue whether the arguments refer to paths or not. (And I would be loathe to switch _all_ callers to do the quoting themselves.)

I see 28 calls to that function, and at least a couple that pass not only a path but also an OID (e.g.
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1611-L1613).

We could of course be sloppy and pass even OIDs through sq_quote_buf_pretty() in path_msg(), knowing that there won't be any special characters in them, but it gets more complicated e.g. in
https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1648-L1651, where we pass an strbuf that contains a somewhat free-form commit message.

I guess we could still pass those through sq_quote_buf_pretty(), even if they are not paths, to ensure that there are no special characters in the machine-parseable lines.

Quoted 1 line

What do you think?

Switching to single quoting paths as a matter of style might make sense, but only if we go through and change every caller to do so so that we can make sure it applies to all paths. And only paths and not OIDs.

But I'm going to reserve the right in merge-ort to modify, add, or delete any of those messages passed to path_msg(), which might wreak havoc on your attempts to parse those strings. I think they're a bad form for communicating information to a script or program, and trying to transform them into such risks making them suboptimal at communicating info to humans. These messages should optimize the latter, and if we want something for the former, it should probably be a new independent bit of info.

Quoted 3 lines

I tend to call things either content-based conflicts or path-based conflicts, where content-based usually means textual-based but also includes merges of binaries.

I like "content-based conflicts".

Quoted 4 lines

Switching to single quoting paths as a matter of style might make sense, but only if we go through and change every caller to do so so that we can make sure it applies to all paths. And only paths and not OIDs.

Yes, that sounds unappealing.

Quoted 8 lines

But I'm going to reserve the right in merge-ort to modify, add, or delete any of those messages passed to path_msg(), which might wreak havoc on your attempts to parse those strings. I think they're a bad form for communicating information to a script or program, and trying to transform them into such risks making them suboptimal at communicating info to humans. These messages should optimize the latter, and if we want something for the former, it should probably be a new independent bit of info.

Makes sense.

Hi Elijah,

Quoted 7 lines · Elijah Newren

Sidenote: Do you lump in binary merge conflicts with "non-semantic merge conflicts"? You would by your definition, but I'm not sure it matches.

I tend to call things either content-based conflicts or path-based conflicts, where content-based usually means textual-based but also includes merges of binaries.

I like "content-based conflicts".

And no, I had not even thought about binary merge conflicts yet...

Quoted 11 lines

Quoted 8 lines · Johannes Schindelin

Concretely: while I am not currently aware of any web UI that allows to resolve simple rename/rename conflicts, it is easily conceivable how to implement such a thing. When that happens, we will need to be able to teach the server-side code to discern between the cases that can be handled in the web UI (trivial merge conflicts, trivial rename/rename conflicts) as compared to scenarios where the conflicts are just too complex.

Quoted 13 lines

Um, I'm really worried about attempting to make the conflict notices machine parseable. I don't like that idea at all, and I even tried to rule that out already with my wording:
"""
In all cases, the
<Informational messages> section has the necessary info, though it is not designed to be machine parseable.
"""
though maybe I should have been even more explicit. The restrictions that those messages be stable is too rigid, I think. I also think they're a poor way to communicate information to a higher level tool. I would much rather us add some kind of additional return data structures from merge ort and use them if we want extra info.

Okay.

I thought that we could keep the CONFLICT (<type>) constant enough to serve as such a machine-parseable thing.

And then presenting
<path>NUL<message>NUL could have served my use case well...