git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index)
@ 2021-12-31  5:03 Elijah Newren via GitGitGadget
  2021-12-31  5:03 ` [PATCH 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
                   ` (8 more replies)
  0 siblings, 9 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-12-31  5:03 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Taylor Blau, Elijah Newren

(NOTE for Junio: This series has a minor conflict with en/remerge-diff --
this series moves a code block into a new function, but en/remerge-diff adds
a BUG() message to that code block. But this series is just RFC, so you may
want to wait to pick it up.)

NOTE2: A preliminary version of this series was discussed here:
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110211147490.56@tvgsbejvaqbjf.bet/

This series introduces a new option to git-merge-tree: --real (best name I
could come up with). This new option is designed to allow a server-side
"real" merge (or allow folks client-side to do merges with branches they
don't even have checked out). Real merges differ from trivial merges in that
they handle:

 * three way content merges
 * recursive ancestor consolidation
 * renames
 * proper directory/file conflict handling
 * etc.

The reason this is different from merge is that merge-tree does NOT:

 * Read/write/update any working tree (and assumes there probably isn't one)
 * Read/write/update any index (and assumes there probably isn't one)
 * Create a commit object
 * Update any refs

This series attempts to guess what kind of output would be wanted, basically
choosing:

 * clean merge or conflict signalled via exit status
 * stdout consists solely of printing the hash of the resulting tree (though
   that tree may include files that have conflict markers)
 * new optional --messages flag for specifying a file where informational
   messages (e.g. conflict notices and files involved in three-way-content
   merges) can be written; by default, this output is simply discarded
 * new optional --conflicted-list flag for specifying a file where the names
   of conflicted-files can be written in a NUL-character-separated list

This design means it's basically just a low-level tool that other scripts
would use and do additional work with. Perhaps something like this:

   NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
   test $? -eq 0 || die "There were conflicts..."
   NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 $BRANCH2)
   git update-ref $BRANCH1 $NEWCOMMIT


Elijah Newren (8):
  merge-tree: rename merge_trees() to trivial_merge_trees()
  merge-tree: move logic for existing merge into new function
  merge-tree: add option parsing and initial shell for real merge
    function
  merge-tree: implement real merges
  merge-ort: split out a separate display_update_messages() function
  merge-ort: allow update messages to be written to different file
    stream
  merge-tree: support saving merge messages to a separate file
  merge-tree: provide an easy way to access which files have conflicts

 Documentation/git-merge-tree.txt |  32 +++++--
 builtin/merge-tree.c             | 152 ++++++++++++++++++++++++++++---
 git.c                            |   2 +-
 merge-ort.c                      |  85 ++++++++++-------
 merge-ort.h                      |  12 +++
 t/t4301-merge-tree-real.sh       | 108 ++++++++++++++++++++++
 6 files changed, 333 insertions(+), 58 deletions(-)
 create mode 100755 t/t4301-merge-tree-real.sh


base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1114%2Fnewren%2Fmerge-into-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1114/newren/merge-into-v1
Pull-Request: https://github.com/git/git/pull/1114
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 1/8] merge-tree: rename merge_trees() to trivial_merge_trees()
  2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
@ 2021-12-31  5:03 ` Elijah Newren via GitGitGadget
  2021-12-31  5:03 ` [PATCH 2/8] merge-tree: move logic for existing merge into new function Elijah Newren via GitGitGadget
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-12-31  5:03 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Taylor Blau, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

merge-recursive.h defined its own merge_trees() function, different than
the one found in builtin/merge-tree.c.  That was okay in the past, but
we want merge-tree to be able to use the merge-ort functions, which will
end up including merge-recursive.h.  Rename the function found in
builtin/merge-tree.c to avoid the conflict.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/merge-tree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 5dc94d6f880..06f9eee9f78 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -28,7 +28,7 @@ static void add_merge_entry(struct merge_list *entry)
 	merge_result_end = &entry->next;
 }
 
-static void merge_trees(struct tree_desc t[3], const char *base);
+static void trivial_merge_trees(struct tree_desc t[3], const char *base);
 
 static const char *explanation(struct merge_list *entry)
 {
@@ -225,7 +225,7 @@ static void unresolved_directory(const struct traverse_info *info,
 	buf2 = fill_tree_descriptor(r, t + 2, ENTRY_OID(n + 2));
 #undef ENTRY_OID
 
-	merge_trees(t, newbase);
+	trivial_merge_trees(t, newbase);
 
 	free(buf0);
 	free(buf1);
@@ -342,7 +342,7 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s
 	return mask;
 }
 
-static void merge_trees(struct tree_desc t[3], const char *base)
+static void trivial_merge_trees(struct tree_desc t[3], const char *base)
 {
 	struct traverse_info info;
 
@@ -378,7 +378,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 	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]);
-	merge_trees(t, "");
+	trivial_merge_trees(t, "");
 	free(buf1);
 	free(buf2);
 	free(buf3);
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 2/8] merge-tree: move logic for existing merge into new function
  2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
  2021-12-31  5:03 ` [PATCH 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
@ 2021-12-31  5:03 ` Elijah Newren via GitGitGadget
  2022-01-01 20:11   ` Johannes Altmanninger
  2021-12-31  5:03 ` [PATCH 3/8] merge-tree: add option parsing and initial shell for real merge function Elijah Newren via GitGitGadget
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-12-31  5:03 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Taylor Blau, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/merge-tree.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 06f9eee9f78..9fe5b99f623 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -366,15 +366,11 @@ static void *get_tree_descriptor(struct repository *r,
 	return buf;
 }
 
-int cmd_merge_tree(int argc, const char **argv, const char *prefix)
-{
+static int trivial_merge(int argc, const char **argv) {
 	struct repository *r = the_repository;
 	struct tree_desc t[3];
 	void *buf1, *buf2, *buf3;
 
-	if (argc != 4)
-		usage(merge_tree_usage);
-
 	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]);
@@ -386,3 +382,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 	show_result();
 	return 0;
 }
+
+int cmd_merge_tree(int argc, const char **argv, const char *prefix)
+{
+	if (argc != 4)
+		usage(merge_tree_usage);
+	return trivial_merge(argc, argv);
+}
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 3/8] merge-tree: add option parsing and initial shell for real merge function
  2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
  2021-12-31  5:03 ` [PATCH 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
  2021-12-31  5:03 ` [PATCH 2/8] merge-tree: move logic for existing merge into new function Elijah Newren via GitGitGadget
@ 2021-12-31  5:03 ` Elijah Newren via GitGitGadget
  2021-12-31  5:04 ` [PATCH 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-12-31  5:03 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Taylor Blau, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

Let merge-tree accept a `--real` parameter for choosing real merges
instead of trivial merges.  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 <newren@gmail.com>
---
 builtin/merge-tree.c | 56 +++++++++++++++++++++++++++++++++++++-------
 git.c                |  2 +-
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 9fe5b99f623..f04b1eaad0a 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -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,14 +365,16 @@ 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);
@@ -383,9 +384,46 @@ static int trivial_merge(int argc, const char **argv) {
 	return 0;
 }
 
+struct merge_tree_options {
+	int real;
+};
+
+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 --real <branch1> <branch2>"),
+		N_("git merge-tree <base-tree> <branch1> <branch2>"),
+		NULL
+	};
+	struct option mt_options[] = {
+		OPT_BOOL(0, "real", &o.real,
+			 N_("do a real merge instead of a trivial merge")),
+		OPT_END()
+	};
+
+	/* Check for a request for basic help */
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(merge_tree_usage, mt_options);
+
+	/* Parse arguments */
+	argc = parse_options(argc, argv, prefix, mt_options,
+			     merge_tree_usage, 0);
+	expected_remaining_argc = (o.real ? 2 : 3);
+	if (argc != expected_remaining_argc)
+		usage_with_options(merge_tree_usage, mt_options);
+
+	/* 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]);
 }
diff --git a/git.c b/git.c
index 7edafd8ecff..0124c053878 100644
--- a/git.c
+++ b/git.c
@@ -561,7 +561,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 },
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 4/8] merge-tree: implement real merges
  2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                   ` (2 preceding siblings ...)
  2021-12-31  5:03 ` [PATCH 3/8] merge-tree: add option parsing and initial shell for real merge function Elijah Newren via GitGitGadget
@ 2021-12-31  5:04 ` Elijah Newren via GitGitGadget
  2022-01-01 20:08   ` Johannes Altmanninger
  2022-01-03 12:23   ` Fabian Stelzer
  2021-12-31  5:04 ` [PATCH 5/8] merge-ort: split out a separate display_update_messages() function Elijah Newren via GitGitGadget
                   ` (4 subsequent siblings)
  8 siblings, 2 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-12-31  5:04 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Taylor Blau, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

This adds the ability to perform real merges rather than just trivial
merges (meaning handling three way content merges, recursive ancestor
consolidation, renames, proper directory/file conflict handling, and so
forth).  However, unlike `git merge`, the working tree and index are
left alone and no branch is updated.

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

This output is mean to be used by some higher level script, perhaps in a
sequence of steps like this:

   NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
   test $? -eq 0 || die "There were conflicts..."
   NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 $BRANCH2)
   git update-ref $BRANCH1 $NEWCOMMIT

Note that higher level scripts may also want to access the
conflict/warning messages normally output during a merge, or have quick
access to a list of files with conflicts.  That is not available in this
preliminary implementation, but subsequent commits will add that
ability.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-merge-tree.txt | 28 +++++++----
 builtin/merge-tree.c             | 55 +++++++++++++++++++++-
 t/t4301-merge-tree-real.sh       | 81 ++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+), 11 deletions(-)
 create mode 100755 t/t4301-merge-tree-real.sh

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index 58731c19422..5823938937f 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -3,26 +3,34 @@ git-merge-tree(1)
 
 NAME
 ----
-git-merge-tree - Show three-way merge without touching index
+git-merge-tree - Perform merge without touching index or working tree
 
 
 SYNOPSIS
 --------
 [verse]
+'git merge-tree' --real <branch1> <branch2>
 'git merge-tree' <base-tree> <branch1> <branch2>
 
 DESCRIPTION
 -----------
-Reads three tree-ish, and output trivial merge results and
-conflicting stages to the standard output.  This is similar to
-what three-way 'git read-tree -m' does, but instead of storing the
-results in the index, the command outputs the entries to the
-standard output.
+Performs a merge, but does not make any new commits and does not read
+from or write to either the working tree or index.
 
-This is meant to be used by higher level scripts to compute
-merge results outside of the index, and stuff the results back into the
-index.  For this reason, the output from the command omits
-entries that match the <branch1> tree.
+The first form will merge the two branches, doing a full recursive
+merge with rename detection.  If the merge is clean, the exit status
+will be `0`, and if the merge has conflicts, the exit status will be
+`1`.  The output will consist solely of the resulting toplevel tree
+(which may have files including conflict markers).
+
+The second form is meant for backward compatibility and will only do a
+trival merge.  It reads three tree-ish, and outputs trivial merge
+results and conflicting stages to the standard output in a semi-diff
+format.  Since this was designed for higher level scripts to consume
+and merge the results back into the index, it omits entries that match
+<branch1>.  The result of this second form is is similar to what
+three-way 'git read-tree -m' does, but instead of storing the results
+in the index, the command outputs the entries to the standard output.
 
 GIT
 ---
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index f04b1eaad0a..c5757bed5bb 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -2,6 +2,9 @@
 #include "builtin.h"
 #include "tree-walk.h"
 #include "xdiff-interface.h"
+#include "help.h"
+#include "commit-reach.h"
+#include "merge-ort.h"
 #include "object-store.h"
 #include "parse-options.h"
 #include "repository.h"
@@ -391,7 +394,57 @@ struct merge_tree_options {
 static int real_merge(struct merge_tree_options *o,
 		      const char *branch1, const char *branch2)
 {
-	die(_("real merges are not yet implemented"));
+	struct commit *parent1, *parent2;
+	struct commit_list *common;
+	struct commit_list *merge_bases = NULL;
+	struct commit_list *j;
+	struct merge_options opt;
+	struct merge_result result = { 0 };
+
+	parent1 = get_merge_parent(branch1);
+	if (!parent1)
+		help_unknown_ref(branch1, "merge",
+				 _("not something we can merge"));
+
+	parent2 = get_merge_parent(branch2);
+	if (!parent2)
+		help_unknown_ref(branch2, "merge",
+				 _("not something we can merge"));
+
+	init_merge_options(&opt, the_repository);
+	/*
+	 * TODO: Support subtree and other -X options?
+	if (use_strategies_nr == 1 &&
+	    !strcmp(use_strategies[0]->name, "subtree"))
+		opt.subtree_shift = "";
+	for (x = 0; x < xopts_nr; x++)
+		if (parse_merge_opt(&opt, xopts[x]))
+			die(_("Unknown strategy option: -X%s"), xopts[x]);
+	*/
+
+	opt.show_rename_progress = 0;
+
+	opt.branch1 = merge_remote_util(parent1)->name; /* or just branch1? */
+	opt.branch2 = merge_remote_util(parent2)->name; /* or just branch2? */
+
+	/*
+	 * Get the merge bases, in reverse order; see comment above
+	 * merge_incore_recursive in merge-ort.h
+	 */
+	common = get_merge_bases(parent1, parent2);
+	for (j = common; j; j = j->next)
+		commit_list_insert(j->item, &merge_bases);
+
+	/*
+	 * TODO: notify if merging unrelated histories?
+	if (!common)
+		fprintf(stderr, _("merging unrelated histories"));
+	 */
+
+	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+	printf("%s\n", oid_to_hex(&result.tree->object.oid));
+	merge_switch_to_result(&opt, NULL, &result, 0, 0);
+	return result.clean ? 0 : 1;
 }
 
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
new file mode 100755
index 00000000000..9fb617ccc7f
--- /dev/null
+++ b/t/t4301-merge-tree-real.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+
+test_description='git merge-tree --real'
+
+. ./test-lib.sh
+
+# This test is ort-specific
+GIT_TEST_MERGE_ALGORITHM=ort
+export GIT_TEST_MERGE_ALGORITHM
+
+test_expect_success setup '
+	test_write_lines 1 2 3 4 5 >numbers &&
+	echo hello >greeting &&
+	echo foo >whatever &&
+	git add numbers greeting whatever &&
+	git commit -m initial &&
+
+	git branch side1 &&
+	git branch side2 &&
+
+	git checkout side1 &&
+	test_write_lines 1 2 3 4 5 6 >numbers &&
+	echo hi >greeting &&
+	echo bar >whatever &&
+	git add numbers greeting whatever &&
+	git commit -m rename-and-modify &&
+
+	git checkout side2 &&
+	test_write_lines 0 1 2 3 4 5 >numbers &&
+	echo yo >greeting &&
+	git rm whatever &&
+	mkdir whatever &&
+	>whatever/empty &&
+	git add numbers greeting whatever/empty &&
+	git commit -m remove-and-rename
+'
+
+test_expect_success 'Content merge and a few conflicts' '
+	git checkout side1^0 &&
+	test_must_fail git merge side2 &&
+	cp .git/AUTO_MERGE EXPECT &&
+	E_TREE=$(cat EXPECT) &&
+
+	git reset --hard &&
+	test_must_fail git merge-tree --real side1 side2 >RESULT &&
+	R_TREE=$(cat RESULT) &&
+
+	# Due to differences of e.g. "HEAD" vs "side1", the results will not
+	# exactly match.  Dig into individual files.
+
+	# Numbers should have three-way merged cleanly
+	test_write_lines 0 1 2 3 4 5 6 >expect &&
+	git show ${R_TREE}:numbers >actual &&
+	test_cmp expect actual &&
+
+	# whatever and whatever~<branch> should have same HASHES
+	git rev-parse ${E_TREE}:whatever ${E_TREE}:whatever~HEAD >expect &&
+	git rev-parse ${R_TREE}:whatever ${R_TREE}:whatever~side1 >actual &&
+	test_cmp expect actual &&
+
+	# greeting should have a merge conflict
+	git show ${E_TREE}:greeting >tmp &&
+	cat tmp | sed -e s/HEAD/side1/ >expect &&
+	git show ${R_TREE}:greeting >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'Barf on misspelled option' '
+	# Mis-spell with single "s" instead of double "s"
+	test_expect_code 129 git merge-tree --real --mesages FOOBAR side1 side2 2>expect &&
+
+	grep "error: unknown option.*mesages" expect
+'
+
+test_expect_success 'Barf on too many arguments' '
+	test_expect_code 129 git merge-tree --real side1 side2 side3 2>expect &&
+
+	grep "^usage: git merge-tree" expect
+'
+
+test_done
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 5/8] merge-ort: split out a separate display_update_messages() function
  2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                   ` (3 preceding siblings ...)
  2021-12-31  5:04 ` [PATCH 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
@ 2021-12-31  5:04 ` Elijah Newren via GitGitGadget
  2022-01-03 12:15   ` Fabian Stelzer
  2021-12-31  5:04 ` [PATCH 6/8] merge-ort: allow update messages to be written to different file stream Elijah Newren via GitGitGadget
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-12-31  5:04 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Taylor Blau, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 69 ++++++++++++++++++++++++++++-------------------------
 merge-ort.h |  8 +++++++
 2 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 0342f104836..6237e2fb7fe 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4197,6 +4197,42 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 	return errs;
 }
 
+void merge_display_update_messages(struct merge_options *opt,
+				   struct merge_result *result)
+{
+	struct merge_options_internal *opti = result->priv;
+	struct hashmap_iter iter;
+	struct strmap_entry *e;
+	struct string_list olist = STRING_LIST_INIT_NODUP;
+	int i;
+
+	trace2_region_enter("merge", "display messages", opt->repo);
+
+	/* Hack to pre-allocate olist to the desired size */
+	ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
+		   olist.alloc);
+
+	/* Put every entry from output into olist, then sort */
+	strmap_for_each_entry(&opti->output, &iter, e) {
+		string_list_append(&olist, e->key)->util = e->value;
+	}
+	string_list_sort(&olist);
+
+	/* Iterate over the items, printing them */
+	for (i = 0; i < olist.nr; ++i) {
+		struct strbuf *sb = olist.items[i].util;
+
+		printf("%s", sb->buf);
+	}
+	string_list_clear(&olist, 0);
+
+	/* Also include needed rename limit adjustment now */
+	diff_warn_rename_limit("merge.renamelimit",
+			       opti->renames.needed_limit, 0);
+
+	trace2_region_leave("merge", "display messages", opt->repo);
+}
+
 void merge_switch_to_result(struct merge_options *opt,
 			    struct tree *head,
 			    struct merge_result *result,
@@ -4235,39 +4271,6 @@ void merge_switch_to_result(struct merge_options *opt,
 		trace2_region_leave("merge", "write_auto_merge", opt->repo);
 	}
 
-	if (display_update_msgs) {
-		struct merge_options_internal *opti = result->priv;
-		struct hashmap_iter iter;
-		struct strmap_entry *e;
-		struct string_list olist = STRING_LIST_INIT_NODUP;
-		int i;
-
-		trace2_region_enter("merge", "display messages", opt->repo);
-
-		/* Hack to pre-allocate olist to the desired size */
-		ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
-			   olist.alloc);
-
-		/* Put every entry from output into olist, then sort */
-		strmap_for_each_entry(&opti->output, &iter, e) {
-			string_list_append(&olist, e->key)->util = e->value;
-		}
-		string_list_sort(&olist);
-
-		/* Iterate over the items, printing them */
-		for (i = 0; i < olist.nr; ++i) {
-			struct strbuf *sb = olist.items[i].util;
-
-			printf("%s", sb->buf);
-		}
-		string_list_clear(&olist, 0);
-
-		/* Also include needed rename limit adjustment now */
-		diff_warn_rename_limit("merge.renamelimit",
-				       opti->renames.needed_limit, 0);
-
-		trace2_region_leave("merge", "display messages", opt->repo);
-	}
 
 	merge_finalize(opt, result);
 }
diff --git a/merge-ort.h b/merge-ort.h
index c011864ffeb..1b93555a60b 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -70,6 +70,14 @@ void merge_switch_to_result(struct merge_options *opt,
 			    int update_worktree_and_index,
 			    int display_update_msgs);
 
+/*
+ * Display messages about conflicts and which files were 3-way merged.
+ * Automatically called by merge_switch_to_result() with stream == stdout,
+ * so only call this when bypassing merge_switch_to_result().
+ */
+void merge_display_update_messages(struct merge_options *opt,
+				   struct merge_result *result);
+
 /* Do needed cleanup when not calling merge_switch_to_result() */
 void merge_finalize(struct merge_options *opt,
 		    struct merge_result *result);
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 6/8] merge-ort: allow update messages to be written to different file stream
  2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                   ` (4 preceding siblings ...)
  2021-12-31  5:04 ` [PATCH 5/8] merge-ort: split out a separate display_update_messages() function Elijah Newren via GitGitGadget
@ 2021-12-31  5:04 ` Elijah Newren via GitGitGadget
  2022-01-01 20:08   ` Johannes Altmanninger
  2021-12-31  5:04 ` [PATCH 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-12-31  5:04 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Taylor Blau, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 7 +++++--
 merge-ort.h | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 6237e2fb7fe..86eebf39166 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4198,7 +4198,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;
@@ -4222,7 +4223,7 @@ void merge_display_update_messages(struct merge_options *opt,
 	for (i = 0; i < olist.nr; ++i) {
 		struct strbuf *sb = olist.items[i].util;
 
-		printf("%s", sb->buf);
+		fprintf(stream, "%s", sb->buf);
 	}
 	string_list_clear(&olist, 0);
 
@@ -4271,6 +4272,8 @@ void merge_switch_to_result(struct merge_options *opt,
 		trace2_region_leave("merge", "write_auto_merge", opt->repo);
 	}
 
+	if (display_update_msgs)
+		merge_display_update_messages(opt, result, stdout);
 
 	merge_finalize(opt, result);
 }
diff --git a/merge-ort.h b/merge-ort.h
index 1b93555a60b..55819a57da8 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -76,7 +76,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


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 7/8] merge-tree: support saving merge messages to a separate file
  2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                   ` (5 preceding siblings ...)
  2021-12-31  5:04 ` [PATCH 6/8] merge-ort: allow update messages to be written to different file stream Elijah Newren via GitGitGadget
@ 2021-12-31  5:04 ` Elijah Newren via GitGitGadget
  2022-01-03 12:31   ` Fabian Stelzer
  2022-01-03 12:35   ` Fabian Stelzer
  2021-12-31  5:04 ` [PATCH 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
  8 siblings, 2 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-12-31  5:04 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Taylor Blau, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

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

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-merge-tree.txt |  6 ++++--
 builtin/merge-tree.c             | 18 ++++++++++++++++--
 t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index 5823938937f..4d5857b390b 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree
 SYNOPSIS
 --------
 [verse]
-'git merge-tree' --real <branch1> <branch2>
+'git merge-tree' --real [--messages=<file>] <branch1> <branch2>
 'git merge-tree' <base-tree> <branch1> <branch2>
 
 DESCRIPTION
@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive
 merge with rename detection.  If the merge is clean, the exit status
 will be `0`, and if the merge has conflicts, the exit status will be
 `1`.  The output will consist solely of the resulting toplevel tree
-(which may have files including conflict markers).
+(which may have files including conflict markers).  With `--messages`,
+it will write any informational messages (such as "Auto-merging
+<path>" and conflict notices) to the given file.
 
 The second form is meant for backward compatibility and will only do a
 trival merge.  It reads three tree-ish, and outputs trivial merge
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index c5757bed5bb..47deef0b199 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -389,6 +389,7 @@ static int trivial_merge(const char *base,
 
 struct merge_tree_options {
 	int real;
+	char *messages_file;
 };
 
 static int real_merge(struct merge_tree_options *o,
@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
 	 */
 
 	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+
+	if (o->messages_file) {
+		FILE *fp = xfopen(o->messages_file, "w");
+		merge_display_update_messages(&opt, &result, fp);
+		fclose(fp);
+	}
 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
-	merge_switch_to_result(&opt, NULL, &result, 0, 0);
+
+	merge_finalize(&opt, &result);
 	return result.clean ? 0 : 1;
 }
 
@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
 	struct merge_tree_options o = { 0 };
 	int expected_remaining_argc;
+	int original_argc;
 
 	const char * const merge_tree_usage[] = {
-		N_("git merge-tree --real <branch1> <branch2>"),
+		N_("git merge-tree --real [<options>] <branch1> <branch2>"),
 		N_("git merge-tree <base-tree> <branch1> <branch2>"),
 		NULL
 	};
 	struct option mt_options[] = {
 		OPT_BOOL(0, "real", &o.real,
 			 N_("do a real merge instead of a trivial merge")),
+		OPT_STRING(0, "messages", &o.messages_file, N_("file"),
+			   N_("filename to write informational/conflict messages to")),
 		OPT_END()
 	};
 
@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 		usage_with_options(merge_tree_usage, mt_options);
 
 	/* Parse arguments */
+	original_argc = argc;
 	argc = parse_options(argc, argv, prefix, mt_options,
 			     merge_tree_usage, 0);
+	if (!o.real && original_argc < argc)
+		die(_("--real must be specified if any other options are"));
 	expected_remaining_argc = (o.real ? 2 : 3);
 	if (argc != expected_remaining_argc)
 		usage_with_options(merge_tree_usage, mt_options);
diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
index 9fb617ccc7f..42218cdc019 100755
--- a/t/t4301-merge-tree-real.sh
+++ b/t/t4301-merge-tree-real.sh
@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
 	grep "^usage: git merge-tree" expect
 '
 
+test_expect_success '--messages gives us the conflict notices and such' '
+	test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&
+
+	# Expected results:
+	#   "greeting" should merge with conflicts
+	#   "numbers" should merge cleanly
+	#   "whatever" has *both* a modify/delete and a file/directory conflict
+	cat <<-EOF >expect &&
+	Auto-merging greeting
+	CONFLICT (content): Merge conflict in greeting
+	Auto-merging numbers
+	CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead.
+	CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1.  Version side1 of whatever~side1 left in tree.
+	EOF
+
+	test_cmp expect MSG_FILE
+'
+
 test_done
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 8/8] merge-tree: provide an easy way to access which files have conflicts
  2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                   ` (6 preceding siblings ...)
  2021-12-31  5:04 ` [PATCH 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
@ 2021-12-31  5:04 ` Elijah Newren via GitGitGadget
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
  8 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-12-31  5:04 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Taylor Blau, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

Callers of `git merge-tree --real` might want an easy way to determine
which files conflicted.  While they could potentially use the --messages
option and parse the resulting messages written to that file, those
messages are not meant to be machine readable.  Provide a simpler
mechanism of having the user specify --unmerged-list=$FILENAME, and then
write a NUL-separated list of unmerged filenames to the specified file.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-merge-tree.txt |  6 ++++--
 builtin/merge-tree.c             | 16 ++++++++++++++++
 merge-ort.c                      | 13 +++++++++++++
 merge-ort.h                      |  3 +++
 t/t4301-merge-tree-real.sh       |  9 +++++++++
 5 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index 4d5857b390b..542cea1a1a8 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree
 SYNOPSIS
 --------
 [verse]
-'git merge-tree' --real [--messages=<file>] <branch1> <branch2>
+'git merge-tree' --real [--messages=<file>] [--conflicted-list=<file>] <branch1> <branch2>
 'git merge-tree' <base-tree> <branch1> <branch2>
 
 DESCRIPTION
@@ -23,7 +23,9 @@ will be `0`, and if the merge has conflicts, the exit status will be
 `1`.  The output will consist solely of the resulting toplevel tree
 (which may have files including conflict markers).  With `--messages`,
 it will write any informational messages (such as "Auto-merging
-<path>" and conflict notices) to the given file.
+<path>" and conflict notices) to the given file.  With
+`--conflicted-list`, it will write a list of unmerged files, one per
+line, to the given file.
 
 The second form is meant for backward compatibility and will only do a
 trival merge.  It reads three tree-ish, and outputs trivial merge
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 47deef0b199..90bd1e92135 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -390,6 +390,7 @@ static int trivial_merge(const char *base,
 struct merge_tree_options {
 	int real;
 	char *messages_file;
+	char *conflicted_file;
 };
 
 static int real_merge(struct merge_tree_options *o,
@@ -449,6 +450,19 @@ static int real_merge(struct merge_tree_options *o,
 		merge_display_update_messages(&opt, &result, fp);
 		fclose(fp);
 	}
+	if (o->conflicted_file) {
+		struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
+		FILE *fp = xfopen(o->conflicted_file, "w");
+		int i;
+
+		merge_get_conflicted_files(&result, &conflicted_files);
+		for (i = 0; i < conflicted_files.nr; i++) {
+			fprintf(fp, "%s", conflicted_files.items[i].string);
+			fputc('\0', fp);
+		}
+		fclose(fp);
+		string_list_clear(&conflicted_files, 0);
+	}
 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
 
 	merge_finalize(&opt, &result);
@@ -471,6 +485,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 			 N_("do a real merge instead of a trivial merge")),
 		OPT_STRING(0, "messages", &o.messages_file, N_("file"),
 			   N_("filename to write informational/conflict messages to")),
+		OPT_STRING(0, "conflicted-list", &o.conflicted_file, N_("file"),
+			   N_("filename to write list of unmerged files")),
 		OPT_END()
 	};
 
diff --git a/merge-ort.c b/merge-ort.c
index 86eebf39166..3d6dd1b234c 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4234,6 +4234,19 @@ 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) {
+		string_list_append(conflicted_files, e->key);
+	}
+	string_list_sort(conflicted_files);
+}
+
 void merge_switch_to_result(struct merge_options *opt,
 			    struct tree *head,
 			    struct merge_result *result,
diff --git a/merge-ort.h b/merge-ort.h
index 55819a57da8..165cef6616f 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -79,6 +79,9 @@ void merge_display_update_messages(struct merge_options *opt,
 				   struct merge_result *result,
 				   FILE *stream);
 
+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);
diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
index 42218cdc019..0b725eef9fc 100755
--- a/t/t4301-merge-tree-real.sh
+++ b/t/t4301-merge-tree-real.sh
@@ -96,4 +96,13 @@ test_expect_success '--messages gives us the conflict notices and such' '
 	test_cmp expect MSG_FILE
 '
 
+test_expect_success '--messages gives us the conflict notices and such' '
+	test_must_fail git merge-tree --real --conflicted-list=UNMERGED side1 side2 &&
+
+	cat UNMERGED | tr "\0" "\n" >actual &&
+	test_write_lines greeting whatever~side1 >expect &&
+
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 4/8] merge-tree: implement real merges
  2021-12-31  5:04 ` [PATCH 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
@ 2022-01-01 20:08   ` Johannes Altmanninger
  2022-01-01 21:11     ` Elijah Newren
  2022-01-03 12:23   ` Fabian Stelzer
  1 sibling, 1 reply; 57+ messages in thread
From: Johannes Altmanninger @ 2022-01-01 20:08 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Elijah Newren

On Fri, Dec 31, 2021 at 05:04:00AM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> This adds the ability to perform real merges rather than just trivial
> merges (meaning handling three way content merges, recursive ancestor
> consolidation, renames, proper directory/file conflict handling, and so
> forth).  However, unlike `git merge`, the working tree and index are
> left alone and no branch is updated.
> 
> The only output is:
>   - the toplevel resulting tree printed on stdout
>   - exit status of 0 (clean) or 1 (conflicts present)
> 
> This output is mean to be used by some higher level script, perhaps in a
> sequence of steps like this:
> 
>    NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
>    test $? -eq 0 || die "There were conflicts..."
>    NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 $BRANCH2)
>    git update-ref $BRANCH1 $NEWCOMMIT
> 
> Note that higher level scripts may also want to access the
> conflict/warning messages normally output during a merge, or have quick
> access to a list of files with conflicts.  That is not available in this
> preliminary implementation, but subsequent commits will add that
> ability.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  Documentation/git-merge-tree.txt | 28 +++++++----
>  builtin/merge-tree.c             | 55 +++++++++++++++++++++-
>  t/t4301-merge-tree-real.sh       | 81 ++++++++++++++++++++++++++++++++
>  3 files changed, 153 insertions(+), 11 deletions(-)
>  create mode 100755 t/t4301-merge-tree-real.sh
> 
> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> index 58731c19422..5823938937f 100644
> --- a/Documentation/git-merge-tree.txt
> +++ b/Documentation/git-merge-tree.txt
> @@ -3,26 +3,34 @@ git-merge-tree(1)
>  
>  NAME
>  ----
> -git-merge-tree - Show three-way merge without touching index
> +git-merge-tree - Perform merge without touching index or working tree
>  
>  
>  SYNOPSIS
>  --------
>  [verse]
> +'git merge-tree' --real <branch1> <branch2>
>  'git merge-tree' <base-tree> <branch1> <branch2>

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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 6/8] merge-ort: allow update messages to be written to different file stream
  2021-12-31  5:04 ` [PATCH 6/8] merge-ort: allow update messages to be written to different file stream Elijah Newren via GitGitGadget
@ 2022-01-01 20:08   ` Johannes Altmanninger
  2022-01-01 20:19     ` Elijah Newren
  0 siblings, 1 reply; 57+ messages in thread
From: Johannes Altmanninger @ 2022-01-01 20:08 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Elijah Newren

On Fri, Dec 31, 2021 at 05:04:02AM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> This modifies the new display_update_messages() function to allow
> printing to somewhere other than stdout.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 7 +++++--
>  merge-ort.h | 3 ++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/merge-ort.c b/merge-ort.c
> index 6237e2fb7fe..86eebf39166 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> [...]
> @@ -4271,6 +4272,8 @@ void merge_switch_to_result(struct merge_options *opt,
>  		trace2_region_leave("merge", "write_auto_merge", opt->repo);
>  	}
>  
> +	if (display_update_msgs)
> +		merge_display_update_messages(opt, result, stdout);

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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/8] merge-tree: move logic for existing merge into new function
  2021-12-31  5:03 ` [PATCH 2/8] merge-tree: move logic for existing merge into new function Elijah Newren via GitGitGadget
@ 2022-01-01 20:11   ` Johannes Altmanninger
  2022-01-01 20:17     ` Elijah Newren
  0 siblings, 1 reply; 57+ messages in thread
From: Johannes Altmanninger @ 2022-01-01 20:11 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Elijah Newren

On Fri, Dec 31, 2021 at 05:03:58AM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> In preparation for adding a non-trivial merge capability to merge-tree,
> move the existing merge logic for trivial merges into a new function.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/merge-tree.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 06f9eee9f78..9fe5b99f623 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -366,15 +366,11 @@ static void *get_tree_descriptor(struct repository *r,
>  	return buf;
>  }
>  
> -int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> -{
> +static int trivial_merge(int argc, const char **argv) {

I guess the brace should probably stay on its own line

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/8] merge-tree: move logic for existing merge into new function
  2022-01-01 20:11   ` Johannes Altmanninger
@ 2022-01-01 20:17     ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-01 20:17 UTC (permalink / raw)
  To: Johannes Altmanninger
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau

On Sat, Jan 1, 2022 at 12:11 PM Johannes Altmanninger <aclopte@gmail.com> wrote:
>
> On Fri, Dec 31, 2021 at 05:03:58AM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > In preparation for adding a non-trivial merge capability to merge-tree,
> > move the existing merge logic for trivial merges into a new function.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  builtin/merge-tree.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > index 06f9eee9f78..9fe5b99f623 100644
> > --- a/builtin/merge-tree.c
> > +++ b/builtin/merge-tree.c
> > @@ -366,15 +366,11 @@ static void *get_tree_descriptor(struct repository *r,
> >       return buf;
> >  }
> >
> > -int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> > -{
> > +static int trivial_merge(int argc, const char **argv) {
>
> I guess the brace should probably stay on its own line

Whoops, indeed.  Thanks for spotting.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 6/8] merge-ort: allow update messages to be written to different file stream
  2022-01-01 20:08   ` Johannes Altmanninger
@ 2022-01-01 20:19     ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-01 20:19 UTC (permalink / raw)
  To: Johannes Altmanninger
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau

On Sat, Jan 1, 2022 at 12:08 PM Johannes Altmanninger <aclopte@gmail.com> wrote:
>
> On Fri, Dec 31, 2021 at 05:04:02AM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > This modifies the new display_update_messages() function to allow
> > printing to somewhere other than stdout.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  merge-ort.c | 7 +++++--
> >  merge-ort.h | 3 ++-
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 6237e2fb7fe..86eebf39166 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > [...]
> > @@ -4271,6 +4272,8 @@ void merge_switch_to_result(struct merge_options *opt,
> >               trace2_region_leave("merge", "write_auto_merge", opt->repo);
> >       }
> >
> > +     if (display_update_msgs)
> > +             merge_display_update_messages(opt, result, stdout);
>
> is it intentional that the previous patch doesn't have this call?

Ugh, oops.  Yeah, bad split of the patches; this should have been part
of the previous one.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 4/8] merge-tree: implement real merges
  2022-01-01 20:08   ` Johannes Altmanninger
@ 2022-01-01 21:11     ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-01 21:11 UTC (permalink / raw)
  To: Johannes Altmanninger
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau

On Sat, Jan 1, 2022 at 12:08 PM Johannes Altmanninger <aclopte@gmail.com> wrote:
>
> On Fri, Dec 31, 2021 at 05:04:00AM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > This adds the ability to perform real merges rather than just trivial
> > merges (meaning handling three way content merges, recursive ancestor
> > consolidation, renames, proper directory/file conflict handling, and so
> > forth).  However, unlike `git merge`, the working tree and index are
> > left alone and no branch is updated.
> >
> > The only output is:
> >   - the toplevel resulting tree printed on stdout
> >   - exit status of 0 (clean) or 1 (conflicts present)
> >
> > This output is mean to be used by some higher level script, perhaps in a
> > sequence of steps like this:
> >
> >    NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
> >    test $? -eq 0 || die "There were conflicts..."
> >    NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 $BRANCH2)
> >    git update-ref $BRANCH1 $NEWCOMMIT
> >
> > Note that higher level scripts may also want to access the
> > conflict/warning messages normally output during a merge, or have quick
> > access to a list of files with conflicts.  That is not available in this
> > preliminary implementation, but subsequent commits will add that
> > ability.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  Documentation/git-merge-tree.txt | 28 +++++++----
> >  builtin/merge-tree.c             | 55 +++++++++++++++++++++-
> >  t/t4301-merge-tree-real.sh       | 81 ++++++++++++++++++++++++++++++++
> >  3 files changed, 153 insertions(+), 11 deletions(-)
> >  create mode 100755 t/t4301-merge-tree-real.sh
> >
> > diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> > index 58731c19422..5823938937f 100644
> > --- a/Documentation/git-merge-tree.txt
> > +++ b/Documentation/git-merge-tree.txt
> > @@ -3,26 +3,34 @@ git-merge-tree(1)
> >
> >  NAME
> >  ----
> > -git-merge-tree - Show three-way merge without touching index
> > +git-merge-tree - Perform merge without touching index or working tree
> >
> >
> >  SYNOPSIS
> >  --------
> >  [verse]
> > +'git merge-tree' --real <branch1> <branch2>
> >  'git merge-tree' <base-tree> <branch1> <branch2>
>
> This is really exciting. It could replace the merge-machinery of git-revise
> (which is a "fast rebase" tool).
> I think for cherry-pick/rebase we need to specify a custom merge base,
> would that suit the new form?

I'm glad you're excited about it.  :-)

I think having a server side tool for replaying commits (which can
double as a fast rebase/cherry-pick tool on the client side) is also
important, but I think it should be part of a proper builtin, not some
script that calls out to `merge-tree --real`.

`merge-tree --real` is simpler, though, so I implemented and submitted it first.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 5/8] merge-ort: split out a separate display_update_messages() function
  2021-12-31  5:04 ` [PATCH 5/8] merge-ort: split out a separate display_update_messages() function Elijah Newren via GitGitGadget
@ 2022-01-03 12:15   ` Fabian Stelzer
  2022-01-03 12:25     ` Fabian Stelzer
  0 siblings, 1 reply; 57+ messages in thread
From: Fabian Stelzer @ 2022-01-03 12:15 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Elijah Newren

On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
>From: Elijah Newren <newren@gmail.com>
>
>No functional changes included in this patch; it's just a preparatory
>step in anticipation of wanting to handle the printed messages
>differently in `git merge-tree --real`.

Not quite. You are missing:
  +       if (display_update_msgs)
  +               merge_display_update_messages(opt, result, stdout);

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

>
>Signed-off-by: Elijah Newren <newren@gmail.com>
>---
> merge-ort.c | 69 ++++++++++++++++++++++++++++-------------------------
> merge-ort.h |  8 +++++++
> 2 files changed, 44 insertions(+), 33 deletions(-)
>
>diff --git a/merge-ort.c b/merge-ort.c
>index 0342f104836..6237e2fb7fe 100644
>--- a/merge-ort.c
>+++ b/merge-ort.c
>@@ -4197,6 +4197,42 @@ static int record_conflicted_index_entries(struct merge_options *opt)
> 	return errs;
> }
>
>+void merge_display_update_messages(struct merge_options *opt,
>+				   struct merge_result *result)
>+{
>+	struct merge_options_internal *opti = result->priv;
>+	struct hashmap_iter iter;
>+	struct strmap_entry *e;
>+	struct string_list olist = STRING_LIST_INIT_NODUP;
>+	int i;
>+
>+	trace2_region_enter("merge", "display messages", opt->repo);
>+
>+	/* Hack to pre-allocate olist to the desired size */
>+	ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
>+		   olist.alloc);
>+
>+	/* Put every entry from output into olist, then sort */
>+	strmap_for_each_entry(&opti->output, &iter, e) {
>+		string_list_append(&olist, e->key)->util = e->value;
>+	}
>+	string_list_sort(&olist);
>+
>+	/* Iterate over the items, printing them */
>+	for (i = 0; i < olist.nr; ++i) {
>+		struct strbuf *sb = olist.items[i].util;
>+
>+		printf("%s", sb->buf);
>+	}
>+	string_list_clear(&olist, 0);
>+
>+	/* Also include needed rename limit adjustment now */
>+	diff_warn_rename_limit("merge.renamelimit",
>+			       opti->renames.needed_limit, 0);
>+
>+	trace2_region_leave("merge", "display messages", opt->repo);
>+}
>+
> void merge_switch_to_result(struct merge_options *opt,
> 			    struct tree *head,
> 			    struct merge_result *result,
>@@ -4235,39 +4271,6 @@ void merge_switch_to_result(struct merge_options *opt,
> 		trace2_region_leave("merge", "write_auto_merge", opt->repo);
> 	}
>
>-	if (display_update_msgs) {
>-		struct merge_options_internal *opti = result->priv;
>-		struct hashmap_iter iter;
>-		struct strmap_entry *e;
>-		struct string_list olist = STRING_LIST_INIT_NODUP;
>-		int i;
>-
>-		trace2_region_enter("merge", "display messages", opt->repo);
>-
>-		/* Hack to pre-allocate olist to the desired size */
>-		ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
>-			   olist.alloc);
>-
>-		/* Put every entry from output into olist, then sort */
>-		strmap_for_each_entry(&opti->output, &iter, e) {
>-			string_list_append(&olist, e->key)->util = e->value;
>-		}
>-		string_list_sort(&olist);
>-
>-		/* Iterate over the items, printing them */
>-		for (i = 0; i < olist.nr; ++i) {
>-			struct strbuf *sb = olist.items[i].util;
>-
>-			printf("%s", sb->buf);
>-		}
>-		string_list_clear(&olist, 0);
>-
>-		/* Also include needed rename limit adjustment now */
>-		diff_warn_rename_limit("merge.renamelimit",
>-				       opti->renames.needed_limit, 0);
>-
>-		trace2_region_leave("merge", "display messages", opt->repo);
>-	}
>
> 	merge_finalize(opt, result);
> }
>diff --git a/merge-ort.h b/merge-ort.h
>index c011864ffeb..1b93555a60b 100644
>--- a/merge-ort.h
>+++ b/merge-ort.h
>@@ -70,6 +70,14 @@ void merge_switch_to_result(struct merge_options *opt,
> 			    int update_worktree_and_index,
> 			    int display_update_msgs);
>
>+/*
>+ * Display messages about conflicts and which files were 3-way merged.
>+ * Automatically called by merge_switch_to_result() with stream == stdout,
>+ * so only call this when bypassing merge_switch_to_result().
>+ */
>+void merge_display_update_messages(struct merge_options *opt,
>+				   struct merge_result *result);
>+
> /* Do needed cleanup when not calling merge_switch_to_result() */
> void merge_finalize(struct merge_options *opt,
> 		    struct merge_result *result);
>-- 
>gitgitgadget
>

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 4/8] merge-tree: implement real merges
  2021-12-31  5:04 ` [PATCH 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
  2022-01-01 20:08   ` Johannes Altmanninger
@ 2022-01-03 12:23   ` Fabian Stelzer
  2022-01-03 16:37     ` Elijah Newren
  1 sibling, 1 reply; 57+ messages in thread
From: Fabian Stelzer @ 2022-01-03 12:23 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Elijah Newren

On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
>From: Elijah Newren <newren@gmail.com>
>
>This adds the ability to perform real merges rather than just trivial
>merges (meaning handling three way content merges, recursive ancestor
>consolidation, renames, proper directory/file conflict handling, and so
>forth).  However, unlike `git merge`, the working tree and index are
>left alone and no branch is updated.
>
>The only output is:
>  - the toplevel resulting tree printed on stdout
>  - exit status of 0 (clean) or 1 (conflicts present)
>
>This output is mean to be used by some higher level script, perhaps in a
>sequence of steps like this:
>
>   NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
>   test $? -eq 0 || die "There were conflicts..."
>   NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 $BRANCH2)
>   git update-ref $BRANCH1 $NEWCOMMIT
>
>Note that higher level scripts may also want to access the
>conflict/warning messages normally output during a merge, or have quick
>access to a list of files with conflicts.  That is not available in this
>preliminary implementation, but subsequent commits will add that
>ability.
>
>Signed-off-by: Elijah Newren <newren@gmail.com>
>---
> Documentation/git-merge-tree.txt | 28 +++++++----
> builtin/merge-tree.c             | 55 +++++++++++++++++++++-
> t/t4301-merge-tree-real.sh       | 81 ++++++++++++++++++++++++++++++++
> 3 files changed, 153 insertions(+), 11 deletions(-)
> create mode 100755 t/t4301-merge-tree-real.sh
>
>diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
>index 58731c19422..5823938937f 100644
>--- a/Documentation/git-merge-tree.txt
>+++ b/Documentation/git-merge-tree.txt
>@@ -3,26 +3,34 @@ git-merge-tree(1)
>
> NAME
> ----
>-git-merge-tree - Show three-way merge without touching index
>+git-merge-tree - Perform merge without touching index or working tree
>
>
> SYNOPSIS
> --------
> [verse]
>+'git merge-tree' --real <branch1> <branch2>
> 'git merge-tree' <base-tree> <branch1> <branch2>
>
> DESCRIPTION
> -----------
>-Reads three tree-ish, and output trivial merge results and
>-conflicting stages to the standard output.  This is similar to
>-what three-way 'git read-tree -m' does, but instead of storing the
>-results in the index, the command outputs the entries to the
>-standard output.
>+Performs a merge, but does not make any new commits and does not read
>+from or write to either the working tree or index.
>
>-This is meant to be used by higher level scripts to compute
>-merge results outside of the index, and stuff the results back into the
>-index.  For this reason, the output from the command omits
>-entries that match the <branch1> tree.
>+The first form will merge the two branches, doing a full recursive
>+merge with rename detection.  If the merge is clean, the exit status
>+will be `0`, and if the merge has conflicts, the exit status will be
>+`1`.  The output will consist solely of the resulting toplevel tree
>+(which may have files including conflict markers).
>+
>+The second form is meant for backward compatibility and will only do a
>+trival merge.  It reads three tree-ish, and outputs trivial merge
>+results and conflicting stages to the standard output in a semi-diff
>+format.  Since this was designed for higher level scripts to consume
>+and merge the results back into the index, it omits entries that match
>+<branch1>.  The result of this second form is is similar to what
>+three-way 'git read-tree -m' does, but instead of storing the results
>+in the index, the command outputs the entries to the standard output.
>
> GIT
> ---
>diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
>index f04b1eaad0a..c5757bed5bb 100644
>--- a/builtin/merge-tree.c
>+++ b/builtin/merge-tree.c
>@@ -2,6 +2,9 @@
> #include "builtin.h"
> #include "tree-walk.h"
> #include "xdiff-interface.h"
>+#include "help.h"
>+#include "commit-reach.h"
>+#include "merge-ort.h"
> #include "object-store.h"
> #include "parse-options.h"
> #include "repository.h"
>@@ -391,7 +394,57 @@ struct merge_tree_options {
> static int real_merge(struct merge_tree_options *o,
> 		      const char *branch1, const char *branch2)
> {
>-	die(_("real merges are not yet implemented"));
>+	struct commit *parent1, *parent2;
>+	struct commit_list *common;
>+	struct commit_list *merge_bases = NULL;
>+	struct commit_list *j;
>+	struct merge_options opt;
>+	struct merge_result result = { 0 };
>+
>+	parent1 = get_merge_parent(branch1);
>+	if (!parent1)
>+		help_unknown_ref(branch1, "merge",
>+				 _("not something we can merge"));
>+
>+	parent2 = get_merge_parent(branch2);
>+	if (!parent2)
>+		help_unknown_ref(branch2, "merge",
>+				 _("not something we can merge"));
>+
>+	init_merge_options(&opt, the_repository);
>+	/*
>+	 * TODO: Support subtree and other -X options?
>+	if (use_strategies_nr == 1 &&
>+	    !strcmp(use_strategies[0]->name, "subtree"))
>+		opt.subtree_shift = "";
>+	for (x = 0; x < xopts_nr; x++)
>+		if (parse_merge_opt(&opt, xopts[x]))
>+			die(_("Unknown strategy option: -X%s"), xopts[x]);
>+	*/
>+
>+	opt.show_rename_progress = 0;
>+
>+	opt.branch1 = merge_remote_util(parent1)->name; /* or just branch1? */
>+	opt.branch2 = merge_remote_util(parent2)->name; /* or just branch2? */
>+
>+	/*
>+	 * Get the merge bases, in reverse order; see comment above
>+	 * merge_incore_recursive in merge-ort.h
>+	 */
>+	common = get_merge_bases(parent1, parent2);
>+	for (j = common; j; j = j->next)
>+		commit_list_insert(j->item, &merge_bases);
>+
>+	/*
>+	 * TODO: notify if merging unrelated histories?
>+	if (!common)
>+		fprintf(stderr, _("merging unrelated histories"));
>+	 */
>+
>+	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
>+	printf("%s\n", oid_to_hex(&result.tree->object.oid));
>+	merge_switch_to_result(&opt, NULL, &result, 0, 0);
>+	return result.clean ? 0 : 1;
> }
>
> int cmd_merge_tree(int argc, const char **argv, const char *prefix)
>diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
>new file mode 100755
>index 00000000000..9fb617ccc7f
>--- /dev/null
>+++ b/t/t4301-merge-tree-real.sh
>@@ -0,0 +1,81 @@
>+#!/bin/sh
>+
>+test_description='git merge-tree --real'
>+
>+. ./test-lib.sh
>+
>+# This test is ort-specific
>+GIT_TEST_MERGE_ALGORITHM=ort
>+export GIT_TEST_MERGE_ALGORITHM
>+
>+test_expect_success setup '
>+	test_write_lines 1 2 3 4 5 >numbers &&
>+	echo hello >greeting &&
>+	echo foo >whatever &&
>+	git add numbers greeting whatever &&
>+	git commit -m initial &&
>+
>+	git branch side1 &&
>+	git branch side2 &&
>+
>+	git checkout side1 &&
>+	test_write_lines 1 2 3 4 5 6 >numbers &&
>+	echo hi >greeting &&
>+	echo bar >whatever &&
>+	git add numbers greeting whatever &&
>+	git commit -m rename-and-modify &&

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

>+
>+	git checkout side2 &&
>+	test_write_lines 0 1 2 3 4 5 >numbers &&
>+	echo yo >greeting &&
>+	git rm whatever &&
>+	mkdir whatever &&
>+	>whatever/empty &&
>+	git add numbers greeting whatever/empty &&
>+	git commit -m remove-and-rename

And this looks more like a remove-and-modify. Is it still a rename when we 
empty the files content?

>+'
>+
>+test_expect_success 'Content merge and a few conflicts' '
>+	git checkout side1^0 &&
>+	test_must_fail git merge side2 &&
>+	cp .git/AUTO_MERGE EXPECT &&
>+	E_TREE=$(cat EXPECT) &&
>+
>+	git reset --hard &&
>+	test_must_fail git merge-tree --real side1 side2 >RESULT &&
>+	R_TREE=$(cat RESULT) &&
>+
>+	# Due to differences of e.g. "HEAD" vs "side1", the results will not
>+	# exactly match.  Dig into individual files.
>+
>+	# Numbers should have three-way merged cleanly
>+	test_write_lines 0 1 2 3 4 5 6 >expect &&
>+	git show ${R_TREE}:numbers >actual &&
>+	test_cmp expect actual &&
>+
>+	# whatever and whatever~<branch> should have same HASHES
>+	git rev-parse ${E_TREE}:whatever ${E_TREE}:whatever~HEAD >expect &&
>+	git rev-parse ${R_TREE}:whatever ${R_TREE}:whatever~side1 >actual &&
>+	test_cmp expect actual &&
>+
>+	# greeting should have a merge conflict
>+	git show ${E_TREE}:greeting >tmp &&
>+	cat tmp | sed -e s/HEAD/side1/ >expect &&
>+	git show ${R_TREE}:greeting >actual &&
>+	test_cmp expect actual
>+'
>+
>+test_expect_success 'Barf on misspelled option' '
>+	# Mis-spell with single "s" instead of double "s"
>+	test_expect_code 129 git merge-tree --real --mesages FOOBAR side1 side2 2>expect &&
>+
>+	grep "error: unknown option.*mesages" expect
>+'
>+
>+test_expect_success 'Barf on too many arguments' '
>+	test_expect_code 129 git merge-tree --real side1 side2 side3 2>expect &&
>+
>+	grep "^usage: git merge-tree" expect
>+'
>+
>+test_done
>-- 
>gitgitgadget
>

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 5/8] merge-ort: split out a separate display_update_messages() function
  2022-01-03 12:15   ` Fabian Stelzer
@ 2022-01-03 12:25     ` Fabian Stelzer
  0 siblings, 0 replies; 57+ messages in thread
From: Fabian Stelzer @ 2022-01-03 12:25 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Elijah Newren

On 03.01.2022 13:15, Fabian Stelzer wrote:
>On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
>>From: Elijah Newren <newren@gmail.com>
>>
>>No functional changes included in this patch; it's just a preparatory
>>step in anticipation of wanting to handle the printed messages
>>differently in `git merge-tree --real`.
>
>Not quite. You are missing:
> +       if (display_update_msgs)
> +               merge_display_update_messages(opt, result, stdout);
>
>in merge_switch_to_result(), which you added in the next commit. Not 
>really relevant for the series as a whole but this commit alone breaks 
>some functionality.

Sorry, just saw this was already noted on the next patch.

>
>>
>>Signed-off-by: Elijah Newren <newren@gmail.com>
>>---
>>merge-ort.c | 69 ++++++++++++++++++++++++++++-------------------------
>>merge-ort.h |  8 +++++++
>>2 files changed, 44 insertions(+), 33 deletions(-)
>>
>>diff --git a/merge-ort.c b/merge-ort.c
>>index 0342f104836..6237e2fb7fe 100644
>>--- a/merge-ort.c
>>+++ b/merge-ort.c
>>@@ -4197,6 +4197,42 @@ static int record_conflicted_index_entries(struct merge_options *opt)
>>	return errs;
>>}
>>
>>+void merge_display_update_messages(struct merge_options *opt,
>>+				   struct merge_result *result)
>>+{
>>+	struct merge_options_internal *opti = result->priv;
>>+	struct hashmap_iter iter;
>>+	struct strmap_entry *e;
>>+	struct string_list olist = STRING_LIST_INIT_NODUP;
>>+	int i;
>>+
>>+	trace2_region_enter("merge", "display messages", opt->repo);
>>+
>>+	/* Hack to pre-allocate olist to the desired size */
>>+	ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
>>+		   olist.alloc);
>>+
>>+	/* Put every entry from output into olist, then sort */
>>+	strmap_for_each_entry(&opti->output, &iter, e) {
>>+		string_list_append(&olist, e->key)->util = e->value;
>>+	}
>>+	string_list_sort(&olist);
>>+
>>+	/* Iterate over the items, printing them */
>>+	for (i = 0; i < olist.nr; ++i) {
>>+		struct strbuf *sb = olist.items[i].util;
>>+
>>+		printf("%s", sb->buf);
>>+	}
>>+	string_list_clear(&olist, 0);
>>+
>>+	/* Also include needed rename limit adjustment now */
>>+	diff_warn_rename_limit("merge.renamelimit",
>>+			       opti->renames.needed_limit, 0);
>>+
>>+	trace2_region_leave("merge", "display messages", opt->repo);
>>+}
>>+
>>void merge_switch_to_result(struct merge_options *opt,
>>			    struct tree *head,
>>			    struct merge_result *result,
>>@@ -4235,39 +4271,6 @@ void merge_switch_to_result(struct merge_options *opt,
>>		trace2_region_leave("merge", "write_auto_merge", opt->repo);
>>	}
>>
>>-	if (display_update_msgs) {
>>-		struct merge_options_internal *opti = result->priv;
>>-		struct hashmap_iter iter;
>>-		struct strmap_entry *e;
>>-		struct string_list olist = STRING_LIST_INIT_NODUP;
>>-		int i;
>>-
>>-		trace2_region_enter("merge", "display messages", opt->repo);
>>-
>>-		/* Hack to pre-allocate olist to the desired size */
>>-		ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
>>-			   olist.alloc);
>>-
>>-		/* Put every entry from output into olist, then sort */
>>-		strmap_for_each_entry(&opti->output, &iter, e) {
>>-			string_list_append(&olist, e->key)->util = e->value;
>>-		}
>>-		string_list_sort(&olist);
>>-
>>-		/* Iterate over the items, printing them */
>>-		for (i = 0; i < olist.nr; ++i) {
>>-			struct strbuf *sb = olist.items[i].util;
>>-
>>-			printf("%s", sb->buf);
>>-		}
>>-		string_list_clear(&olist, 0);
>>-
>>-		/* Also include needed rename limit adjustment now */
>>-		diff_warn_rename_limit("merge.renamelimit",
>>-				       opti->renames.needed_limit, 0);
>>-
>>-		trace2_region_leave("merge", "display messages", opt->repo);
>>-	}
>>
>>	merge_finalize(opt, result);
>>}
>>diff --git a/merge-ort.h b/merge-ort.h
>>index c011864ffeb..1b93555a60b 100644
>>--- a/merge-ort.h
>>+++ b/merge-ort.h
>>@@ -70,6 +70,14 @@ void merge_switch_to_result(struct merge_options *opt,
>>			    int update_worktree_and_index,
>>			    int display_update_msgs);
>>
>>+/*
>>+ * Display messages about conflicts and which files were 3-way merged.
>>+ * Automatically called by merge_switch_to_result() with stream == stdout,
>>+ * so only call this when bypassing merge_switch_to_result().
>>+ */
>>+void merge_display_update_messages(struct merge_options *opt,
>>+				   struct merge_result *result);
>>+
>>/* Do needed cleanup when not calling merge_switch_to_result() */
>>void merge_finalize(struct merge_options *opt,
>>		    struct merge_result *result);
>>-- 
>>gitgitgadget
>>

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 7/8] merge-tree: support saving merge messages to a separate file
  2021-12-31  5:04 ` [PATCH 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
@ 2022-01-03 12:31   ` Fabian Stelzer
  2022-01-03 16:51     ` Elijah Newren
  2022-01-03 12:35   ` Fabian Stelzer
  1 sibling, 1 reply; 57+ messages in thread
From: Fabian Stelzer @ 2022-01-03 12:31 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Elijah Newren

On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
>From: Elijah Newren <newren@gmail.com>
>
>When running `git merge-tree --real`, we previously would only return an
>exit status reflecting the cleanness of a merge, and print out the
>toplevel tree of the resulting merge.  Merges also have informational
>messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT
>(file/directory)", etc.)  In fact, when non-content conflicts occur
>(such as file/directory, modify/delete, add/add with differing modes,
>rename/rename (1to2), etc.), these informational messages are often the
>only notification since these conflicts are not representable in the
>contents of the file.
>
>Add a --messages option which names a file so that callers can request
>these messages be recorded somewhere.
>
>Signed-off-by: Elijah Newren <newren@gmail.com>
>---
> Documentation/git-merge-tree.txt |  6 ++++--
> builtin/merge-tree.c             | 18 ++++++++++++++++--
> t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
> 3 files changed, 38 insertions(+), 4 deletions(-)
>
>diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
>index 5823938937f..4d5857b390b 100644
>--- a/Documentation/git-merge-tree.txt
>+++ b/Documentation/git-merge-tree.txt
>@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree
> SYNOPSIS
> --------
> [verse]
>-'git merge-tree' --real <branch1> <branch2>
>+'git merge-tree' --real [--messages=<file>] <branch1> <branch2>
> 'git merge-tree' <base-tree> <branch1> <branch2>
>
> DESCRIPTION
>@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive
> merge with rename detection.  If the merge is clean, the exit status
> will be `0`, and if the merge has conflicts, the exit status will be
> `1`.  The output will consist solely of the resulting toplevel tree
>-(which may have files including conflict markers).
>+(which may have files including conflict markers).  With `--messages`,
>+it will write any informational messages (such as "Auto-merging
>+<path>" and conflict notices) to the given file.
>
> The second form is meant for backward compatibility and will only do a
> trival merge.  It reads three tree-ish, and outputs trivial merge
>diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
>index c5757bed5bb..47deef0b199 100644
>--- a/builtin/merge-tree.c
>+++ b/builtin/merge-tree.c
>@@ -389,6 +389,7 @@ static int trivial_merge(const char *base,
>
> struct merge_tree_options {
> 	int real;
>+	char *messages_file;
> };
>
> static int real_merge(struct merge_tree_options *o,
>@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
> 	 */
>
> 	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
>+
>+	if (o->messages_file) {
>+		FILE *fp = xfopen(o->messages_file, "w");
>+		merge_display_update_messages(&opt, &result, fp);
>+		fclose(fp);

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

>+	}
> 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
>-	merge_switch_to_result(&opt, NULL, &result, 0, 0);
>+
>+	merge_finalize(&opt, &result);
> 	return result.clean ? 0 : 1;
> }
>
>@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> 	struct merge_tree_options o = { 0 };
> 	int expected_remaining_argc;
>+	int original_argc;
>
> 	const char * const merge_tree_usage[] = {
>-		N_("git merge-tree --real <branch1> <branch2>"),
>+		N_("git merge-tree --real [<options>] <branch1> <branch2>"),
> 		N_("git merge-tree <base-tree> <branch1> <branch2>"),
> 		NULL
> 	};
> 	struct option mt_options[] = {
> 		OPT_BOOL(0, "real", &o.real,
> 			 N_("do a real merge instead of a trivial merge")),
>+		OPT_STRING(0, "messages", &o.messages_file, N_("file"),
>+			   N_("filename to write informational/conflict messages to")),
> 		OPT_END()
> 	};
>
>@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> 		usage_with_options(merge_tree_usage, mt_options);
>
> 	/* Parse arguments */
>+	original_argc = argc;
> 	argc = parse_options(argc, argv, prefix, mt_options,
> 			     merge_tree_usage, 0);
>+	if (!o.real && original_argc < argc)
>+		die(_("--real must be specified if any other options are"));
> 	expected_remaining_argc = (o.real ? 2 : 3);
> 	if (argc != expected_remaining_argc)
> 		usage_with_options(merge_tree_usage, mt_options);
>diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
>index 9fb617ccc7f..42218cdc019 100755
>--- a/t/t4301-merge-tree-real.sh
>+++ b/t/t4301-merge-tree-real.sh
>@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
> 	grep "^usage: git merge-tree" expect
> '
>
>+test_expect_success '--messages gives us the conflict notices and such' '
>+	test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&
>+
>+	# Expected results:
>+	#   "greeting" should merge with conflicts
>+	#   "numbers" should merge cleanly
>+	#   "whatever" has *both* a modify/delete and a file/directory conflict
>+	cat <<-EOF >expect &&
>+	Auto-merging greeting
>+	CONFLICT (content): Merge conflict in greeting
>+	Auto-merging numbers
>+	CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead.
>+	CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1.  Version side1 of whatever~side1 left in tree.
>+	EOF
>+
>+	test_cmp expect MSG_FILE
>+'
>+
> test_done
>-- 
>gitgitgadget
>

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 7/8] merge-tree: support saving merge messages to a separate file
  2021-12-31  5:04 ` [PATCH 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
  2022-01-03 12:31   ` Fabian Stelzer
@ 2022-01-03 12:35   ` Fabian Stelzer
  2022-01-03 16:55     ` Elijah Newren
  1 sibling, 1 reply; 57+ messages in thread
From: Fabian Stelzer @ 2022-01-03 12:35 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Elijah Newren

On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
>From: Elijah Newren <newren@gmail.com>
>
>When running `git merge-tree --real`, we previously would only return an
>exit status reflecting the cleanness of a merge, and print out the
>toplevel tree of the resulting merge.  Merges also have informational
>messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT
>(file/directory)", etc.)  In fact, when non-content conflicts occur
>(such as file/directory, modify/delete, add/add with differing modes,
>rename/rename (1to2), etc.), these informational messages are often the
>only notification since these conflicts are not representable in the
>contents of the file.
>
>Add a --messages option which names a file so that callers can request
>these messages be recorded somewhere.
>
>Signed-off-by: Elijah Newren <newren@gmail.com>
>---
> Documentation/git-merge-tree.txt |  6 ++++--
> builtin/merge-tree.c             | 18 ++++++++++++++++--
> t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
> 3 files changed, 38 insertions(+), 4 deletions(-)
>
>diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
>index 5823938937f..4d5857b390b 100644
>--- a/Documentation/git-merge-tree.txt
>+++ b/Documentation/git-merge-tree.txt
>@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree
> SYNOPSIS
> --------
> [verse]
>-'git merge-tree' --real <branch1> <branch2>
>+'git merge-tree' --real [--messages=<file>] <branch1> <branch2>
> 'git merge-tree' <base-tree> <branch1> <branch2>
>
> DESCRIPTION
>@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive
> merge with rename detection.  If the merge is clean, the exit status
> will be `0`, and if the merge has conflicts, the exit status will be
> `1`.  The output will consist solely of the resulting toplevel tree
>-(which may have files including conflict markers).
>+(which may have files including conflict markers).  With `--messages`,
>+it will write any informational messages (such as "Auto-merging
>+<path>" and conflict notices) to the given file.
>
> The second form is meant for backward compatibility and will only do a
> trival merge.  It reads three tree-ish, and outputs trivial merge
>diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
>index c5757bed5bb..47deef0b199 100644
>--- a/builtin/merge-tree.c
>+++ b/builtin/merge-tree.c
>@@ -389,6 +389,7 @@ static int trivial_merge(const char *base,
>
> struct merge_tree_options {
> 	int real;
>+	char *messages_file;
> };
>
> static int real_merge(struct merge_tree_options *o,
>@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
> 	 */
>
> 	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
>+
>+	if (o->messages_file) {
>+		FILE *fp = xfopen(o->messages_file, "w");
>+		merge_display_update_messages(&opt, &result, fp);
>+		fclose(fp);
>+	}

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

> 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
>-	merge_switch_to_result(&opt, NULL, &result, 0, 0);
>+
>+	merge_finalize(&opt, &result);
> 	return result.clean ? 0 : 1;
> }
>
>@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> 	struct merge_tree_options o = { 0 };
> 	int expected_remaining_argc;
>+	int original_argc;
>
> 	const char * const merge_tree_usage[] = {
>-		N_("git merge-tree --real <branch1> <branch2>"),
>+		N_("git merge-tree --real [<options>] <branch1> <branch2>"),
> 		N_("git merge-tree <base-tree> <branch1> <branch2>"),
> 		NULL
> 	};
> 	struct option mt_options[] = {
> 		OPT_BOOL(0, "real", &o.real,
> 			 N_("do a real merge instead of a trivial merge")),
>+		OPT_STRING(0, "messages", &o.messages_file, N_("file"),
>+			   N_("filename to write informational/conflict messages to")),
> 		OPT_END()
> 	};
>
>@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> 		usage_with_options(merge_tree_usage, mt_options);
>
> 	/* Parse arguments */
>+	original_argc = argc;
> 	argc = parse_options(argc, argv, prefix, mt_options,
> 			     merge_tree_usage, 0);
>+	if (!o.real && original_argc < argc)
>+		die(_("--real must be specified if any other options are"));
> 	expected_remaining_argc = (o.real ? 2 : 3);
> 	if (argc != expected_remaining_argc)
> 		usage_with_options(merge_tree_usage, mt_options);
>diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
>index 9fb617ccc7f..42218cdc019 100755
>--- a/t/t4301-merge-tree-real.sh
>+++ b/t/t4301-merge-tree-real.sh
>@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
> 	grep "^usage: git merge-tree" expect
> '
>
>+test_expect_success '--messages gives us the conflict notices and such' '
>+	test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&
>+
>+	# Expected results:
>+	#   "greeting" should merge with conflicts
>+	#   "numbers" should merge cleanly
>+	#   "whatever" has *both* a modify/delete and a file/directory conflict
>+	cat <<-EOF >expect &&
>+	Auto-merging greeting
>+	CONFLICT (content): Merge conflict in greeting
>+	Auto-merging numbers
>+	CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead.
>+	CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1.  Version side1 of whatever~side1 left in tree.
>+	EOF
>+
>+	test_cmp expect MSG_FILE
>+'
>+
> test_done
>-- 
>gitgitgadget
>

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 4/8] merge-tree: implement real merges
  2022-01-03 12:23   ` Fabian Stelzer
@ 2022-01-03 16:37     ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-03 16:37 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau

On Mon, Jan 3, 2022 at 4:23 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>
> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
> >From: Elijah Newren <newren@gmail.com>
> >
> >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.
> >
...
> >+test_expect_success setup '
> >+      test_write_lines 1 2 3 4 5 >numbers &&
> >+      echo hello >greeting &&
> >+      echo foo >whatever &&
> >+      git add numbers greeting whatever &&
> >+      git commit -m initial &&
> >+
> >+      git branch side1 &&
> >+      git branch side2 &&
> >+
> >+      git checkout side1 &&
> >+      test_write_lines 1 2 3 4 5 6 >numbers &&
> >+      echo hi >greeting &&
> >+      echo bar >whatever &&
> >+      git add numbers greeting whatever &&
> >+      git commit -m rename-and-modify &&
>
> The commit implies a rename as well which I think is missing.

Sorry, I revised the testcase (multiple times) and forgot to update
this commit message string.

> >+
> >+      git checkout side2 &&
> >+      test_write_lines 0 1 2 3 4 5 >numbers &&
> >+      echo yo >greeting &&
> >+      git rm whatever &&
> >+      mkdir whatever &&
> >+      >whatever/empty &&
> >+      git add numbers greeting whatever/empty &&
> >+      git commit -m remove-and-rename
>
> And this looks more like a remove-and-modify.

Likewise.


I'll fix these up; thanks for pointing them out.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 7/8] merge-tree: support saving merge messages to a separate file
  2022-01-03 12:31   ` Fabian Stelzer
@ 2022-01-03 16:51     ` Elijah Newren
  2022-01-03 17:22       ` Fabian Stelzer
  0 siblings, 1 reply; 57+ messages in thread
From: Elijah Newren @ 2022-01-03 16:51 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau

On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>
> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
> >From: Elijah Newren <newren@gmail.com>
> >
> >When running `git merge-tree --real`, we previously would only return an
> >exit status reflecting the cleanness of a merge, and print out the
> >toplevel tree of the resulting merge.  Merges also have informational
> >messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT
> >(file/directory)", etc.)  In fact, when non-content conflicts occur
> >(such as file/directory, modify/delete, add/add with differing modes,
> >rename/rename (1to2), etc.), these informational messages are often the
> >only notification since these conflicts are not representable in the
> >contents of the file.
> >
> >Add a --messages option which names a file so that callers can request
> >these messages be recorded somewhere.
> >
> >Signed-off-by: Elijah Newren <newren@gmail.com>
> >---
> > Documentation/git-merge-tree.txt |  6 ++++--
> > builtin/merge-tree.c             | 18 ++++++++++++++++--
> > t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
> > 3 files changed, 38 insertions(+), 4 deletions(-)
> >
> >diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> >index 5823938937f..4d5857b390b 100644
> >--- a/Documentation/git-merge-tree.txt
> >+++ b/Documentation/git-merge-tree.txt
> >@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree
> > SYNOPSIS
> > --------
> > [verse]
> >-'git merge-tree' --real <branch1> <branch2>
> >+'git merge-tree' --real [--messages=<file>] <branch1> <branch2>
> > 'git merge-tree' <base-tree> <branch1> <branch2>
> >
> > DESCRIPTION
> >@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive
> > merge with rename detection.  If the merge is clean, the exit status
> > will be `0`, and if the merge has conflicts, the exit status will be
> > `1`.  The output will consist solely of the resulting toplevel tree
> >-(which may have files including conflict markers).
> >+(which may have files including conflict markers).  With `--messages`,
> >+it will write any informational messages (such as "Auto-merging
> >+<path>" and conflict notices) to the given file.
> >
> > The second form is meant for backward compatibility and will only do a
> > trival merge.  It reads three tree-ish, and outputs trivial merge
> >diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> >index c5757bed5bb..47deef0b199 100644
> >--- a/builtin/merge-tree.c
> >+++ b/builtin/merge-tree.c
> >@@ -389,6 +389,7 @@ static int trivial_merge(const char *base,
> >
> > struct merge_tree_options {
> >       int real;
> >+      char *messages_file;
> > };
> >
> > static int real_merge(struct merge_tree_options *o,
> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
> >        */
> >
> >       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> >+
> >+      if (o->messages_file) {
> >+              FILE *fp = xfopen(o->messages_file, "w");
> >+              merge_display_update_messages(&opt, &result, fp);
> >+              fclose(fp);
>
> I don't know enough about how merge-ort works internally, but it looks to me
> like at this point the merge already happened and we just didn't clean up
> (finalize) yet. It feels wrong to die() at this point just because we can't
> open messages_file.

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

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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 7/8] merge-tree: support saving merge messages to a separate file
  2022-01-03 12:35   ` Fabian Stelzer
@ 2022-01-03 16:55     ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-03 16:55 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau

On Mon, Jan 3, 2022 at 4:35 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>
> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
> >From: Elijah Newren <newren@gmail.com>
> >
> >When running `git merge-tree --real`, we previously would only return an
> >exit status reflecting the cleanness of a merge, and print out the
> >toplevel tree of the resulting merge.  Merges also have informational
> >messages, ("Auto-merging <PATH>", "CONFLICT (content): ...", "CONFLICT
> >(file/directory)", etc.)  In fact, when non-content conflicts occur
> >(such as file/directory, modify/delete, add/add with differing modes,
> >rename/rename (1to2), etc.), these informational messages are often the
> >only notification since these conflicts are not representable in the
> >contents of the file.
> >
> >Add a --messages option which names a file so that callers can request
> >these messages be recorded somewhere.
> >
> >Signed-off-by: Elijah Newren <newren@gmail.com>
> >---
> > Documentation/git-merge-tree.txt |  6 ++++--
> > builtin/merge-tree.c             | 18 ++++++++++++++++--
> > t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
> > 3 files changed, 38 insertions(+), 4 deletions(-)
> >
> >diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> >index 5823938937f..4d5857b390b 100644
> >--- a/Documentation/git-merge-tree.txt
> >+++ b/Documentation/git-merge-tree.txt
> >@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree
> > SYNOPSIS
> > --------
> > [verse]
> >-'git merge-tree' --real <branch1> <branch2>
> >+'git merge-tree' --real [--messages=<file>] <branch1> <branch2>
> > 'git merge-tree' <base-tree> <branch1> <branch2>
> >
> > DESCRIPTION
> >@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive
> > merge with rename detection.  If the merge is clean, the exit status
> > will be `0`, and if the merge has conflicts, the exit status will be
> > `1`.  The output will consist solely of the resulting toplevel tree
> >-(which may have files including conflict markers).
> >+(which may have files including conflict markers).  With `--messages`,
> >+it will write any informational messages (such as "Auto-merging
> >+<path>" and conflict notices) to the given file.
> >
> > The second form is meant for backward compatibility and will only do a
> > trival merge.  It reads three tree-ish, and outputs trivial merge
> >diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> >index c5757bed5bb..47deef0b199 100644
> >--- a/builtin/merge-tree.c
> >+++ b/builtin/merge-tree.c
> >@@ -389,6 +389,7 @@ static int trivial_merge(const char *base,
> >
> > struct merge_tree_options {
> >       int real;
> >+      char *messages_file;
> > };
> >
> > static int real_merge(struct merge_tree_options *o,
> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
> >        */
> >
> >       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> >+
> >+      if (o->messages_file) {
> >+              FILE *fp = xfopen(o->messages_file, "w");
> >+              merge_display_update_messages(&opt, &result, fp);
> >+              fclose(fp);
> >+      }
>
> Something else I just wondered. Can the user differentiate between the die()
> in xfopen() and a failed/unclean merge?
> Both just exit(1) don't they?

xfopen() calls die_errno(), which calls die_routine(), which will be
pointing at die_builtin() since we don't change it in
builtin/merge-tree.c, and die_builtin() calls exit(128).

So, a different error code.

But good question...perhaps I should mention exit codes other than 0
and 1 in the documentation of merge-tree for other failures.

>
> >       printf("%s\n", oid_to_hex(&result.tree->object.oid));
> >-      merge_switch_to_result(&opt, NULL, &result, 0, 0);
> >+
> >+      merge_finalize(&opt, &result);
> >       return result.clean ? 0 : 1;
> > }
> >
> >@@ -451,15 +459,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> > {
> >       struct merge_tree_options o = { 0 };
> >       int expected_remaining_argc;
> >+      int original_argc;
> >
> >       const char * const merge_tree_usage[] = {
> >-              N_("git merge-tree --real <branch1> <branch2>"),
> >+              N_("git merge-tree --real [<options>] <branch1> <branch2>"),
> >               N_("git merge-tree <base-tree> <branch1> <branch2>"),
> >               NULL
> >       };
> >       struct option mt_options[] = {
> >               OPT_BOOL(0, "real", &o.real,
> >                        N_("do a real merge instead of a trivial merge")),
> >+              OPT_STRING(0, "messages", &o.messages_file, N_("file"),
> >+                         N_("filename to write informational/conflict messages to")),
> >               OPT_END()
> >       };
> >
> >@@ -468,8 +479,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> >               usage_with_options(merge_tree_usage, mt_options);
> >
> >       /* Parse arguments */
> >+      original_argc = argc;
> >       argc = parse_options(argc, argv, prefix, mt_options,
> >                            merge_tree_usage, 0);
> >+      if (!o.real && original_argc < argc)
> >+              die(_("--real must be specified if any other options are"));
> >       expected_remaining_argc = (o.real ? 2 : 3);
> >       if (argc != expected_remaining_argc)
> >               usage_with_options(merge_tree_usage, mt_options);
> >diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> >index 9fb617ccc7f..42218cdc019 100755
> >--- a/t/t4301-merge-tree-real.sh
> >+++ b/t/t4301-merge-tree-real.sh
> >@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
> >       grep "^usage: git merge-tree" expect
> > '
> >
> >+test_expect_success '--messages gives us the conflict notices and such' '
> >+      test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&
> >+
> >+      # Expected results:
> >+      #   "greeting" should merge with conflicts
> >+      #   "numbers" should merge cleanly
> >+      #   "whatever" has *both* a modify/delete and a file/directory conflict
> >+      cat <<-EOF >expect &&
> >+      Auto-merging greeting
> >+      CONFLICT (content): Merge conflict in greeting
> >+      Auto-merging numbers
> >+      CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead.
> >+      CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1.  Version side1 of whatever~side1 left in tree.
> >+      EOF
> >+
> >+      test_cmp expect MSG_FILE
> >+'
> >+
> > test_done
> >--
> >gitgitgadget
> >

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 7/8] merge-tree: support saving merge messages to a separate file
  2022-01-03 16:51     ` Elijah Newren
@ 2022-01-03 17:22       ` Fabian Stelzer
  2022-01-03 19:46         ` Elijah Newren
  0 siblings, 1 reply; 57+ messages in thread
From: Fabian Stelzer @ 2022-01-03 17:22 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau

On 03.01.2022 08:51, Elijah Newren wrote:
>On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>>
>> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
>> >From: Elijah Newren <newren@gmail.com>
[...]
>> >
>> > static int real_merge(struct merge_tree_options *o,
>> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
>> >        */
>> >
>> >       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
>> >+
>> >+      if (o->messages_file) {
>> >+              FILE *fp = xfopen(o->messages_file, "w");
>> >+              merge_display_update_messages(&opt, &result, fp);
>> >+              fclose(fp);
>>
>> I don't know enough about how merge-ort works internally, but it looks to me
>> like at this point the merge already happened and we just didn't clean up
>> (finalize) yet. It feels wrong to die() at this point just because we can't
>> open messages_file.
>
>Yes, the merge already happened; there now exists a new toplevel tree
>(that nothing references).  I'm not sure I understand what's wrong
>with die'ing here, though.  I can't tell if you want to defer the
>die-ing until later, or just avoid the die-ing and return some kind of
>success despite failing to complete what the user requested.
>

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

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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 7/8] merge-tree: support saving merge messages to a separate file
  2022-01-03 17:22       ` Fabian Stelzer
@ 2022-01-03 19:46         ` Elijah Newren
  2022-01-04 13:05           ` Fabian Stelzer
  0 siblings, 1 reply; 57+ messages in thread
From: Elijah Newren @ 2022-01-03 19:46 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau

On Mon, Jan 3, 2022 at 9:23 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>
> On 03.01.2022 08:51, Elijah Newren wrote:
> >On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> >>
> >> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
> >> >From: Elijah Newren <newren@gmail.com>
> [...]
> >> >
> >> > static int real_merge(struct merge_tree_options *o,
> >> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
> >> >        */
> >> >
> >> >       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> >> >+
> >> >+      if (o->messages_file) {
> >> >+              FILE *fp = xfopen(o->messages_file, "w");
> >> >+              merge_display_update_messages(&opt, &result, fp);
> >> >+              fclose(fp);
> >>
> >> I don't know enough about how merge-ort works internally, but it looks to me
> >> like at this point the merge already happened and we just didn't clean up
> >> (finalize) yet. It feels wrong to die() at this point just because we can't
> >> open messages_file.
> >
> >Yes, the merge already happened; there now exists a new toplevel tree
> >(that nothing references).  I'm not sure I understand what's wrong
> >with die'ing here, though.  I can't tell if you want to defer the
> >die-ing until later, or just avoid the die-ing and return some kind of
> >success despite failing to complete what the user requested.
> >
>
> I think i would prefer the merge operation to abort before actually merging
> when not being able to write its logfile. Otherwise we possibly do a whole
> lot of work that`s inaccessible afterwards isn't it? (since we don`t print
> the hash)

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

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

merge-ort is pretty snappy.  For a simple example of rebasing a single
patch in linux.git across a branch with 28000 renames, I get 176
milliseconds for merge_incore_nonrecursive().  Granted, linux.git is
pretty small in terms of number of files, but Stolee did some
measurements a while back on the Microsoft repos with millions of
files at HEAD.  For those, for a trivial merge he saw
merge_incore_recursive() complete in 2 milliseconds, and for a trivial
rebase he saw merge_incore_nonrecursive() complete in 4 milliseconds
(See https://lore.kernel.org/git/CABPp-BHO7bZ3H7A=E9TudhvBoNfwPvRiDMm8S9kq3mYeSXrpXw@mail.gmail.com/).
So huge numbers of files pose much less of a problem than lots of
interesting work like renames, and merge-ort is pretty fast in either
case.  Sure, if we were talking about traditional merge-recursive
which would have taken 150000 milliseconds on the same single patch in
linux.git testcase (due to the 28000 renames), then we might worry
more about not letting work get tossed, but at only 176 milliseconds
even with a crazy number of renames, it's just not worth worrying
about.

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

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

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


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

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

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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 7/8] merge-tree: support saving merge messages to a separate file
  2022-01-03 19:46         ` Elijah Newren
@ 2022-01-04 13:05           ` Fabian Stelzer
  0 siblings, 0 replies; 57+ messages in thread
From: Fabian Stelzer @ 2022-01-04 13:05 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau

On 03.01.2022 11:46, Elijah Newren wrote:
>On Mon, Jan 3, 2022 at 9:23 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>>
>> On 03.01.2022 08:51, Elijah Newren wrote:
>> >On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> >>
>> >> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
>> >> >From: Elijah Newren <newren@gmail.com>
>> [...]
>> >> >
>> >> > static int real_merge(struct merge_tree_options *o,
>> >> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
>> >> >        */
>> >> >
>> >> >       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
>> >> >+
>> >> >+      if (o->messages_file) {
>> >> >+              FILE *fp = xfopen(o->messages_file, "w");
>> >> >+              merge_display_update_messages(&opt, &result, fp);
>> >> >+              fclose(fp);
>> >>
>> >> I don't know enough about how merge-ort works internally, but it looks to me
>> >> like at this point the merge already happened and we just didn't clean up
>> >> (finalize) yet. It feels wrong to die() at this point just because we can't
>> >> open messages_file.
>> >
>> >Yes, the merge already happened; there now exists a new toplevel tree
>> >(that nothing references).  I'm not sure I understand what's wrong
>> >with die'ing here, though.  I can't tell if you want to defer the
>> >die-ing until later, or just avoid the die-ing and return some kind of
>> >success despite failing to complete what the user requested.
>> >
>>
>> I think i would prefer the merge operation to abort before actually merging
>> when not being able to write its logfile. Otherwise we possibly do a whole
>> lot of work that`s inaccessible afterwards isn't it? (since we don`t print
>> the hash)
>
>I see where you're coming from, but I don't see this as worth worrying
>about.  For two reasons:
>
>(1) I'm not sure I buy the "whole lot of work" concern.
>
>merge-ort is pretty snappy.  For a simple example of rebasing a single
>patch in linux.git across a branch with 28000 renames, I get 176
>milliseconds for merge_incore_nonrecursive().  Granted, linux.git is
>pretty small in terms of number of files, but Stolee did some
>measurements a while back on the Microsoft repos with millions of
>files at HEAD.  For those, for a trivial merge he saw
>merge_incore_recursive() complete in 2 milliseconds, and for a trivial
>rebase he saw merge_incore_nonrecursive() complete in 4 milliseconds
>(See https://lore.kernel.org/git/CABPp-BHO7bZ3H7A=E9TudhvBoNfwPvRiDMm8S9kq3mYeSXrpXw@mail.gmail.com/).
>So huge numbers of files pose much less of a problem than lots of
>interesting work like renames, and merge-ort is pretty fast in either
>case.  Sure, if we were talking about traditional merge-recursive
>which would have taken 150000 milliseconds on the same single patch in
>linux.git testcase (due to the 28000 renames), then we might worry
>more about not letting work get tossed, but at only 176 milliseconds
>even with a crazy number of renames, it's just not worth worrying
>about.

I guess I'm just not used to how good ort is yet. I was still expecting 
cases like the merge-recursive ones :)

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

Completely understandable for the regular merge. In this case we are not 
touching the index/working tree at all though so I don't quite see how this 
is comparable.

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

Especially since the extra work penalty is so miniscule i agree.

Thanks

>
>> Thanks for your work on this feature. I think this could open a lot of new
>> possibilities.
>
>I hope people do interesting things with it, and with the server-side
>commit replaying I'm working on as well.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index)
  2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                   ` (7 preceding siblings ...)
  2021-12-31  5:04 ` [PATCH 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
@ 2022-01-05 17:27 ` Elijah Newren via GitGitGadget
  2022-01-05 17:27   ` [PATCH v2 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
                     ` (9 more replies)
  8 siblings, 10 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-01-05 17:27 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren

(NOTE for Junio: This series has a minor conflict with en/remerge-diff --
this series moves a code block into a new function, but en/remerge-diff adds
a BUG() message to that code block. But this series is just RFC, so you may
want to wait to pick it up.)

Updates since v1:

 * Fixed a bad patch splitting, and a style issue pointed out by Johannes
   Altimanninger
 * Fixed misleading commit messages in new test cases
 * Fixed my comments about how commit-tree could be used to correctly use
   two -p flags

NOTE2: A preliminary version of this series was discussed here:
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110211147490.56@tvgsbejvaqbjf.bet/

NOTE3: An alternative has been implemented by Christian, over here:
https://lore.kernel.org/git/20220105163324.73369-1-chriscool@tuxfamily.org/

This series introduces a new option to git-merge-tree: --real (best name I
could come up with). This new option is designed to allow a server-side
"real" merge (or allow folks client-side to do merges with branches they
don't even have checked out). Real merges differ from trivial merges in that
they handle:

 * three way content merges
 * recursive ancestor consolidation
 * renames
 * proper directory/file conflict handling
 * etc.

The reason this is different from merge is that merge-tree does NOT:

 * Read/write/update any working tree (and assumes there probably isn't one)
 * Read/write/update any index (and assumes there probably isn't one)
 * Create a commit object
 * Update any refs

This series attempts to guess what kind of output would be wanted, basically
choosing:

 * clean merge or conflict signalled via exit status
 * stdout consists solely of printing the hash of the resulting tree (though
   that tree may include files that have conflict markers)
 * new optional --messages flag for specifying a file where informational
   messages (e.g. conflict notices and files involved in three-way-content
   merges) can be written; by default, this output is simply discarded
 * new optional --conflicted-list flag for specifying a file where the names
   of conflicted-files can be written in a NUL-character-separated list

This design means it's basically just a low-level tool that other scripts
would use and do additional work with. Perhaps something like this:

   NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
   test $? -eq 0 || die "There were conflicts..."
   NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 -p $BRANCH2)
   git update-ref $BRANCH1 $NEWCOMMIT


Elijah Newren (8):
  merge-tree: rename merge_trees() to trivial_merge_trees()
  merge-tree: move logic for existing merge into new function
  merge-tree: add option parsing and initial shell for real merge
    function
  merge-tree: implement real merges
  merge-ort: split out a separate display_update_messages() function
  merge-ort: allow update messages to be written to different file
    stream
  merge-tree: support saving merge messages to a separate file
  merge-tree: provide an easy way to access which files have conflicts

 Documentation/git-merge-tree.txt |  32 +++++--
 builtin/merge-tree.c             | 151 ++++++++++++++++++++++++++++---
 git.c                            |   2 +-
 merge-ort.c                      |  85 ++++++++++-------
 merge-ort.h                      |  12 +++
 t/t4301-merge-tree-real.sh       | 108 ++++++++++++++++++++++
 6 files changed, 333 insertions(+), 57 deletions(-)
 create mode 100755 t/t4301-merge-tree-real.sh


base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1114%2Fnewren%2Fmerge-into-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1114/newren/merge-into-v2
Pull-Request: https://github.com/git/git/pull/1114

Range-diff vs v1:

 1:  a7c7910d834 = 1:  a7c7910d834 merge-tree: rename merge_trees() to trivial_merge_trees()
 2:  9da8e77c1d7 ! 2:  aafe67d7c69 merge-tree: move logic for existing merge into new function
     @@ builtin/merge-tree.c: static void *get_tree_descriptor(struct repository *r,
       }
       
      -int cmd_merge_tree(int argc, const char **argv, const char *prefix)
     --{
     -+static int trivial_merge(int argc, const char **argv) {
     ++static int trivial_merge(int argc, const char **argv)
     + {
       	struct repository *r = the_repository;
       	struct tree_desc t[3];
       	void *buf1, *buf2, *buf3;
 3:  9d03d3f56ab ! 3:  ee21aed0115 merge-tree: add option parsing and initial shell for real merge function
     @@ builtin/merge-tree.c: static void *get_tree_descriptor(struct repository *r,
       	return buf;
       }
       
     --static int trivial_merge(int argc, const char **argv) {
     +-static int trivial_merge(int argc, const char **argv)
      +static int trivial_merge(const char *base,
      +			 const char *branch1,
     -+			 const char *branch2) {
     ++			 const char *branch2)
     + {
       	struct repository *r = the_repository;
       	struct tree_desc t[3];
       	void *buf1, *buf2, *buf3;
     @@ builtin/merge-tree.c: static void *get_tree_descriptor(struct repository *r,
       	trivial_merge_trees(t, "");
       	free(buf1);
       	free(buf2);
     -@@ builtin/merge-tree.c: static int trivial_merge(int argc, const char **argv) {
     +@@ builtin/merge-tree.c: static int trivial_merge(int argc, const char **argv)
       	return 0;
       }
       
 4:  9fc71f4511b ! 4:  1710ba4a9e4 merge-tree: implement real merges
     @@ Commit message
      
             NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
             test $? -eq 0 || die "There were conflicts..."
     -       NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 $BRANCH2)
     +       NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 -p $BRANCH2)
             git update-ref $BRANCH1 $NEWCOMMIT
      
          Note that higher level scripts may also want to access the
     @@ t/t4301-merge-tree-real.sh (new)
      +	echo hi >greeting &&
      +	echo bar >whatever &&
      +	git add numbers greeting whatever &&
     -+	git commit -m rename-and-modify &&
     ++	git commit -m modify-stuff &&
      +
      +	git checkout side2 &&
      +	test_write_lines 0 1 2 3 4 5 >numbers &&
     @@ t/t4301-merge-tree-real.sh (new)
      +	mkdir whatever &&
      +	>whatever/empty &&
      +	git add numbers greeting whatever/empty &&
     -+	git commit -m remove-and-rename
     ++	git commit -m other-modifications
      +'
      +
      +test_expect_success 'Content merge and a few conflicts' '
 5:  aa816e766e9 ! 5:  bc6d01f1a0e merge-ort: split out a separate display_update_messages() function
     @@ merge-ort.c: void merge_switch_to_result(struct merge_options *opt,
      -
      -		trace2_region_leave("merge", "display messages", opt->repo);
      -	}
     ++	if (display_update_msgs)
     ++		merge_display_update_messages(opt, result);
       
       	merge_finalize(opt, result);
       }
 6:  32ad5b5c10d ! 6:  c9e95a70d19 merge-ort: allow update messages to be written to different file stream
     @@ merge-ort.c: void merge_display_update_messages(struct merge_options *opt,
       	string_list_clear(&olist, 0);
       
      @@ merge-ort.c: void merge_switch_to_result(struct merge_options *opt,
     - 		trace2_region_leave("merge", "write_auto_merge", opt->repo);
       	}
       
     -+	if (display_update_msgs)
     + 	if (display_update_msgs)
     +-		merge_display_update_messages(opt, result);
      +		merge_display_update_messages(opt, result, stdout);
       
       	merge_finalize(opt, result);
 7:  777de92d9f1 = 7:  4b513a6d696 merge-tree: support saving merge messages to a separate file
 8:  1d24a4f4070 = 8:  01364bb020e merge-tree: provide an easy way to access which files have conflicts

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 1/8] merge-tree: rename merge_trees() to trivial_merge_trees()
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
@ 2022-01-05 17:27   ` Elijah Newren via GitGitGadget
  2022-01-05 17:27   ` [PATCH v2 2/8] merge-tree: move logic for existing merge into new function Elijah Newren via GitGitGadget
                     ` (8 subsequent siblings)
  9 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-01-05 17:27 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

merge-recursive.h defined its own merge_trees() function, different than
the one found in builtin/merge-tree.c.  That was okay in the past, but
we want merge-tree to be able to use the merge-ort functions, which will
end up including merge-recursive.h.  Rename the function found in
builtin/merge-tree.c to avoid the conflict.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/merge-tree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 5dc94d6f880..06f9eee9f78 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -28,7 +28,7 @@ static void add_merge_entry(struct merge_list *entry)
 	merge_result_end = &entry->next;
 }
 
-static void merge_trees(struct tree_desc t[3], const char *base);
+static void trivial_merge_trees(struct tree_desc t[3], const char *base);
 
 static const char *explanation(struct merge_list *entry)
 {
@@ -225,7 +225,7 @@ static void unresolved_directory(const struct traverse_info *info,
 	buf2 = fill_tree_descriptor(r, t + 2, ENTRY_OID(n + 2));
 #undef ENTRY_OID
 
-	merge_trees(t, newbase);
+	trivial_merge_trees(t, newbase);
 
 	free(buf0);
 	free(buf1);
@@ -342,7 +342,7 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s
 	return mask;
 }
 
-static void merge_trees(struct tree_desc t[3], const char *base)
+static void trivial_merge_trees(struct tree_desc t[3], const char *base)
 {
 	struct traverse_info info;
 
@@ -378,7 +378,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 	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]);
-	merge_trees(t, "");
+	trivial_merge_trees(t, "");
 	free(buf1);
 	free(buf2);
 	free(buf3);
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 2/8] merge-tree: move logic for existing merge into new function
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
  2022-01-05 17:27   ` [PATCH v2 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
@ 2022-01-05 17:27   ` Elijah Newren via GitGitGadget
  2022-01-05 17:27   ` [PATCH v2 3/8] merge-tree: add option parsing and initial shell for real merge function Elijah Newren via GitGitGadget
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-01-05 17:27 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/merge-tree.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 06f9eee9f78..914ec960b7e 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -366,15 +366,12 @@ static void *get_tree_descriptor(struct repository *r,
 	return buf;
 }
 
-int cmd_merge_tree(int argc, const char **argv, const char *prefix)
+static int trivial_merge(int argc, const char **argv)
 {
 	struct repository *r = the_repository;
 	struct tree_desc t[3];
 	void *buf1, *buf2, *buf3;
 
-	if (argc != 4)
-		usage(merge_tree_usage);
-
 	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]);
@@ -386,3 +383,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 	show_result();
 	return 0;
 }
+
+int cmd_merge_tree(int argc, const char **argv, const char *prefix)
+{
+	if (argc != 4)
+		usage(merge_tree_usage);
+	return trivial_merge(argc, argv);
+}
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 3/8] merge-tree: add option parsing and initial shell for real merge function
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
  2022-01-05 17:27   ` [PATCH v2 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
  2022-01-05 17:27   ` [PATCH v2 2/8] merge-tree: move logic for existing merge into new function Elijah Newren via GitGitGadget
@ 2022-01-05 17:27   ` Elijah Newren via GitGitGadget
  2022-01-05 17:27   ` [PATCH v2 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
                     ` (6 subsequent siblings)
  9 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-01-05 17:27 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

Let merge-tree accept a `--real` parameter for choosing real merges
instead of trivial merges.  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 <newren@gmail.com>
---
 builtin/merge-tree.c | 56 +++++++++++++++++++++++++++++++++++++-------
 git.c                |  2 +-
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 914ec960b7e..e1d2832c809 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -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,46 @@ static int trivial_merge(int argc, const char **argv)
 	return 0;
 }
 
+struct merge_tree_options {
+	int real;
+};
+
+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 --real <branch1> <branch2>"),
+		N_("git merge-tree <base-tree> <branch1> <branch2>"),
+		NULL
+	};
+	struct option mt_options[] = {
+		OPT_BOOL(0, "real", &o.real,
+			 N_("do a real merge instead of a trivial merge")),
+		OPT_END()
+	};
+
+	/* Check for a request for basic help */
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(merge_tree_usage, mt_options);
+
+	/* Parse arguments */
+	argc = parse_options(argc, argv, prefix, mt_options,
+			     merge_tree_usage, 0);
+	expected_remaining_argc = (o.real ? 2 : 3);
+	if (argc != expected_remaining_argc)
+		usage_with_options(merge_tree_usage, mt_options);
+
+	/* 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]);
 }
diff --git a/git.c b/git.c
index 7edafd8ecff..0124c053878 100644
--- a/git.c
+++ b/git.c
@@ -561,7 +561,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 },
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 4/8] merge-tree: implement real merges
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                     ` (2 preceding siblings ...)
  2022-01-05 17:27   ` [PATCH v2 3/8] merge-tree: add option parsing and initial shell for real merge function Elijah Newren via GitGitGadget
@ 2022-01-05 17:27   ` Elijah Newren via GitGitGadget
  2022-01-07 15:30     ` Johannes Schindelin
  2022-01-07 18:12     ` Christian Couder
  2022-01-05 17:27   ` [PATCH v2 5/8] merge-ort: split out a separate display_update_messages() function Elijah Newren via GitGitGadget
                     ` (5 subsequent siblings)
  9 siblings, 2 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-01-05 17:27 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

This adds the ability to perform real merges rather than just trivial
merges (meaning handling three way content merges, recursive ancestor
consolidation, renames, proper directory/file conflict handling, and so
forth).  However, unlike `git merge`, the working tree and index are
left alone and no branch is updated.

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

This output is mean to be used by some higher level script, perhaps in a
sequence of steps like this:

   NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
   test $? -eq 0 || die "There were conflicts..."
   NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 -p $BRANCH2)
   git update-ref $BRANCH1 $NEWCOMMIT

Note that higher level scripts may also want to access the
conflict/warning messages normally output during a merge, or have quick
access to a list of files with conflicts.  That is not available in this
preliminary implementation, but subsequent commits will add that
ability.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-merge-tree.txt | 28 +++++++----
 builtin/merge-tree.c             | 55 +++++++++++++++++++++-
 t/t4301-merge-tree-real.sh       | 81 ++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+), 11 deletions(-)
 create mode 100755 t/t4301-merge-tree-real.sh

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index 58731c19422..5823938937f 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -3,26 +3,34 @@ git-merge-tree(1)
 
 NAME
 ----
-git-merge-tree - Show three-way merge without touching index
+git-merge-tree - Perform merge without touching index or working tree
 
 
 SYNOPSIS
 --------
 [verse]
+'git merge-tree' --real <branch1> <branch2>
 'git merge-tree' <base-tree> <branch1> <branch2>
 
 DESCRIPTION
 -----------
-Reads three tree-ish, and output trivial merge results and
-conflicting stages to the standard output.  This is similar to
-what three-way 'git read-tree -m' does, but instead of storing the
-results in the index, the command outputs the entries to the
-standard output.
+Performs a merge, but does not make any new commits and does not read
+from or write to either the working tree or index.
 
-This is meant to be used by higher level scripts to compute
-merge results outside of the index, and stuff the results back into the
-index.  For this reason, the output from the command omits
-entries that match the <branch1> tree.
+The first form will merge the two branches, doing a full recursive
+merge with rename detection.  If the merge is clean, the exit status
+will be `0`, and if the merge has conflicts, the exit status will be
+`1`.  The output will consist solely of the resulting toplevel tree
+(which may have files including conflict markers).
+
+The second form is meant for backward compatibility and will only do a
+trival merge.  It reads three tree-ish, and outputs trivial merge
+results and conflicting stages to the standard output in a semi-diff
+format.  Since this was designed for higher level scripts to consume
+and merge the results back into the index, it omits entries that match
+<branch1>.  The result of this second form is is similar to what
+three-way 'git read-tree -m' does, but instead of storing the results
+in the index, the command outputs the entries to the standard output.
 
 GIT
 ---
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index e1d2832c809..ac50f3d108b 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -2,6 +2,9 @@
 #include "builtin.h"
 #include "tree-walk.h"
 #include "xdiff-interface.h"
+#include "help.h"
+#include "commit-reach.h"
+#include "merge-ort.h"
 #include "object-store.h"
 #include "parse-options.h"
 #include "repository.h"
@@ -392,7 +395,57 @@ struct merge_tree_options {
 static int real_merge(struct merge_tree_options *o,
 		      const char *branch1, const char *branch2)
 {
-	die(_("real merges are not yet implemented"));
+	struct commit *parent1, *parent2;
+	struct commit_list *common;
+	struct commit_list *merge_bases = NULL;
+	struct commit_list *j;
+	struct merge_options opt;
+	struct merge_result result = { 0 };
+
+	parent1 = get_merge_parent(branch1);
+	if (!parent1)
+		help_unknown_ref(branch1, "merge",
+				 _("not something we can merge"));
+
+	parent2 = get_merge_parent(branch2);
+	if (!parent2)
+		help_unknown_ref(branch2, "merge",
+				 _("not something we can merge"));
+
+	init_merge_options(&opt, the_repository);
+	/*
+	 * TODO: Support subtree and other -X options?
+	if (use_strategies_nr == 1 &&
+	    !strcmp(use_strategies[0]->name, "subtree"))
+		opt.subtree_shift = "";
+	for (x = 0; x < xopts_nr; x++)
+		if (parse_merge_opt(&opt, xopts[x]))
+			die(_("Unknown strategy option: -X%s"), xopts[x]);
+	*/
+
+	opt.show_rename_progress = 0;
+
+	opt.branch1 = merge_remote_util(parent1)->name; /* or just branch1? */
+	opt.branch2 = merge_remote_util(parent2)->name; /* or just branch2? */
+
+	/*
+	 * Get the merge bases, in reverse order; see comment above
+	 * merge_incore_recursive in merge-ort.h
+	 */
+	common = get_merge_bases(parent1, parent2);
+	for (j = common; j; j = j->next)
+		commit_list_insert(j->item, &merge_bases);
+
+	/*
+	 * TODO: notify if merging unrelated histories?
+	if (!common)
+		fprintf(stderr, _("merging unrelated histories"));
+	 */
+
+	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+	printf("%s\n", oid_to_hex(&result.tree->object.oid));
+	merge_switch_to_result(&opt, NULL, &result, 0, 0);
+	return result.clean ? 0 : 1;
 }
 
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
new file mode 100755
index 00000000000..f7aa310f8c1
--- /dev/null
+++ b/t/t4301-merge-tree-real.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+
+test_description='git merge-tree --real'
+
+. ./test-lib.sh
+
+# This test is ort-specific
+GIT_TEST_MERGE_ALGORITHM=ort
+export GIT_TEST_MERGE_ALGORITHM
+
+test_expect_success setup '
+	test_write_lines 1 2 3 4 5 >numbers &&
+	echo hello >greeting &&
+	echo foo >whatever &&
+	git add numbers greeting whatever &&
+	git commit -m initial &&
+
+	git branch side1 &&
+	git branch side2 &&
+
+	git checkout side1 &&
+	test_write_lines 1 2 3 4 5 6 >numbers &&
+	echo hi >greeting &&
+	echo bar >whatever &&
+	git add numbers greeting whatever &&
+	git commit -m modify-stuff &&
+
+	git checkout side2 &&
+	test_write_lines 0 1 2 3 4 5 >numbers &&
+	echo yo >greeting &&
+	git rm whatever &&
+	mkdir whatever &&
+	>whatever/empty &&
+	git add numbers greeting whatever/empty &&
+	git commit -m other-modifications
+'
+
+test_expect_success 'Content merge and a few conflicts' '
+	git checkout side1^0 &&
+	test_must_fail git merge side2 &&
+	cp .git/AUTO_MERGE EXPECT &&
+	E_TREE=$(cat EXPECT) &&
+
+	git reset --hard &&
+	test_must_fail git merge-tree --real side1 side2 >RESULT &&
+	R_TREE=$(cat RESULT) &&
+
+	# Due to differences of e.g. "HEAD" vs "side1", the results will not
+	# exactly match.  Dig into individual files.
+
+	# Numbers should have three-way merged cleanly
+	test_write_lines 0 1 2 3 4 5 6 >expect &&
+	git show ${R_TREE}:numbers >actual &&
+	test_cmp expect actual &&
+
+	# whatever and whatever~<branch> should have same HASHES
+	git rev-parse ${E_TREE}:whatever ${E_TREE}:whatever~HEAD >expect &&
+	git rev-parse ${R_TREE}:whatever ${R_TREE}:whatever~side1 >actual &&
+	test_cmp expect actual &&
+
+	# greeting should have a merge conflict
+	git show ${E_TREE}:greeting >tmp &&
+	cat tmp | sed -e s/HEAD/side1/ >expect &&
+	git show ${R_TREE}:greeting >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'Barf on misspelled option' '
+	# Mis-spell with single "s" instead of double "s"
+	test_expect_code 129 git merge-tree --real --mesages FOOBAR side1 side2 2>expect &&
+
+	grep "error: unknown option.*mesages" expect
+'
+
+test_expect_success 'Barf on too many arguments' '
+	test_expect_code 129 git merge-tree --real side1 side2 side3 2>expect &&
+
+	grep "^usage: git merge-tree" expect
+'
+
+test_done
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 5/8] merge-ort: split out a separate display_update_messages() function
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                     ` (3 preceding siblings ...)
  2022-01-05 17:27   ` [PATCH v2 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
@ 2022-01-05 17:27   ` Elijah Newren via GitGitGadget
  2022-01-05 17:27   ` [PATCH v2 6/8] merge-ort: allow update messages to be written to different file stream Elijah Newren via GitGitGadget
                     ` (4 subsequent siblings)
  9 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-01-05 17:27 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 71 ++++++++++++++++++++++++++++-------------------------
 merge-ort.h |  8 ++++++
 2 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 0342f104836..3cdef173cd7 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4197,6 +4197,42 @@ static int record_conflicted_index_entries(struct merge_options *opt)
 	return errs;
 }
 
+void merge_display_update_messages(struct merge_options *opt,
+				   struct merge_result *result)
+{
+	struct merge_options_internal *opti = result->priv;
+	struct hashmap_iter iter;
+	struct strmap_entry *e;
+	struct string_list olist = STRING_LIST_INIT_NODUP;
+	int i;
+
+	trace2_region_enter("merge", "display messages", opt->repo);
+
+	/* Hack to pre-allocate olist to the desired size */
+	ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
+		   olist.alloc);
+
+	/* Put every entry from output into olist, then sort */
+	strmap_for_each_entry(&opti->output, &iter, e) {
+		string_list_append(&olist, e->key)->util = e->value;
+	}
+	string_list_sort(&olist);
+
+	/* Iterate over the items, printing them */
+	for (i = 0; i < olist.nr; ++i) {
+		struct strbuf *sb = olist.items[i].util;
+
+		printf("%s", sb->buf);
+	}
+	string_list_clear(&olist, 0);
+
+	/* Also include needed rename limit adjustment now */
+	diff_warn_rename_limit("merge.renamelimit",
+			       opti->renames.needed_limit, 0);
+
+	trace2_region_leave("merge", "display messages", opt->repo);
+}
+
 void merge_switch_to_result(struct merge_options *opt,
 			    struct tree *head,
 			    struct merge_result *result,
@@ -4235,39 +4271,8 @@ void merge_switch_to_result(struct merge_options *opt,
 		trace2_region_leave("merge", "write_auto_merge", opt->repo);
 	}
 
-	if (display_update_msgs) {
-		struct merge_options_internal *opti = result->priv;
-		struct hashmap_iter iter;
-		struct strmap_entry *e;
-		struct string_list olist = STRING_LIST_INIT_NODUP;
-		int i;
-
-		trace2_region_enter("merge", "display messages", opt->repo);
-
-		/* Hack to pre-allocate olist to the desired size */
-		ALLOC_GROW(olist.items, strmap_get_size(&opti->output),
-			   olist.alloc);
-
-		/* Put every entry from output into olist, then sort */
-		strmap_for_each_entry(&opti->output, &iter, e) {
-			string_list_append(&olist, e->key)->util = e->value;
-		}
-		string_list_sort(&olist);
-
-		/* Iterate over the items, printing them */
-		for (i = 0; i < olist.nr; ++i) {
-			struct strbuf *sb = olist.items[i].util;
-
-			printf("%s", sb->buf);
-		}
-		string_list_clear(&olist, 0);
-
-		/* Also include needed rename limit adjustment now */
-		diff_warn_rename_limit("merge.renamelimit",
-				       opti->renames.needed_limit, 0);
-
-		trace2_region_leave("merge", "display messages", opt->repo);
-	}
+	if (display_update_msgs)
+		merge_display_update_messages(opt, result);
 
 	merge_finalize(opt, result);
 }
diff --git a/merge-ort.h b/merge-ort.h
index c011864ffeb..1b93555a60b 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -70,6 +70,14 @@ void merge_switch_to_result(struct merge_options *opt,
 			    int update_worktree_and_index,
 			    int display_update_msgs);
 
+/*
+ * Display messages about conflicts and which files were 3-way merged.
+ * Automatically called by merge_switch_to_result() with stream == stdout,
+ * so only call this when bypassing merge_switch_to_result().
+ */
+void merge_display_update_messages(struct merge_options *opt,
+				   struct merge_result *result);
+
 /* Do needed cleanup when not calling merge_switch_to_result() */
 void merge_finalize(struct merge_options *opt,
 		    struct merge_result *result);
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 6/8] merge-ort: allow update messages to be written to different file stream
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                     ` (4 preceding siblings ...)
  2022-01-05 17:27   ` [PATCH v2 5/8] merge-ort: split out a separate display_update_messages() function Elijah Newren via GitGitGadget
@ 2022-01-05 17:27   ` Elijah Newren via GitGitGadget
  2022-01-05 17:27   ` [PATCH v2 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
                     ` (3 subsequent siblings)
  9 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-01-05 17:27 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 7 ++++---
 merge-ort.h | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 3cdef173cd7..86eebf39166 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4198,7 +4198,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;
@@ -4222,7 +4223,7 @@ void merge_display_update_messages(struct merge_options *opt,
 	for (i = 0; i < olist.nr; ++i) {
 		struct strbuf *sb = olist.items[i].util;
 
-		printf("%s", sb->buf);
+		fprintf(stream, "%s", sb->buf);
 	}
 	string_list_clear(&olist, 0);
 
@@ -4272,7 +4273,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);
 }
diff --git a/merge-ort.h b/merge-ort.h
index 1b93555a60b..55819a57da8 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -76,7 +76,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


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 7/8] merge-tree: support saving merge messages to a separate file
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                     ` (5 preceding siblings ...)
  2022-01-05 17:27   ` [PATCH v2 6/8] merge-ort: allow update messages to be written to different file stream Elijah Newren via GitGitGadget
@ 2022-01-05 17:27   ` Elijah Newren via GitGitGadget
  2022-01-07 18:07     ` Johannes Schindelin
  2022-01-05 17:27   ` [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
                     ` (2 subsequent siblings)
  9 siblings, 1 reply; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-01-05 17:27 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

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

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-merge-tree.txt |  6 ++++--
 builtin/merge-tree.c             | 18 ++++++++++++++++--
 t/t4301-merge-tree-real.sh       | 18 ++++++++++++++++++
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index 5823938937f..4d5857b390b 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree
 SYNOPSIS
 --------
 [verse]
-'git merge-tree' --real <branch1> <branch2>
+'git merge-tree' --real [--messages=<file>] <branch1> <branch2>
 'git merge-tree' <base-tree> <branch1> <branch2>
 
 DESCRIPTION
@@ -21,7 +21,9 @@ The first form will merge the two branches, doing a full recursive
 merge with rename detection.  If the merge is clean, the exit status
 will be `0`, and if the merge has conflicts, the exit status will be
 `1`.  The output will consist solely of the resulting toplevel tree
-(which may have files including conflict markers).
+(which may have files including conflict markers).  With `--messages`,
+it will write any informational messages (such as "Auto-merging
+<path>" and conflict notices) to the given file.
 
 The second form is meant for backward compatibility and will only do a
 trival merge.  It reads three tree-ish, and outputs trivial merge
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index ac50f3d108b..46b746b6b7c 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -390,6 +390,7 @@ static int trivial_merge(const char *base,
 
 struct merge_tree_options {
 	int real;
+	char *messages_file;
 };
 
 static int real_merge(struct merge_tree_options *o,
@@ -443,8 +444,15 @@ static int real_merge(struct merge_tree_options *o,
 	 */
 
 	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+
+	if (o->messages_file) {
+		FILE *fp = xfopen(o->messages_file, "w");
+		merge_display_update_messages(&opt, &result, fp);
+		fclose(fp);
+	}
 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
-	merge_switch_to_result(&opt, NULL, &result, 0, 0);
+
+	merge_finalize(&opt, &result);
 	return result.clean ? 0 : 1;
 }
 
@@ -452,15 +460,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
 	struct merge_tree_options o = { 0 };
 	int expected_remaining_argc;
+	int original_argc;
 
 	const char * const merge_tree_usage[] = {
-		N_("git merge-tree --real <branch1> <branch2>"),
+		N_("git merge-tree --real [<options>] <branch1> <branch2>"),
 		N_("git merge-tree <base-tree> <branch1> <branch2>"),
 		NULL
 	};
 	struct option mt_options[] = {
 		OPT_BOOL(0, "real", &o.real,
 			 N_("do a real merge instead of a trivial merge")),
+		OPT_STRING(0, "messages", &o.messages_file, N_("file"),
+			   N_("filename to write informational/conflict messages to")),
 		OPT_END()
 	};
 
@@ -469,8 +480,11 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 		usage_with_options(merge_tree_usage, mt_options);
 
 	/* Parse arguments */
+	original_argc = argc;
 	argc = parse_options(argc, argv, prefix, mt_options,
 			     merge_tree_usage, 0);
+	if (!o.real && original_argc < argc)
+		die(_("--real must be specified if any other options are"));
 	expected_remaining_argc = (o.real ? 2 : 3);
 	if (argc != expected_remaining_argc)
 		usage_with_options(merge_tree_usage, mt_options);
diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
index f7aa310f8c1..5f3f27f504d 100755
--- a/t/t4301-merge-tree-real.sh
+++ b/t/t4301-merge-tree-real.sh
@@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
 	grep "^usage: git merge-tree" expect
 '
 
+test_expect_success '--messages gives us the conflict notices and such' '
+	test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&
+
+	# Expected results:
+	#   "greeting" should merge with conflicts
+	#   "numbers" should merge cleanly
+	#   "whatever" has *both* a modify/delete and a file/directory conflict
+	cat <<-EOF >expect &&
+	Auto-merging greeting
+	CONFLICT (content): Merge conflict in greeting
+	Auto-merging numbers
+	CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead.
+	CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1.  Version side1 of whatever~side1 left in tree.
+	EOF
+
+	test_cmp expect MSG_FILE
+'
+
 test_done
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                     ` (6 preceding siblings ...)
  2022-01-05 17:27   ` [PATCH v2 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
@ 2022-01-05 17:27   ` Elijah Newren via GitGitGadget
  2022-01-05 19:09     ` Ramsay Jones
  2022-01-07 19:36     ` Johannes Schindelin
  2022-01-05 20:18   ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Junio C Hamano
  2022-01-07 18:46   ` Christian Couder
  9 siblings, 2 replies; 57+ messages in thread
From: Elijah Newren via GitGitGadget @ 2022-01-05 17:27 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

Callers of `git merge-tree --real` might want an easy way to determine
which files conflicted.  While they could potentially use the --messages
option and parse the resulting messages written to that file, those
messages are not meant to be machine readable.  Provide a simpler
mechanism of having the user specify --unmerged-list=$FILENAME, and then
write a NUL-separated list of unmerged filenames to the specified file.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-merge-tree.txt |  6 ++++--
 builtin/merge-tree.c             | 16 ++++++++++++++++
 merge-ort.c                      | 13 +++++++++++++
 merge-ort.h                      |  3 +++
 t/t4301-merge-tree-real.sh       |  9 +++++++++
 5 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index 4d5857b390b..542cea1a1a8 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -9,7 +9,7 @@ git-merge-tree - Perform merge without touching index or working tree
 SYNOPSIS
 --------
 [verse]
-'git merge-tree' --real [--messages=<file>] <branch1> <branch2>
+'git merge-tree' --real [--messages=<file>] [--conflicted-list=<file>] <branch1> <branch2>
 'git merge-tree' <base-tree> <branch1> <branch2>
 
 DESCRIPTION
@@ -23,7 +23,9 @@ will be `0`, and if the merge has conflicts, the exit status will be
 `1`.  The output will consist solely of the resulting toplevel tree
 (which may have files including conflict markers).  With `--messages`,
 it will write any informational messages (such as "Auto-merging
-<path>" and conflict notices) to the given file.
+<path>" and conflict notices) to the given file.  With
+`--conflicted-list`, it will write a list of unmerged files, one per
+line, to the given file.
 
 The second form is meant for backward compatibility and will only do a
 trival merge.  It reads three tree-ish, and outputs trivial merge
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 46b746b6b7c..4ae34da98b1 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -391,6 +391,7 @@ static int trivial_merge(const char *base,
 struct merge_tree_options {
 	int real;
 	char *messages_file;
+	char *conflicted_file;
 };
 
 static int real_merge(struct merge_tree_options *o,
@@ -450,6 +451,19 @@ static int real_merge(struct merge_tree_options *o,
 		merge_display_update_messages(&opt, &result, fp);
 		fclose(fp);
 	}
+	if (o->conflicted_file) {
+		struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
+		FILE *fp = xfopen(o->conflicted_file, "w");
+		int i;
+
+		merge_get_conflicted_files(&result, &conflicted_files);
+		for (i = 0; i < conflicted_files.nr; i++) {
+			fprintf(fp, "%s", conflicted_files.items[i].string);
+			fputc('\0', fp);
+		}
+		fclose(fp);
+		string_list_clear(&conflicted_files, 0);
+	}
 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
 
 	merge_finalize(&opt, &result);
@@ -472,6 +486,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 			 N_("do a real merge instead of a trivial merge")),
 		OPT_STRING(0, "messages", &o.messages_file, N_("file"),
 			   N_("filename to write informational/conflict messages to")),
+		OPT_STRING(0, "conflicted-list", &o.conflicted_file, N_("file"),
+			   N_("filename to write list of unmerged files")),
 		OPT_END()
 	};
 
diff --git a/merge-ort.c b/merge-ort.c
index 86eebf39166..3d6dd1b234c 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4234,6 +4234,19 @@ 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) {
+		string_list_append(conflicted_files, e->key);
+	}
+	string_list_sort(conflicted_files);
+}
+
 void merge_switch_to_result(struct merge_options *opt,
 			    struct tree *head,
 			    struct merge_result *result,
diff --git a/merge-ort.h b/merge-ort.h
index 55819a57da8..165cef6616f 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -79,6 +79,9 @@ void merge_display_update_messages(struct merge_options *opt,
 				   struct merge_result *result,
 				   FILE *stream);
 
+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);
diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
index 5f3f27f504d..ec7bd8efd06 100755
--- a/t/t4301-merge-tree-real.sh
+++ b/t/t4301-merge-tree-real.sh
@@ -96,4 +96,13 @@ test_expect_success '--messages gives us the conflict notices and such' '
 	test_cmp expect MSG_FILE
 '
 
+test_expect_success '--messages gives us the conflict notices and such' '
+	test_must_fail git merge-tree --real --conflicted-list=UNMERGED side1 side2 &&
+
+	cat UNMERGED | tr "\0" "\n" >actual &&
+	test_write_lines greeting whatever~side1 >expect &&
+
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts
  2022-01-05 17:27   ` [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
@ 2022-01-05 19:09     ` Ramsay Jones
  2022-01-05 19:17       ` Elijah Newren
  2022-01-07 19:36     ` Johannes Schindelin
  1 sibling, 1 reply; 57+ messages in thread
From: Ramsay Jones @ 2022-01-05 19:09 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget, git
  Cc: Christian Couder, Taylor Blau, Johannes Altmanninger, Elijah Newren



On 05/01/2022 17:27, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Callers of `git merge-tree --real` might want an easy way to determine
> which files conflicted.  While they could potentially use the --messages
> option and parse the resulting messages written to that file, those
> messages are not meant to be machine readable.  Provide a simpler
> mechanism of having the user specify --unmerged-list=$FILENAME, and then

s/unmerged-list/conflicted-list/

ATB,
Ramsay Jones



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts
  2022-01-05 19:09     ` Ramsay Jones
@ 2022-01-05 19:17       ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-05 19:17 UTC (permalink / raw)
  To: Ramsay Jones
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

On Wed, Jan 5, 2022 at 11:09 AM Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> On 05/01/2022 17:27, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Callers of `git merge-tree --real` might want an easy way to determine
> > which files conflicted.  While they could potentially use the --messages
> > option and parse the resulting messages written to that file, those
> > messages are not meant to be machine readable.  Provide a simpler
> > mechanism of having the user specify --unmerged-list=$FILENAME, and then
>
> s/unmerged-list/conflicted-list/

Indeed.  I had noticed after v1 that I had a mixture of using both
"unmerged" and "conflicted" to refer to the same thing, and tried to
fix it by always using the latter term.  Unfortunately, I missed
updating this commit message.  Thanks for pointing it out; will fix.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index)
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                     ` (7 preceding siblings ...)
  2022-01-05 17:27   ` [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
@ 2022-01-05 20:18   ` Junio C Hamano
  2022-01-05 22:35     ` Elijah Newren
  2022-01-07 18:46   ` Christian Couder
  9 siblings, 1 reply; 57+ messages in thread
From: Junio C Hamano @ 2022-01-05 20:18 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Johannes Altmanninger, Elijah Newren

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This series introduces a new option to git-merge-tree: --real (best name I
> could come up with). This new option is designed to allow a server-side
> "real" merge (or allow folks client-side to do merges with branches they
> don't even have checked out).

Finally.  merge-tree was added by Linus mostly as a demonstration of
idea to trick other developers into enhancing it to implement a full
merge that does not need to touch the index or the working tree, but
everybody failed to be enticed by it so far.  It is true that it can
be used server-side, but I do not think that is what we want to sell
it as (after all, receiving a push, merging it to the history in the
central repository, and checking the result out to the working tree,
would be a good "server-side" operation to have, but it can be done
today without this series).  The selling point would rather be it is
done mostly in-core, without touching working tree or the index file,
no?

Exciting ;-).




^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index)
  2022-01-05 20:18   ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Junio C Hamano
@ 2022-01-05 22:35     ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-05 22:35 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

On Wed, Jan 5, 2022 at 12:18 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > This series introduces a new option to git-merge-tree: --real (best name I
> > could come up with). This new option is designed to allow a server-side
> > "real" merge (or allow folks client-side to do merges with branches they
> > don't even have checked out).
>
> Finally.  merge-tree was added by Linus mostly as a demonstration of
> idea to trick other developers into enhancing it to implement a full
> merge that does not need to touch the index or the working tree, but
> everybody failed to be enticed by it so far. It is true that it can
> be used server-side, but I do not think that is what we want to sell
> it as (after all, receiving a push, merging it to the history in the
> central repository, and checking the result out to the working tree,
> would be a good "server-side" operation to have, but it can be done
> today without this series).  The selling point would rather be it is
> done mostly in-core, without touching working tree or the index file,
> no?

You're probably right about how we try to sell it as a project to
external folks, but I was focused instead on selling it to reviewers
within the project.

"Server side merge" was the name of the topic at the Git Summit and
lots of folks had interesting comments back then, so I was hoping to
grab people's attention with a phrase they would have seen previously
and commented on.

Further, the folks I know of who have experience trying to do an
in-core merge are folks who operate on the server side (using libgit2
instead of git, which they have some gripes with).  I wanted their
experience and views in the review and wanted to make sure it met
their needs, and tried to highlight that to lure them into responding
and reviewing.

> Exciting ;-).

Thanks.  :-)

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 4/8] merge-tree: implement real merges
  2022-01-05 17:27   ` [PATCH v2 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
@ 2022-01-07 15:30     ` Johannes Schindelin
  2022-01-07 17:26       ` Elijah Newren
  2022-01-07 18:12     ` Christian Couder
  1 sibling, 1 reply; 57+ messages in thread
From: Johannes Schindelin @ 2022-01-07 15:30 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

Hi Elijah,

On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> This adds the ability to perform real merges rather than just trivial
> merges (meaning handling three way content merges, recursive ancestor
> consolidation, renames, proper directory/file conflict handling, and so
> forth).  However, unlike `git merge`, the working tree and index are
> left alone and no branch is updated.
>
> The only output is:
>   - the toplevel resulting tree printed on stdout
>   - exit status of 0 (clean) or 1 (conflicts present)
>
> This output is mean to be used by some higher level script, perhaps in a
                 ^^^^

My apologies for pointing out a grammar issue: This probably intended to
say "meant", as the word "mean" changes the sense of the sentence.

In my defense, I have more substantial suggestions below.

> sequence of steps like this:
>
>    NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
>    test $? -eq 0 || die "There were conflicts..."
>    NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 -p $BRANCH2)
>    git update-ref $BRANCH1 $NEWCOMMIT
>
> Note that higher level scripts may also want to access the
> conflict/warning messages normally output during a merge, or have quick
> access to a list of files with conflicts.  That is not available in this
> preliminary implementation, but subsequent commits will add that
> ability.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  Documentation/git-merge-tree.txt | 28 +++++++----
>  builtin/merge-tree.c             | 55 +++++++++++++++++++++-
>  t/t4301-merge-tree-real.sh       | 81 ++++++++++++++++++++++++++++++++
>  3 files changed, 153 insertions(+), 11 deletions(-)
>  create mode 100755 t/t4301-merge-tree-real.sh
>
> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> index 58731c19422..5823938937f 100644
> --- a/Documentation/git-merge-tree.txt
> +++ b/Documentation/git-merge-tree.txt
> @@ -3,26 +3,34 @@ git-merge-tree(1)
>
>  NAME
>  ----
> -git-merge-tree - Show three-way merge without touching index
> +git-merge-tree - Perform merge without touching index or working tree
>
>
>  SYNOPSIS
>  --------
>  [verse]
> +'git merge-tree' --real <branch1> <branch2>
>  'git merge-tree' <base-tree> <branch1> <branch2>

Here is an idea: How about aiming for this synopsis instead, exploiting
the fact that the "real" mode takes a different amount of arguments?

   'git merge-tree' [--write-tree] <branch1> <branch2>
   'git merge-tree' [--demo-trivial-merge] <base-tree> <branch1> <branch2>

That way, the old mode can still function, and can even at some stage be
deprecated and eventually removed.

>
>  DESCRIPTION
>  -----------
> -Reads three tree-ish, and output trivial merge results and
> -conflicting stages to the standard output.  This is similar to
> -what three-way 'git read-tree -m' does, but instead of storing the
> -results in the index, the command outputs the entries to the
> -standard output.
> +Performs a merge, but does not make any new commits and does not read
> +from or write to either the working tree or index.
>
> -This is meant to be used by higher level scripts to compute
> -merge results outside of the index, and stuff the results back into the
> -index.  For this reason, the output from the command omits
> -entries that match the <branch1> tree.
> +The first form will merge the two branches, doing a full recursive
> +merge with rename detection.  If the merge is clean, the exit status
> +will be `0`, and if the merge has conflicts, the exit status will be
> +`1`.  The output will consist solely of the resulting toplevel tree
> +(which may have files including conflict markers).
> +
> +The second form is meant for backward compatibility and will only do a
> +trival merge.  It reads three tree-ish, and outputs trivial merge
> +results and conflicting stages to the standard output in a semi-diff
> +format.  Since this was designed for higher level scripts to consume
> +and merge the results back into the index, it omits entries that match
> +<branch1>.  The result of this second form is is similar to what
> +three-way 'git read-tree -m' does, but instead of storing the results
> +in the index, the command outputs the entries to the standard output.
>
>  GIT
>  ---
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index e1d2832c809..ac50f3d108b 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -2,6 +2,9 @@
>  #include "builtin.h"
>  #include "tree-walk.h"
>  #include "xdiff-interface.h"
> +#include "help.h"
> +#include "commit-reach.h"
> +#include "merge-ort.h"
>  #include "object-store.h"
>  #include "parse-options.h"
>  #include "repository.h"
> @@ -392,7 +395,57 @@ struct merge_tree_options {
>  static int real_merge(struct merge_tree_options *o,
>  		      const char *branch1, const char *branch2)
>  {
> -	die(_("real merges are not yet implemented"));
> +	struct commit *parent1, *parent2;
> +	struct commit_list *common;
> +	struct commit_list *merge_bases = NULL;
> +	struct commit_list *j;
> +	struct merge_options opt;
> +	struct merge_result result = { 0 };
> +
> +	parent1 = get_merge_parent(branch1);
> +	if (!parent1)
> +		help_unknown_ref(branch1, "merge",
> +				 _("not something we can merge"));
> +
> +	parent2 = get_merge_parent(branch2);
> +	if (!parent2)
> +		help_unknown_ref(branch2, "merge",
> +				 _("not something we can merge"));
> +
> +	init_merge_options(&opt, the_repository);
> +	/*
> +	 * TODO: Support subtree and other -X options?
> +	if (use_strategies_nr == 1 &&
> +	    !strcmp(use_strategies[0]->name, "subtree"))
> +		opt.subtree_shift = "";
> +	for (x = 0; x < xopts_nr; x++)
> +		if (parse_merge_opt(&opt, xopts[x]))
> +			die(_("Unknown strategy option: -X%s"), xopts[x]);
> +	*/
> +
> +	opt.show_rename_progress = 0;
> +
> +	opt.branch1 = merge_remote_util(parent1)->name; /* or just branch1? */
> +	opt.branch2 = merge_remote_util(parent2)->name; /* or just branch2? */
> +
> +	/*
> +	 * Get the merge bases, in reverse order; see comment above
> +	 * merge_incore_recursive in merge-ort.h
> +	 */
> +	common = get_merge_bases(parent1, parent2);
> +	for (j = common; j; j = j->next)
> +		commit_list_insert(j->item, &merge_bases);
> +
> +	/*
> +	 * TODO: notify if merging unrelated histories?

I guess that it would make most sense to add a flag whether this is
allowed or not, and I would suggest the default to be `off`.

> +	if (!common)
> +		fprintf(stderr, _("merging unrelated histories"));
> +	 */
> +
> +	merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> +	printf("%s\n", oid_to_hex(&result.tree->object.oid));
> +	merge_switch_to_result(&opt, NULL, &result, 0, 0);

This looks to be idempotent to `merge_finalize(&opt, &result)`, so maybe
use that instead?

> +	return result.clean ? 0 : 1;
>  }
>
>  int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> new file mode 100755
> index 00000000000..f7aa310f8c1
> --- /dev/null
> +++ b/t/t4301-merge-tree-real.sh
> @@ -0,0 +1,81 @@
> +#!/bin/sh
> +
> +test_description='git merge-tree --real'
> +
> +. ./test-lib.sh
> +
> +# This test is ort-specific
> +GIT_TEST_MERGE_ALGORITHM=ort
> +export GIT_TEST_MERGE_ALGORITHM

It might make sense to skip the entire test if the user asked for
`recursive` to be tested:

	test "${GIT_TEST_MERGE_ALGORITHM:-ort}" = ort ||
		skip_all="GIT_TEST_MERGE_ALGORITHM != ort"
		test_done
	}

> +
> +test_expect_success setup '
> +	test_write_lines 1 2 3 4 5 >numbers &&
> +	echo hello >greeting &&
> +	echo foo >whatever &&
> +	git add numbers greeting whatever &&
> +	git commit -m initial &&

I would really like to encourage the use of `test_tick`. It makes the
commit consistent, just in case you run into an issue that depends on some
hash order.

> +
> +	git branch side1 &&
> +	git branch side2 &&
> +
> +	git checkout side1 &&

Please use `git switch -c side1` or `git checkout -b side1`: it is more
compact than `git branch ... && git checkout ...`.

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

This could be written as `git checkout -b side2 HEAD^`, to make the setup
more succinct.

> +	test_write_lines 0 1 2 3 4 5 >numbers &&
> +	echo yo >greeting &&
> +	git rm whatever &&
> +	mkdir whatever &&
> +	>whatever/empty &&
> +	git add numbers greeting whatever/empty &&
> +	git commit -m other-modifications
> +'
> +
> +test_expect_success 'Content merge and a few conflicts' '
> +	git checkout side1^0 &&
> +	test_must_fail git merge side2 &&
> +	cp .git/AUTO_MERGE EXPECT &&
> +	E_TREE=$(cat EXPECT) &&

The file `EXPECT` is not used below. And can we use a more obvious name?
SOmething like:

	expected_tree=$(cat .git/AUTO_MERGE)

> +	git reset --hard &&

For an extra bonus, we could delay this via `test_when_finished`, to prove
that `git merge-tree --real` works even in a dirty worktree _with
conflicts_.

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

How about `actual_tree` instead?

> +
> +	# Due to differences of e.g. "HEAD" vs "side1", the results will not
> +	# exactly match.  Dig into individual files.
> +
> +	# Numbers should have three-way merged cleanly
> +	test_write_lines 0 1 2 3 4 5 6 >expect &&
> +	git show ${R_TREE}:numbers >actual &&
> +	test_cmp expect actual &&
> +
> +	# whatever and whatever~<branch> should have same HASHES
> +	git rev-parse ${E_TREE}:whatever ${E_TREE}:whatever~HEAD >expect &&
> +	git rev-parse ${R_TREE}:whatever ${R_TREE}:whatever~side1 >actual &&
> +	test_cmp expect actual &&
> +
> +	# greeting should have a merge conflict
> +	git show ${E_TREE}:greeting >tmp &&
> +	cat tmp | sed -e s/HEAD/side1/ >expect &&
> +	git show ${R_TREE}:greeting >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'Barf on misspelled option' '
> +	# Mis-spell with single "s" instead of double "s"
> +	test_expect_code 129 git merge-tree --real --mesages FOOBAR side1 side2 2>expect &&
> +
> +	grep "error: unknown option.*mesages" expect
> +'

I do not think that this test case adds much, and we already test the
`parse_options()` machinery elsewhere.

> +
> +test_expect_success 'Barf on too many arguments' '
> +	test_expect_code 129 git merge-tree --real side1 side2 side3 2>expect &&
> +
> +	grep "^usage: git merge-tree" expect
> +'
> +
> +test_done

The rest looks awesome. Thank you for working on it! I will definitely
come back to review the rest (have to take a break now), and then probably
add quite a bit of food for thought based on my experience _actually_
using `merge-ort` on the server-side. Stay tuned.

Thank you,
Dscho

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 4/8] merge-tree: implement real merges
  2022-01-07 15:30     ` Johannes Schindelin
@ 2022-01-07 17:26       ` Elijah Newren
  2022-01-07 18:22         ` Johannes Schindelin
  2022-01-07 20:56         ` Junio C Hamano
  0 siblings, 2 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-07 17:26 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

On Fri, Jan 7, 2022 at 7:30 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > This adds the ability to perform real merges rather than just trivial
> > merges (meaning handling three way content merges, recursive ancestor
> > consolidation, renames, proper directory/file conflict handling, and so
> > forth).  However, unlike `git merge`, the working tree and index are
> > left alone and no branch is updated.
> >
> > The only output is:
> >   - the toplevel resulting tree printed on stdout
> >   - exit status of 0 (clean) or 1 (conflicts present)
> >
> > This output is mean to be used by some higher level script, perhaps in a
>                  ^^^^
>
> My apologies for pointing out a grammar issue: This probably intended to
> say "meant", as the word "mean" changes the sense of the sentence.

Oops.  Yeah, I'll correct that; thanks for pointing it out.

> In my defense, I have more substantial suggestions below.
>
> > sequence of steps like this:
> >
> >    NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
> >    test $? -eq 0 || die "There were conflicts..."
> >    NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 -p $BRANCH2)
> >    git update-ref $BRANCH1 $NEWCOMMIT
> >
> > Note that higher level scripts may also want to access the
> > conflict/warning messages normally output during a merge, or have quick
> > access to a list of files with conflicts.  That is not available in this
> > preliminary implementation, but subsequent commits will add that
> > ability.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  Documentation/git-merge-tree.txt | 28 +++++++----
> >  builtin/merge-tree.c             | 55 +++++++++++++++++++++-
> >  t/t4301-merge-tree-real.sh       | 81 ++++++++++++++++++++++++++++++++
> >  3 files changed, 153 insertions(+), 11 deletions(-)
> >  create mode 100755 t/t4301-merge-tree-real.sh
> >
> > diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> > index 58731c19422..5823938937f 100644
> > --- a/Documentation/git-merge-tree.txt
> > +++ b/Documentation/git-merge-tree.txt
> > @@ -3,26 +3,34 @@ git-merge-tree(1)
> >
> >  NAME
> >  ----
> > -git-merge-tree - Show three-way merge without touching index
> > +git-merge-tree - Perform merge without touching index or working tree
> >
> >
> >  SYNOPSIS
> >  --------
> >  [verse]
> > +'git merge-tree' --real <branch1> <branch2>
> >  'git merge-tree' <base-tree> <branch1> <branch2>
>
> Here is an idea: How about aiming for this synopsis instead, exploiting
> the fact that the "real" mode takes a different amount of arguments?

My turn on the grammar thing: s/amount/number/.   :-)

>
>    'git merge-tree' [--write-tree] <branch1> <branch2>
>    'git merge-tree' [--demo-trivial-merge] <base-tree> <branch1> <branch2>
>
> That way, the old mode can still function, and can even at some stage be
> deprecated and eventually removed.

Ooh, interesting.

> >
> >  DESCRIPTION
> >  -----------
> > -Reads three tree-ish, and output trivial merge results and
> > -conflicting stages to the standard output.  This is similar to
> > -what three-way 'git read-tree -m' does, but instead of storing the
> > -results in the index, the command outputs the entries to the
> > -standard output.
> > +Performs a merge, but does not make any new commits and does not read
> > +from or write to either the working tree or index.
> >
> > -This is meant to be used by higher level scripts to compute
> > -merge results outside of the index, and stuff the results back into the
> > -index.  For this reason, the output from the command omits
> > -entries that match the <branch1> tree.
> > +The first form will merge the two branches, doing a full recursive
> > +merge with rename detection.  If the merge is clean, the exit status
> > +will be `0`, and if the merge has conflicts, the exit status will be
> > +`1`.  The output will consist solely of the resulting toplevel tree
> > +(which may have files including conflict markers).
> > +
> > +The second form is meant for backward compatibility and will only do a
> > +trival merge.  It reads three tree-ish, and outputs trivial merge
> > +results and conflicting stages to the standard output in a semi-diff
> > +format.  Since this was designed for higher level scripts to consume
> > +and merge the results back into the index, it omits entries that match
> > +<branch1>.  The result of this second form is is similar to what
> > +three-way 'git read-tree -m' does, but instead of storing the results
> > +in the index, the command outputs the entries to the standard output.
> >
> >  GIT
> >  ---
> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > index e1d2832c809..ac50f3d108b 100644
> > --- a/builtin/merge-tree.c
> > +++ b/builtin/merge-tree.c
> > @@ -2,6 +2,9 @@
> >  #include "builtin.h"
> >  #include "tree-walk.h"
> >  #include "xdiff-interface.h"
> > +#include "help.h"
> > +#include "commit-reach.h"
> > +#include "merge-ort.h"
> >  #include "object-store.h"
> >  #include "parse-options.h"
> >  #include "repository.h"
> > @@ -392,7 +395,57 @@ struct merge_tree_options {
> >  static int real_merge(struct merge_tree_options *o,
> >                     const char *branch1, const char *branch2)
> >  {
> > -     die(_("real merges are not yet implemented"));
> > +     struct commit *parent1, *parent2;
> > +     struct commit_list *common;
> > +     struct commit_list *merge_bases = NULL;
> > +     struct commit_list *j;
> > +     struct merge_options opt;
> > +     struct merge_result result = { 0 };
> > +
> > +     parent1 = get_merge_parent(branch1);
> > +     if (!parent1)
> > +             help_unknown_ref(branch1, "merge",
> > +                              _("not something we can merge"));
> > +
> > +     parent2 = get_merge_parent(branch2);
> > +     if (!parent2)
> > +             help_unknown_ref(branch2, "merge",
> > +                              _("not something we can merge"));
> > +
> > +     init_merge_options(&opt, the_repository);
> > +     /*
> > +      * TODO: Support subtree and other -X options?
> > +     if (use_strategies_nr == 1 &&
> > +         !strcmp(use_strategies[0]->name, "subtree"))
> > +             opt.subtree_shift = "";
> > +     for (x = 0; x < xopts_nr; x++)
> > +             if (parse_merge_opt(&opt, xopts[x]))
> > +                     die(_("Unknown strategy option: -X%s"), xopts[x]);
> > +     */
> > +
> > +     opt.show_rename_progress = 0;
> > +
> > +     opt.branch1 = merge_remote_util(parent1)->name; /* or just branch1? */
> > +     opt.branch2 = merge_remote_util(parent2)->name; /* or just branch2? */
> > +
> > +     /*
> > +      * Get the merge bases, in reverse order; see comment above
> > +      * merge_incore_recursive in merge-ort.h
> > +      */
> > +     common = get_merge_bases(parent1, parent2);
> > +     for (j = common; j; j = j->next)
> > +             commit_list_insert(j->item, &merge_bases);
> > +
> > +     /*
> > +      * TODO: notify if merging unrelated histories?
>
> I guess that it would make most sense to add a flag whether this is
> allowed or not, and I would suggest the default to be `off`.

Sounds fair.  Thanks for commenting on one of the TODOs that I was unsure about.

> > +     if (!common)
> > +             fprintf(stderr, _("merging unrelated histories"));
> > +      */
> > +
> > +     merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> > +     printf("%s\n", oid_to_hex(&result.tree->object.oid));
> > +     merge_switch_to_result(&opt, NULL, &result, 0, 0);
>
> This looks to be idempotent to `merge_finalize(&opt, &result)`, so maybe
> use that instead?

Yeah, and add a TODO about the display messages (that'll be addressed
in a later patch, unlike the above TODOs).

>
> > +     return result.clean ? 0 : 1;
> >  }
> >
> >  int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> > diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> > new file mode 100755
> > index 00000000000..f7aa310f8c1
> > --- /dev/null
> > +++ b/t/t4301-merge-tree-real.sh
> > @@ -0,0 +1,81 @@
> > +#!/bin/sh
> > +
> > +test_description='git merge-tree --real'
> > +
> > +. ./test-lib.sh
> > +
> > +# This test is ort-specific
> > +GIT_TEST_MERGE_ALGORITHM=ort
> > +export GIT_TEST_MERGE_ALGORITHM
>
> It might make sense to skip the entire test if the user asked for
> `recursive` to be tested:
>
>         test "${GIT_TEST_MERGE_ALGORITHM:-ort}" = ort ||
>                 skip_all="GIT_TEST_MERGE_ALGORITHM != ort"
>                 test_done
>         }

The idea makes sense, but it took me a bit to understand this code
block.  I think you're just missing an opening left curly brace right
after the '||'?

> > +
> > +test_expect_success setup '
> > +     test_write_lines 1 2 3 4 5 >numbers &&
> > +     echo hello >greeting &&
> > +     echo foo >whatever &&
> > +     git add numbers greeting whatever &&
> > +     git commit -m initial &&
>
> I would really like to encourage the use of `test_tick`. It makes the
> commit consistent, just in case you run into an issue that depends on some
> hash order.

I've used test_tick before, but I already know this test can't depend
on hash order.  Further, the hashes in the output are also replaced
before comparing in order to make the tests also work as-is under
sha256.  So the tests are explicitly ignoring precise hashes.  As
such, I'm not sure I see the value of test_tick here.

> > +
> > +     git branch side1 &&
> > +     git branch side2 &&
> > +
> > +     git checkout side1 &&
>
> Please use `git switch -c side1` or `git checkout -b side1`: it is more
> compact than `git branch ... && git checkout ...`.

Yes, but less forgiving to later modification where I go and add
additional commits on one of the sides, because...

>
> > +     test_write_lines 1 2 3 4 5 6 >numbers &&
> > +     echo hi >greeting &&
> > +     echo bar >whatever &&
> > +     git add numbers greeting whatever &&
> > +     git commit -m modify-stuff &&
> > +
> > +     git checkout side2 &&
>
> This could be written as `git checkout -b side2 HEAD^`, to make the setup
> more succinct.

...the presumption of HEAD^ is hardcoded and has to be parsed by
readers to understand the test.  It felt like more cognitive overhead
to me, in addition to being less malleable.

> > +     test_write_lines 0 1 2 3 4 5 >numbers &&
> > +     echo yo >greeting &&
> > +     git rm whatever &&
> > +     mkdir whatever &&
> > +     >whatever/empty &&
> > +     git add numbers greeting whatever/empty &&
> > +     git commit -m other-modifications
> > +'
> > +
> > +test_expect_success 'Content merge and a few conflicts' '
> > +     git checkout side1^0 &&
> > +     test_must_fail git merge side2 &&
> > +     cp .git/AUTO_MERGE EXPECT &&
> > +     E_TREE=$(cat EXPECT) &&
>
> The file `EXPECT` is not used below. And can we use a more obvious name?
> SOmething like:
>
>         expected_tree=$(cat .git/AUTO_MERGE)

There go my beautiful <80 character lines below.  :-(

But on a more serious note, yeah this is probably better.  I'll change it.  :-)

>
> > +     git reset --hard &&
>
> For an extra bonus, we could delay this via `test_when_finished`, to prove
> that `git merge-tree --real` works even in a dirty worktree _with
> conflicts_.

Ooh, good thought.  I like that.

>
> > +     test_must_fail git merge-tree --real side1 side2 >RESULT &&
> > +     R_TREE=$(cat RESULT) &&
>
> How about `actual_tree` instead?

But my 80-characters rev-parse lines....waaah.  Just kidding, yeah
this would be better.

> > +
> > +     # Due to differences of e.g. "HEAD" vs "side1", the results will not
> > +     # exactly match.  Dig into individual files.
> > +
> > +     # Numbers should have three-way merged cleanly
> > +     test_write_lines 0 1 2 3 4 5 6 >expect &&
> > +     git show ${R_TREE}:numbers >actual &&
> > +     test_cmp expect actual &&
> > +
> > +     # whatever and whatever~<branch> should have same HASHES
> > +     git rev-parse ${E_TREE}:whatever ${E_TREE}:whatever~HEAD >expect &&
> > +     git rev-parse ${R_TREE}:whatever ${R_TREE}:whatever~side1 >actual &&
> > +     test_cmp expect actual &&
> > +
> > +     # greeting should have a merge conflict
> > +     git show ${E_TREE}:greeting >tmp &&
> > +     cat tmp | sed -e s/HEAD/side1/ >expect &&
> > +     git show ${R_TREE}:greeting >actual &&
> > +     test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'Barf on misspelled option' '
> > +     # Mis-spell with single "s" instead of double "s"
> > +     test_expect_code 129 git merge-tree --real --mesages FOOBAR side1 side2 2>expect &&
> > +
> > +     grep "error: unknown option.*mesages" expect
> > +'
>
> I do not think that this test case adds much, and we already test the
> `parse_options()` machinery elsewhere.

It's more about verifying that exit codes of 0 & 1 are reserved for
"completed with no conflicts" and "completed with conflicts".  The 129
bit in this test is the important bit (and perhaps is well-known to
lots of other folks, but I thought it was worth highlighting).  That
said, I did a bad job mentioning that in the test description; I'll
fix it up.

> > +
> > +test_expect_success 'Barf on too many arguments' '
> > +     test_expect_code 129 git merge-tree --real side1 side2 side3 2>expect &&
> > +
> > +     grep "^usage: git merge-tree" expect
> > +'
> > +
> > +test_done
>
> The rest looks awesome. Thank you for working on it! I will definitely
> come back to review the rest (have to take a break now), and then probably
> add quite a bit of food for thought based on my experience _actually_
> using `merge-ort` on the server-side. Stay tuned.

Ooh, I'm intrigued.  And thanks for reviewing!

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 7/8] merge-tree: support saving merge messages to a separate file
  2022-01-05 17:27   ` [PATCH v2 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
@ 2022-01-07 18:07     ` Johannes Schindelin
  2022-01-08  1:02       ` Elijah Newren
  0 siblings, 1 reply; 57+ messages in thread
From: Johannes Schindelin @ 2022-01-07 18:07 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

Hi Elijah,

On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:

> diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> index f7aa310f8c1..5f3f27f504d 100755
> --- a/t/t4301-merge-tree-real.sh
> +++ b/t/t4301-merge-tree-real.sh
> @@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
>  	grep "^usage: git merge-tree" expect
>  '
>
> +test_expect_success '--messages gives us the conflict notices and such' '
> +	test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&

Since we discern between exit status 1 (= merge conflict) and >1 (fatal
error), we should probably use `test_expect_code` here.

Other than that, this patch looks good.

Thank you,
Dscho

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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 4/8] merge-tree: implement real merges
  2022-01-05 17:27   ` [PATCH v2 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
  2022-01-07 15:30     ` Johannes Schindelin
@ 2022-01-07 18:12     ` Christian Couder
  2022-01-07 19:09       ` Elijah Newren
  1 sibling, 1 reply; 57+ messages in thread
From: Christian Couder @ 2022-01-07 18:12 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Johannes Altmanninger, Elijah Newren

On Wed, Jan 5, 2022 at 6:27 PM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:

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

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

> +The first form will merge the two branches, doing a full recursive
> +merge with rename detection.  If the merge is clean, the exit status
> +will be `0`, and if the merge has conflicts, the exit status will be
> +`1`.

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

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

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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 4/8] merge-tree: implement real merges
  2022-01-07 17:26       ` Elijah Newren
@ 2022-01-07 18:22         ` Johannes Schindelin
  2022-01-07 19:15           ` Elijah Newren
  2022-01-07 20:56         ` Junio C Hamano
  1 sibling, 1 reply; 57+ messages in thread
From: Johannes Schindelin @ 2022-01-07 18:22 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

Hi Elijah,


On Fri, 7 Jan 2022, Elijah Newren wrote:

> On Fri, Jan 7, 2022 at 7:30 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:
> >
> > >  SYNOPSIS
> > >  --------
> > >  [verse]
> > > +'git merge-tree' --real <branch1> <branch2>
> > >  'git merge-tree' <base-tree> <branch1> <branch2>
> >
> > Here is an idea: How about aiming for this synopsis instead, exploiting
> > the fact that the "real" mode takes a different amount of arguments?
>
> My turn on the grammar thing: s/amount/number/.   :-)

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

> > > diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> > > new file mode 100755
> > > index 00000000000..f7aa310f8c1
> > > --- /dev/null
> > > +++ b/t/t4301-merge-tree-real.sh
> > > @@ -0,0 +1,81 @@
> > > +#!/bin/sh
> > > +
> > > +test_description='git merge-tree --real'
> > > +
> > > +. ./test-lib.sh
> > > +
> > > +# This test is ort-specific
> > > +GIT_TEST_MERGE_ALGORITHM=ort
> > > +export GIT_TEST_MERGE_ALGORITHM
> >
> > It might make sense to skip the entire test if the user asked for
> > `recursive` to be tested:
> >
> >         test "${GIT_TEST_MERGE_ALGORITHM:-ort}" = ort ||
> >                 skip_all="GIT_TEST_MERGE_ALGORITHM != ort"
> >                 test_done
> >         }
>
> The idea makes sense, but it took me a bit to understand this code
> block.  I think you're just missing an opening left curly brace right
> after the '||'?

Yes. Sorry.

> > > +test_expect_success setup '
> > > +     test_write_lines 1 2 3 4 5 >numbers &&
> > > +     echo hello >greeting &&
> > > +     echo foo >whatever &&
> > > +     git add numbers greeting whatever &&
> > > +     git commit -m initial &&
> >
> > I would really like to encourage the use of `test_tick`. It makes the
> > commit consistent, just in case you run into an issue that depends on some
> > hash order.
>
> I've used test_tick before, but I already know this test can't depend
> on hash order.  Further, the hashes in the output are also replaced
> before comparing in order to make the tests also work as-is under
> sha256.  So the tests are explicitly ignoring precise hashes.  As
> such, I'm not sure I see the value of test_tick here.

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

> > > +
> > > +     git branch side1 &&
> > > +     git branch side2 &&
> > > +
> > > +     git checkout side1 &&
> >
> > Please use `git switch -c side1` or `git checkout -b side1`: it is more
> > compact than `git branch ... && git checkout ...`.
>
> Yes, but less forgiving to later modification where I go and add
> additional commits on one of the sides, because...
>
> >
> > > +     test_write_lines 1 2 3 4 5 6 >numbers &&
> > > +     echo hi >greeting &&
> > > +     echo bar >whatever &&
> > > +     git add numbers greeting whatever &&
> > > +     git commit -m modify-stuff &&
> > > +
> > > +     git checkout side2 &&
> >
> > This could be written as `git checkout -b side2 HEAD^`, to make the setup
> > more succinct.
>
> ...the presumption of HEAD^ is hardcoded and has to be parsed by
> readers to understand the test.  It felt like more cognitive overhead
> to me, in addition to being less malleable.

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

> > > +test_expect_success 'Barf on misspelled option' '
> > > +     # Mis-spell with single "s" instead of double "s"
> > > +     test_expect_code 129 git merge-tree --real --mesages FOOBAR side1 side2 2>expect &&
> > > +
> > > +     grep "error: unknown option.*mesages" expect
> > > +'
> >
> > I do not think that this test case adds much, and we already test the
> > `parse_options()` machinery elsewhere.
>
> It's more about verifying that exit codes of 0 & 1 are reserved for
> "completed with no conflicts" and "completed with conflicts".  The 129
> bit in this test is the important bit (and perhaps is well-known to
> lots of other folks, but I thought it was worth highlighting).

Fair enough.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index)
  2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
                     ` (8 preceding siblings ...)
  2022-01-05 20:18   ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Junio C Hamano
@ 2022-01-07 18:46   ` Christian Couder
  2022-01-07 19:59     ` Elijah Newren
  9 siblings, 1 reply; 57+ messages in thread
From: Christian Couder @ 2022-01-07 18:46 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Johannes Altmanninger, Elijah Newren

On Wed, Jan 5, 2022 at 6:27 PM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:

> This series attempts to guess what kind of output would be wanted, basically
> choosing:
>
>  * clean merge or conflict signalled via exit status

(Maybe s/signalled/signaled/)

Not sure that's the best way by default. I think it's very likely that
many users will be interested in parsing the command ouput, and they
might prefer that merge related errors be signaled in a different way
than other errors.

>  * stdout consists solely of printing the hash of the resulting tree (though
>    that tree may include files that have conflict markers)

Maybe users will want diffs, the conflicted list and other things on
stdout, as they might want to parse it anyway, and it would be a
burden to have to perform diffs, or get other interesting info in a
different way or using a different process or call.

>  * new optional --messages flag for specifying a file where informational
>    messages (e.g. conflict notices and files involved in three-way-content
>    merges) can be written; by default, this output is simply discarded
>  * new optional --conflicted-list flag for specifying a file where the names
>    of conflicted-files can be written in a NUL-character-separated list

It would be nice if output was printed on stdout when the above flags
are used without argument.

Thanks for working on this!

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 4/8] merge-tree: implement real merges
  2022-01-07 18:12     ` Christian Couder
@ 2022-01-07 19:09       ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-07 19:09 UTC (permalink / raw)
  To: Christian Couder
  Cc: Elijah Newren via GitGitGadget, git, Christian Couder,
	Taylor Blau, Johannes Altmanninger

On Fri, Jan 7, 2022 at 10:12 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Jan 5, 2022 at 6:27 PM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>
> > The only output is:
> >   - the toplevel resulting tree printed on stdout
> >   - exit status of 0 (clean) or 1 (conflicts present)
>
> I thought that the merge-ort API could (at least theoretically
> according to merge-ort.h) return something < 0 in case of internal
> error. In this case I would be interested in knowing what's the output
> of the command.
>
> > +The first form will merge the two branches, doing a full recursive
> > +merge with rename detection.  If the merge is clean, the exit status
> > +will be `0`, and if the merge has conflicts, the exit status will be
> > +`1`.
>
> No mention of what happens in case of an internal error in the merge-ort API.
>
> > +       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> > +       printf("%s\n", oid_to_hex(&result.tree->object.oid));
> > +       merge_switch_to_result(&opt, NULL, &result, 0, 0);
> > +       return result.clean ? 0 : 1;
>
> If result.clean can be < 0, this might pretend that the merge was clean.

Ooh, these are very good points.  Thanks for bringing them up; I'll
try to address them in a re-roll.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 4/8] merge-tree: implement real merges
  2022-01-07 18:22         ` Johannes Schindelin
@ 2022-01-07 19:15           ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-07 19:15 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

Hi Dscho,

On Fri, Jan 7, 2022 at 10:23 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Fri, 7 Jan 2022, Elijah Newren wrote:
>
> > On Fri, Jan 7, 2022 at 7:30 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:
> > >
> > > >  SYNOPSIS
> > > >  --------
> > > >  [verse]
> > > > +'git merge-tree' --real <branch1> <branch2>
> > > >  'git merge-tree' <base-tree> <branch1> <branch2>
> > >
> > > Here is an idea: How about aiming for this synopsis instead, exploiting
> > > the fact that the "real" mode takes a different amount of arguments?
> >
> > My turn on the grammar thing: s/amount/number/.   :-)
>
> See? I know why I'm refraining from nitpicking. It's just not good for
> anyone involved.

Well, in your case, the point you brought up will improve the commit
message for future readers, and so it was totally justified (and I'm
glad you brought it up).  My comment is useful for nothing more than a
bit of good-natured ribbing.  But I'm not sure it was taken that way,
so I'm sorry if my comment had any effect other than making you smile.

> > > > diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> > > > new file mode 100755
> > > > index 00000000000..f7aa310f8c1
> > > > --- /dev/null
> > > > +++ b/t/t4301-merge-tree-real.sh
> > > > @@ -0,0 +1,81 @@
> > > > +#!/bin/sh
> > > > +
> > > > +test_description='git merge-tree --real'
> > > > +
> > > > +. ./test-lib.sh
> > > > +
> > > > +# This test is ort-specific
> > > > +GIT_TEST_MERGE_ALGORITHM=ort
> > > > +export GIT_TEST_MERGE_ALGORITHM
> > >
> > > It might make sense to skip the entire test if the user asked for
> > > `recursive` to be tested:
> > >
> > >         test "${GIT_TEST_MERGE_ALGORITHM:-ort}" = ort ||
> > >                 skip_all="GIT_TEST_MERGE_ALGORITHM != ort"
> > >                 test_done
> > >         }
> >
> > The idea makes sense, but it took me a bit to understand this code
> > block.  I think you're just missing an opening left curly brace right
> > after the '||'?
>
> Yes. Sorry.
>
> > > > +test_expect_success setup '
> > > > +     test_write_lines 1 2 3 4 5 >numbers &&
> > > > +     echo hello >greeting &&
> > > > +     echo foo >whatever &&
> > > > +     git add numbers greeting whatever &&
> > > > +     git commit -m initial &&
> > >
> > > I would really like to encourage the use of `test_tick`. It makes the
> > > commit consistent, just in case you run into an issue that depends on some
> > > hash order.
> >
> > I've used test_tick before, but I already know this test can't depend
> > on hash order.  Further, the hashes in the output are also replaced
> > before comparing in order to make the tests also work as-is under
> > sha256.  So the tests are explicitly ignoring precise hashes.  As
> > such, I'm not sure I see the value of test_tick here.
>
> Nevertheless. To make comparing logs of two different test runs easier, it
> makes more sense to insist on consistency.

Ah...comparing logs between two different test runs; that sounds like
a reasonable justification.  I'll add the test_tick's.

> > > > +
> > > > +     git branch side1 &&
> > > > +     git branch side2 &&
> > > > +
> > > > +     git checkout side1 &&
> > >
> > > Please use `git switch -c side1` or `git checkout -b side1`: it is more
> > > compact than `git branch ... && git checkout ...`.
> >
> > Yes, but less forgiving to later modification where I go and add
> > additional commits on one of the sides, because...
> >
> > >
> > > > +     test_write_lines 1 2 3 4 5 6 >numbers &&
> > > > +     echo hi >greeting &&
> > > > +     echo bar >whatever &&
> > > > +     git add numbers greeting whatever &&
> > > > +     git commit -m modify-stuff &&
> > > > +
> > > > +     git checkout side2 &&
> > >
> > > This could be written as `git checkout -b side2 HEAD^`, to make the setup
> > > more succinct.
> >
> > ...the presumption of HEAD^ is hardcoded and has to be parsed by
> > readers to understand the test.  It felt like more cognitive overhead
> > to me, in addition to being less malleable.
>
> Right. Different developers, different preferences. I wish we had a
> standard way in the test suite to initialize a test setup that _everybody_
> could agree to be succinct and helpful. So far, we use shell scripted Git
> commands to recreate an initial commit topology, but especially when
> comparing to existing test suites with fixtures that are not only
> well-documented but also easy to wrap your head around, I find Git's test
> suite awfully lacking. Mind you, the code _I_ introduced isn't stellar in
> this respect, either, not by a very far stretch.
>
> > > > +test_expect_success 'Barf on misspelled option' '
> > > > +     # Mis-spell with single "s" instead of double "s"
> > > > +     test_expect_code 129 git merge-tree --real --mesages FOOBAR side1 side2 2>expect &&
> > > > +
> > > > +     grep "error: unknown option.*mesages" expect
> > > > +'
> > >
> > > I do not think that this test case adds much, and we already test the
> > > `parse_options()` machinery elsewhere.
> >
> > It's more about verifying that exit codes of 0 & 1 are reserved for
> > "completed with no conflicts" and "completed with conflicts".  The 129
> > bit in this test is the important bit (and perhaps is well-known to
> > lots of other folks, but I thought it was worth highlighting).
>
> Fair enough.
>
> Ciao,
> Dscho

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts
  2022-01-05 17:27   ` [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
  2022-01-05 19:09     ` Ramsay Jones
@ 2022-01-07 19:36     ` Johannes Schindelin
  2022-01-07 22:12       ` Elijah Newren
  2022-01-08  1:28       ` Elijah Newren
  1 sibling, 2 replies; 57+ messages in thread
From: Johannes Schindelin @ 2022-01-07 19:36 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Taylor Blau, Johannes Altmanninger,
	Elijah Newren, Elijah Newren, Elijah Newren

Hi Elijah,

On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> Callers of `git merge-tree --real` might want an easy way to determine
> which files conflicted.  While they could potentially use the --messages
> option and parse the resulting messages written to that file, those
> messages are not meant to be machine readable.  Provide a simpler
> mechanism of having the user specify --unmerged-list=$FILENAME, and then
> write a NUL-separated list of unmerged filenames to the specified file.

This patch does what the commit message says, and it looks quite
plausible. However, in practice it seems that you need either a tree (if
the merge succeeded) or the list of conflicted files (if the merge
succeeded).

So while it looks relatively clean from the implementation's point of
view, the design itself could probably withstand a bit of consideration.

As I hinted earlier (to be precise, in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2201071602110.339@tvgsbejvaqbjf.bet/),
I had the chance in December to work on the server-side, using `merge-ort`
for a bit. In the following, I will talk about this a bit more than about
this particular patch, but I think it is highly relevant (not a tangent).

One of the things that became clear to me is that we really have an
either/or situation here. Either the merge succeeds, and we _need_ that
tree, or it fails, and we could not care less about the tree at all.

In fact, if the merge fails, we completely ignore the tree, and it would
be better if we would not even write out any Git objects in that case at
all: even just writing the objects would be quite costly at the
server-side scale.

So my (somewhat hacky) patches for a proof-of-concept produced _either_
the hash of the tree on `stdout`, _or_ a header saying that there were
conflicts followed by a NUL-separated list of file names.

Mind you, I did not even get to the point of analyzing things even more
deeply. My partner in crime and I only got to comparing the `merge-ort`
way to the libgit2-based way, trying to get them to compare as much
apples-to-apples as possible [*1*], and we found that even the time to
spawn the Git process (~1-3ms, with all overhead counted in) is _quite_
noticeable, at server-side scale.

Of course, the `merge-ort` performance was _really_ nice when doing
anything remotely complex, then `merge-ort` really blew the libgit2-based
merge out of the water. But that's not the common case. The common case
are merges that involve very few modified files, a single merge base, and
they don't conflict. And those can be processed by the libgit2-based
method within a fraction of the time it takes to even only so much as
spawn `git` (libgit2-based merges can complete in less than a fifth
millisecond, that's at most a fifth of the time it takes to merely run
`git merge-tree`).

The difference between 0.2-0.5ms for libgit2-based merges on the one hand,
and 1-3ms for `merge-ort`-based merges on the other hand, might not seem
like much, but you have to multiply it by the times such a merge is
performed on the server. Which is a _lot_. Way more often than I thought.

In this particular instance, there is a silver-lining: the libgit2-based
merge is not actually recursive. It is a three-way merge. Which means that
we first have to determine a merge base. In our case, this is done by
spawning a Git process anyway, so one of my ideas to move forward is to fold
that merge-base logic into `git merge-tree`, too.

Anyway, the short short is: whenever we can avoid unnecessary work, we
should do so. In the context of this patch, I would say that we should
avoid writing out a tree (and avoid printing its hash to `stdout`) if
there are merge conflicts. And we should avoid writing (and later reading)
a file, if we can get away with avoiding it.

At least in the default case, that is. We still might need a flag to
produce some more information about those merge conflicts. But even in
that case, it would be better to have a list of file names with the three
associated stages than to output the hash of a tree that contains
conflicts (and tons of files _without_ conflicts). The UI needs to
re-generate those conflicts anyway. And remember: a tree can contain
millions of files even if there is but a single conflict. It makes more
sense for `merge-tree --real` to output a concrete list of files that
conflicted, rather than expecting the caller to discern between conflicts
and non-conflicts by processing a tree object.

Maybe you agree with this rationale and re-design the `--real` mode to try
to avoid writing out files in the common case?

About the form of the patch itself: I was tempted to go with the
nitpicking spirit I see on the Git mailing list these days, especially
about the shell script code in the test scripts. But then I realized that
I find such nitpicking pretty unhelpful, myself. The code is good as-is,
even if I would write it differently. It is clear, and it does exactly
what it is supposed to do.

Thank you,
Dscho

Footnote *1*: I did not _quite_ get to the point of comparing the
`merge-ort` merges to the libgit2 ones, unfortunately. I was on my way to
add code to respect `merge.renames = false` so that we could _truly_
compare the `merge-ort` merges to the libgit2 merges (we really will want
to verify that the output is identical, before even considering to enable
recursive merges on the server side, and then only after studying the time
difference), and then had to take off due to the holidays. If you already
have that need to be able to turn off rename-detection on your radar, even
if only for a transitional period, I would be _so_ delighted.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index)
  2022-01-07 18:46   ` Christian Couder
@ 2022-01-07 19:59     ` Elijah Newren
  2022-01-07 21:26       ` René Scharfe
  0 siblings, 1 reply; 57+ messages in thread
From: Elijah Newren @ 2022-01-07 19:59 UTC (permalink / raw)
  To: Christian Couder
  Cc: Elijah Newren via GitGitGadget, git, Christian Couder,
	Taylor Blau, Johannes Altmanninger

On Fri, Jan 7, 2022 at 10:46 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Jan 5, 2022 at 6:27 PM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>
> > This series attempts to guess what kind of output would be wanted, basically
> > choosing:
> >
> >  * clean merge or conflict signalled via exit status
>
> (Maybe s/signalled/signaled/)

I can't determine the difference after a few Google searches, and both
seem to be in dictionaries with the same meaning so I'm having
difficulty figuring out which is preferred.  Usually my searches will
either suggest that one is a misspelling or at least bring up whether
one is a regional variance, but I'm not seeing anything of the sort.

It can't hurt to switch, though, so I'm happy to switch.

> Not sure that's the best way by default. I think it's very likely that
> many users will be interested in parsing the command ouput, and they
> might prefer that merge related errors be signaled in a different way
> than other errors.

That's fair.

I was thinking in terms of various plumbing commands: hash-object,
mktree, commit-tree, read-tree, write-tree and update-ref.  Output
from commands in that last can be fed as input to other commands and
be chained together to do various interesting and useful things.  I
have done that at various times in the past.  I thought merge-tree
might augment that category of commands (particularly since Peff
suggested to make the command be low-level at the summit), and thus
outputting just a tree (at least by default) would make the command be
a useful building block within that context.  That was part of my
reason for including the code snippet

   NEWTREE=$(git merge-tree --real $BRANCH1 $BRANCH2)
   test $? -eq 0 || die "There were conflicts..."
   NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 -p $BRANCH2)
   git update-ref $BRANCH1 $NEWCOMMIT

in the cover letter.

But merge-tree is much more likely to run into problems (i.e. into
merge conflicts), so maybe it doesn't belong in the same set, and the
NEWTREE definition perhaps deserves to have additional special case
command-line parsing that the user needs to do.

I'm curious about others' thoughts on this matter too.

> >  * stdout consists solely of printing the hash of the resulting tree (though
> >    that tree may include files that have conflict markers)
>
> Maybe users will want diffs, the conflicted list and other things on
> stdout, as they might want to parse it anyway, and it would be a
> burden to have to perform diffs, or get other interesting info in a
> different way or using a different process or call.

You mention the stdout thing both above and below, so I'll concentrate
here on the diffs part.

Do you have a specific usecase you have in mind where diffs are
wanted, separate from the two examples you gave in the other thread
(namely Ævar's misguided hack for looking for whether there were
conflicts, and a desire to just follow merge-tree's convoluted
precedent)?  I'd rather not add diffs pre-emptively on the basis that
users "might" want them, especially if they come with the huge gamut
of options Ævar was spitballing in [1] (some of which appeared to have
misguided assumptions relative to the possibility of renames and might
introduce edge and corner case bugs that'd be with us forever).  If we
don't have concrete usecases yet, I'd rather avoid adding such options
until we do have concrete usecases so we don't paint ourselves into a
corner.

[1] https://lore.kernel.org/git/211109.861r3qdpt8.gmgdl@evledraar.gmail.com/

> >  * new optional --messages flag for specifying a file where informational
> >    messages (e.g. conflict notices and files involved in three-way-content
> >    merges) can be written; by default, this output is simply discarded
> >  * new optional --conflicted-list flag for specifying a file where the names
> >    of conflicted-files can be written in a NUL-character-separated list
>
> It would be nice if output was printed on stdout when the above flags
> are used without argument.

Oh, that's an interesting idea.  The --conflicted-list flag, though,
separates filenames by NUL characters, for simplicity of parsing.  If
I'm printing them to stdout, would that be problematic? (If so, should
it instead print them in e.g. ls-tree format, where it escapes
filenames only when necessary)?

> Thanks for working on this!

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 4/8] merge-tree: implement real merges
  2022-01-07 17:26       ` Elijah Newren
  2022-01-07 18:22         ` Johannes Schindelin
@ 2022-01-07 20:56         ` Junio C Hamano
  2022-01-11 13:39           ` Johannes Schindelin
  1 sibling, 1 reply; 57+ messages in thread
From: Junio C Hamano @ 2022-01-07 20:56 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Johannes Schindelin, Elijah Newren via GitGitGadget,
	Git Mailing List, Christian Couder, Taylor Blau,
	Johannes Altmanninger

Elijah Newren <newren@gmail.com> writes:

>>    'git merge-tree' [--write-tree] <branch1> <branch2>
>>    'git merge-tree' [--demo-trivial-merge] <base-tree> <branch1> <branch2>
>>
>> That way, the old mode can still function, and can even at some stage be
>> deprecated and eventually removed.
>
> Ooh, interesting.

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

The internal result is expressed in this way:

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

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

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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index)
  2022-01-07 19:59     ` Elijah Newren
@ 2022-01-07 21:26       ` René Scharfe
  0 siblings, 0 replies; 57+ messages in thread
From: René Scharfe @ 2022-01-07 21:26 UTC (permalink / raw)
  To: Elijah Newren, Christian Couder
  Cc: Elijah Newren via GitGitGadget, git, Christian Couder,
	Taylor Blau, Johannes Altmanninger

Am 07.01.22 um 20:59 schrieb Elijah Newren:
> On Fri, Jan 7, 2022 at 10:46 AM Christian Couder
> <christian.couder@gmail.com> wrote:
>>
>> On Wed, Jan 5, 2022 at 6:27 PM Elijah Newren via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>
>>> This series attempts to guess what kind of output would be wanted, basically
>>> choosing:
>>>
>>>  * clean merge or conflict signalled via exit status
>>
>> (Maybe s/signalled/signaled/)
>
> I can't determine the difference after a few Google searches, and both
> seem to be in dictionaries with the same meaning so I'm having
> difficulty figuring out which is preferred.  Usually my searches will
> either suggest that one is a misspelling or at least bring up whether
> one is a regional variance, but I'm not seeing anything of the sort.
>
> It can't hurt to switch, though, so I'm happy to switch.

https://en.wiktionary.org/wiki/signal#Verb says: "present participle
(UK) signalling or (US) signaling".  And Documentation/SubmittingPatches
says: "We prefer to gradually reconcile the inconsistencies in favor of
US English".  So this seems to go in the right direction.

René

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts
  2022-01-07 19:36     ` Johannes Schindelin
@ 2022-01-07 22:12       ` Elijah Newren
  2022-02-22 13:03         ` Johannes Schindelin
  2022-01-08  1:28       ` Elijah Newren
  1 sibling, 1 reply; 57+ messages in thread
From: Elijah Newren @ 2022-01-07 22:12 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

On Fri, Jan 7, 2022 at 11:36 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Callers of `git merge-tree --real` might want an easy way to determine
> > which files conflicted.  While they could potentially use the --messages
> > option and parse the resulting messages written to that file, those
> > messages are not meant to be machine readable.  Provide a simpler
> > mechanism of having the user specify --unmerged-list=$FILENAME, and then
> > write a NUL-separated list of unmerged filenames to the specified file.
>
> This patch does what the commit message says, and it looks quite
> plausible. However, in practice it seems that you need either a tree (if
> the merge succeeded) or the list of conflicted files (if the merge
> succeeded).
>
> So while it looks relatively clean from the implementation's point of
> view, the design itself could probably withstand a bit of consideration.
>
> As I hinted earlier (to be precise, in
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2201071602110.339@tvgsbejvaqbjf.bet/),
> I had the chance in December to work on the server-side, using `merge-ort`
> for a bit. In the following, I will talk about this a bit more than about
> this particular patch, but I think it is highly relevant (not a tangent).
>
> One of the things that became clear to me is that we really have an
> either/or situation here. Either the merge succeeds, and we _need_ that
> tree, or it fails, and we could not care less about the tree at all.
>
> In fact, if the merge fails, we completely ignore the tree, and it would
> be better if we would not even write out any Git objects in that case at
> all: even just writing the objects would be quite costly at the
> server-side scale.
>
> So my (somewhat hacky) patches for a proof-of-concept produced _either_
> the hash of the tree on `stdout`, _or_ a header saying that there were
> conflicts followed by a NUL-separated list of file names.

Did you really check that it only produced one of these?  If you were
using ort, you wrote blob and tree objects to disk, even if you didn't
print their hash on stdout.

> Mind you, I did not even get to the point of analyzing things even more
> deeply. My partner in crime and I only got to comparing the `merge-ort`
> way to the libgit2-based way, trying to get them to compare as much
> apples-to-apples as possible [*1*], and we found that even the time to
> spawn the Git process (~1-3ms, with all overhead counted in) is _quite_
> noticeable, at server-side scale.

1-3ms?  I thought it was a good bit more than that.

> Of course, the `merge-ort` performance was _really_ nice when doing
> anything remotely complex, then `merge-ort` really blew the libgit2-based
> merge out of the water. But that's not the common case. The common case
> are merges that involve very few modified files, a single merge base, and
> they don't conflict. And those can be processed by the libgit2-based
> method within a fraction of the time it takes to even only so much as
> spawn `git` (libgit2-based merges can complete in less than a fifth
> millisecond, that's at most a fifth of the time it takes to merely run
> `git merge-tree`).
>
> The difference between 0.2-0.5ms for libgit2-based merges on the one hand,
> and 1-3ms for `merge-ort`-based merges on the other hand, might not seem
> like much, but you have to multiply it by the times such a merge is
> performed on the server. Which is a _lot_. Way more often than I thought.

Ah, I had been wondering a bit about process overhead.  Having
something that avoids that is definitely helpful.  I tried to design
ort such that its API is relatively clean and easy-to-use, and I
carefully tested and fixed it until it ran memory-leak free.  If
process execution overhead is such a big problem, perhaps you could
use these new functions via libgit.a instead of invoking a git process
and get the best of both worlds?

Unfortunately, in-process would run into problems with finding merge
bases, because the revision walking machinery is definitely not
leak-free -- an annoyance I had to deal with while attempting to clean
up merge-ort since the code I was using always did the merge-base
finding as well as the calls into merge-ort.  (I hear Ævar has some
not-yet-submitted patches that might help with memory leaks in the
revision walking machinery.)

> In this particular instance, there is a silver-lining: the libgit2-based
> merge is not actually recursive. It is a three-way merge. Which means that
> we first have to determine a merge base. In our case, this is done by
> spawning a Git process anyway, so one of my ideas to move forward is to fold
> that merge-base logic into `git merge-tree`, too.

The merge-base logic is already part of merge-tree in my patches.
What exactly did you do with merge-tree?  Were you using the existing
one and feeding it with a merge-base as an input, or did you write
your own that was more like Christian's that expected a merge base?

> Anyway, the short short is: whenever we can avoid unnecessary work, we
> should do so. In the context of this patch, I would say that we should
> avoid writing out a tree (and avoid printing its hash to `stdout`) if
> there are merge conflicts.

We can avoid printing its hash to `stdout`, but it'd take significant
work to avoid writing the tree to an object store, and it cannot be
done at the merge-tree level, it'd require replumbing some bits of
merge-ort (and making some already complex codepaths a bit more
complex, but that's a price I'm willing to pay for significant
performance wins).

Can I first suggest a simpler alternative that may give some
performance wins despite keeping the object writes:

We could make use of the tmp_objdir API that recently merged (see
b3cecf49ea ("tmp-objdir: new API for creating temporary writable
databases", 2021-12-06)).  We could put that tmp-objdir on /dev/shm or
other ramdisk, and if the merge is clean, migrate the contents into
the real object store.  Perhaps we could even pack those objects first
if there are a large number of them, but If it's not clean, we can
just discard the tmp-objdir. Also, as a further variant on this
alternative... packing these objects before migrating if there are a
sufficient number of them.  Now, this is rather unlikely to be needed
in general by merge-tree, because you only need to write new objects
(thus representing files modified on both sides, or whatever leading
trees are needed to reference the updated paths).  However, it might
matter for big enough repos with large enough numbers of changes on
both sides.  And it'd align nicely with my idea for server-side
rebases (where implementing this is on my TODO list), because
server-side rebases are much more likely to generate a large number of
objects.

But if you really want to learn about avoiding object writes...

If you really want to only write tree and blob objects when the merge
is clean, then as far as I can tell you have two options in regards to
the blobs: (1) you'll need to keep all files from three-way content
merges simultaneously in memory until you've determined if the result
is clean, so that you can then write the merged contents out as blobs
at the end.  Or (2) doing all the three-way content merges and keeping
track of whether the result for each is clean, and if they all turn
out to be clean, then redo every single one of those three-way content
merges afterwards so that you can actually write out the merged-result
to disk that time.

I think (2) would cost you a lot more work than you'd save, and I
worry that (1) might risk using large amounts of memory in the big
repositories if there are lots of changes on both sides.  While that
may be uncommon, I've seen folks try to merge things with lots of
changes on both sides, and you do have the server side to worry about
after all.

There are similar issues with the fact that trees are written as they
are processed as well.  Those would also require re-running afterwards
to re-generate the trees from the list of relevant-files-and-trees we
operate on.

However, if you are really curious about trying this out despite the
fact that I think you might be causing more work than you're avoiding
(or potentially requiring a lot more RAM), look for calls to
write_tree() (there are precisely two in merge-ort.c, one for
intermediate trees and one for the toplevel tree) and
write_object_file() (there are precisely two in merge-ort.c, one
within write_tree() for writing tree objects, and one in
handle_content_merge() for writing blob objects).

> And we should avoid writing (and later reading)
> a file, if we can get away with avoiding it.

Sure, we can do that.  Christian had a suggestion that if the
--conflicted-list didn't have an associated filename, then we just
print those to stdout.  Would that be to your liking?  And would you
prefer NUL-separated, or ls-tree style escaping of filenames?

> At least in the default case, that is. We still might need a flag to
> produce some more information about those merge conflicts. But even in
> that case, it would be better to have a list of file names with the three
> associated stages than to output the hash of a tree that contains
> conflicts (and tons of files _without_ conflicts). The UI needs to
> re-generate those conflicts anyway. And remember: a tree can contain
> millions of files even if there is but a single conflict. It makes more
> sense for `merge-tree --real` to output a concrete list of files that
> conflicted, rather than expecting the caller to discern between conflicts
> and non-conflicts by processing a tree object.

???

Where did I suggest that we discern between conflicts and
non-conflicts by processing a tree object?  That's not merely
something with atrocious performance, it's also utterly crazy from a
UI perspective, and is downright *impossible* to achieve in general
anyway.  (Failure to merge binary files.  modify/delete conflicts.
mode conflicts.  file/directory conflicts.  Various rename
permutatations.  There's all kinds of non-content conflicts that are
not representable in a tree, and which folks will miss if they attempt
to parse a tree and surmise what conflicts there were.)  Whatever I
wrote that might have suggested such a course of action needs some
serious rewording or clarification; I consider attempting that to be a
horrible idea.

The tree exists so that if people want to get extra information (and
presumably in a format similar to what they would find in their
working directory if they had asked `git merge` to merge those same
two branches on their laptop), then they can do so.  For example,
perhaps in the list of --conflicted-files they notice a file of
interest and want to ask, "What's found in this particular file?".
They can use the tree together with the filename to get that kind of
info.

> Maybe you agree with this rationale and re-design the `--real` mode to try
> to avoid writing out files in the common case?

I'm totally open to something like having --conflicted-list be given
without a filename (or maybe a '-') and write to stdout instead.

I'm amenable to various tradeoffs to improve performance, but I'm
worried that "not-writing-tree-and-blob object files" sounds like a
goal that might hurt performance rather than help it (either worse
performance in general due to the need to do things twice when the
merge is clean, or else a peak memory usage that is unmanageable and
causes problems).  Perhaps there's a smarter solution I'm not seeing,
or my worries about maximum memory usage are overblown.  However, if
you still want to pursue such core changes, I would also like to see
more performance measuring work to justify them -- especially since in
the common case merge-ort won't recurse into most directories and will
end up only writing a few object files (since only new blob or tree
objects need to be written).  In particular, I think we'd need to do
the following first:

  * Do a real comparison; libgit2 + the separate find-merge-base git
process, vs. the single `git merge-tree --real` call that handles
both.  Excluding the merge-base computation makes it apples to oranges
in my opinion.

  * While measuring the performance of the above, specifically use
trace2 to measure time spent in write_loose_object() in object-file.c.
See how much time object writing actually takes vs. everything else.

  * Try the tmp-objdir changes I suggested (examples of using the API
can be found by searching for "tmp_objdir" at
https://lore.kernel.org/git/d57ae218cf9eaee0b66db299ee1bba9b488b69b1.1640907369.git.gitgitgadget@gmail.com/
and https://lore.kernel.org/git/e1747ce00af7ab3170a69955b07d995d5321d6f3.1637020263.git.gitgitgadget@gmail.com/),
and then re-measure the performance particular in respect to
write_loose_object() as a percentage of overall time.

  * Get some kind of measure of the maximum number of three-way
content merges needed in merges and the overall size of holding all
the results in memory simultaneously.  If repositories have a million
files and there are merges involving a high enough percentage of files
changes on both sides, then that could certainly theoretically be
quite large.


I also think that doing something in-process (via linking against
libgit.a?) rather than forking `git merge-tree --real` would probably
net you _much_ bigger wins than avoiding writing these object files,
though it'd be unsafe without memory leak fixes for all the merge-base
computations.  That, of course, might not be the only reason you're
leery of such an approach.

> About the form of the patch itself: I was tempted to go with the
> nitpicking spirit I see on the Git mailing list these days, especially
> about the shell script code in the test scripts. But then I realized that
> I find such nitpicking pretty unhelpful, myself. The code is good as-is,
> even if I would write it differently. It is clear, and it does exactly
> what it is supposed to do.
>
> Thank you,
> Dscho
>
> Footnote *1*: I did not _quite_ get to the point of comparing the
> `merge-ort` merges to the libgit2 ones, unfortunately. I was on my way to
> add code to respect `merge.renames = false` so that we could _truly_
> compare the `merge-ort` merges to the libgit2 merges (we really will want
> to verify that the output is identical, before even considering to enable
> recursive merges on the server side, and then only after studying the time
> difference), and then had to take off due to the holidays. If you already
> have that need to be able to turn off rename-detection on your radar, even
> if only for a transitional period, I would be _so_ delighted.

Well, I had no intention of submitting it (and still don't), but I did
implement it a while back for folks who have needs for it in a
transitional period.  It's a pretty simple change.

https://github.com/newren/git/commit/5330a9de77f56f20e546acc65c924fc783f092e6
https://github.com/newren/git/commit/2e7e8d79b0995d352558608b6308060fbc055fd1

:-)

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 7/8] merge-tree: support saving merge messages to a separate file
  2022-01-07 18:07     ` Johannes Schindelin
@ 2022-01-08  1:02       ` Elijah Newren
  0 siblings, 0 replies; 57+ messages in thread
From: Elijah Newren @ 2022-01-08  1:02 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

On Fri, Jan 7, 2022 at 10:07 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Wed, 5 Jan 2022, Elijah Newren via GitGitGadget wrote:
>
> > diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> > index f7aa310f8c1..5f3f27f504d 100755
> > --- a/t/t4301-merge-tree-real.sh
> > +++ b/t/t4301-merge-tree-real.sh
> > @@ -78,4 +78,22 @@ test_expect_success 'Barf on too many arguments' '
> >       grep "^usage: git merge-tree" expect
> >  '
> >
> > +test_expect_success '--messages gives us the conflict notices and such' '
> > +     test_must_fail git merge-tree --real --messages=MSG_FILE side1 side2 &&
>
> Since we discern between exit status 1 (= merge conflict) and >1 (fatal
> error), we should probably use `test_expect_code` here.

Good point; will do.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts
  2022-01-07 19:36     ` Johannes Schindelin
  2022-01-07 22:12       ` Elijah Newren
@ 2022-01-08  1:28       ` Elijah Newren
  2022-02-22 13:05         ` Johannes Schindelin
  1 sibling, 1 reply; 57+ messages in thread
From: Elijah Newren @ 2022-01-08  1:28 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

Hi Dscho,

One more thing I forgot to ask...

On Fri, Jan 7, 2022 at 11:36 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
...
> Mind you, I did not even get to the point of analyzing things even more
> deeply. My partner in crime and I only got to comparing the `merge-ort`
> way to the libgit2-based way, trying to get them to compare as much
> apples-to-apples as possible [*1*], and we found that even the time to
> spawn the Git process (~1-3ms, with all overhead counted in) is _quite_
> noticeable, at server-side scale.
>
> Of course, the `merge-ort` performance was _really_ nice when doing
> anything remotely complex, then `merge-ort` really blew the libgit2-based
> merge out of the water. But that's not the common case. The common case
> are merges that involve very few modified files, a single merge base, and
> they don't conflict. And those can be processed by the libgit2-based
> method within a fraction of the time it takes to even only so much as
> spawn `git` (libgit2-based merges can complete in less than a fifth
> millisecond, that's at most a fifth of the time it takes to merely run
> `git merge-tree`).

Out of curiosity, are you only doing merges, or are you also
attempting server-side rebases in some fashion?

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 4/8] merge-tree: implement real merges
  2022-01-07 20:56         ` Junio C Hamano
@ 2022-01-11 13:39           ` Johannes Schindelin
  0 siblings, 0 replies; 57+ messages in thread
From: Johannes Schindelin @ 2022-01-11 13:39 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Elijah Newren, Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

Hi Junio,

On Fri, 7 Jan 2022, Junio C Hamano wrote:

> Elijah Newren <newren@gmail.com> writes:
>
> >>    'git merge-tree' [--write-tree] <branch1> <branch2>
> >>    'git merge-tree' [--demo-trivial-merge] <base-tree> <branch1> <branch2>
> >>
> >> That way, the old mode can still function, and can even at some stage be
> >> deprecated and eventually removed.
> >
> > Ooh, interesting.
>
> I wondered if we can _also_ extend the trivial-merge mode so that we
> do not have to call it "demo".
>
> The internal result is expressed in this way:
>
>     struct merge_list {
>             struct merge_list *next;
>             struct merge_list *link;	/* other stages for this object */
>
>             unsigned int stage : 2;
>             unsigned int mode;
>             const char *path;
>             struct blob *blob;
>     };
>
> because the command was not designed to resolve content level
> merges, but show the half-resolved state with the "stage" number.
> The "explanation" the command gives on the result is truly trivial,
> but there is no reason for it to stay that way.

The original `merge-tree` code outputs a diff, which I think has now been
firmly established as something a low-level merge tool should not do at
all.

So I am not sure how necessary it is to maintain the original UI. I don't
think it is a good UI. In fact, I am rather certain that we will want to
get rid of it.

We can keep it for backwards-compatibility for now, keeping it working for
existing users (if any!) by that 3-arg vs 2-arg trick, eventually
deprecate and then remove it.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts
  2022-01-07 22:12       ` Elijah Newren
@ 2022-02-22 13:03         ` Johannes Schindelin
  0 siblings, 0 replies; 57+ messages in thread
From: Johannes Schindelin @ 2022-02-22 13:03 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

Hi Elijah,

I meant to answer this mail much earlier, oh well. Sorry. I will only
answer the still open questions below, clipping the quoted text to save
every reader some time.

On Fri, 7 Jan 2022, Elijah Newren wrote:

> On Fri, Jan 7, 2022 at 11:36 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > So my (somewhat hacky) patches for a proof-of-concept produced
> > _either_ the hash of the tree on `stdout`, _or_ a header saying that
> > there were conflicts followed by a NUL-separated list of file names.
>
> Did you really check that it only produced one of these?  If you were
> using ort, you wrote blob and tree objects to disk, even if you didn't
> print their hash on stdout.

D'oh. No, I had not checked that.

> > Mind you, I did not even get to the point of analyzing things even more
> > deeply. My partner in crime and I only got to comparing the `merge-ort`
> > way to the libgit2-based way, trying to get them to compare as much
> > apples-to-apples as possible [*1*], and we found that even the time to
> > spawn the Git process (~1-3ms, with all overhead counted in) is _quite_
> > noticeable, at server-side scale.
>
> 1-3ms?  I thought it was a good bit more than that.

The absolute number really depends on a lot of factors, i.e. what is
relevant is the relative difference between in-process vs spawning a new
process.

> If process execution overhead is such a big problem, perhaps you could
> use these new functions via libgit.a instead of invoking a git process
> and get the best of both worlds?

For now, I solved this by combining multiple Git process invocations into
a single one, as described here:

> > In this particular instance, there is a silver-lining: the
> > libgit2-based merge is not actually recursive. It is a three-way
> > merge. Which means that we first have to determine a merge base. In
> > our case, this is done by spawning a Git process anyway, so one of my
> > ideas to move forward is to fold that merge-base logic into `git
> > merge-tree`, too.
>
> The merge-base logic is already part of merge-tree in my patches.
> What exactly did you do with merge-tree?  Were you using the existing
> one and feeding it with a merge-base as an input, or did you write
> your own that was more like Christian's that expected a merge base?

For an apples-to-apples comparison, I had to stay with the very same merge
base logic as before, i.e. originally I added a new option to `merge-tree`
that would take a merge base and then take a non-recursive path. In my
current version, it is merely an option that even determines said merge
base in the same process (and yes, it leaks memory, but that does not
matter because the process is short-lived anyway).

> > Anyway, the short short is: whenever we can avoid unnecessary work, we
> > should do so. In the context of this patch, I would say that we should
> > avoid writing out a tree (and avoid printing its hash to `stdout`) if
> > there are merge conflicts.
>
> We can avoid printing its hash to `stdout`, but it'd take significant
> work to avoid writing the tree to an object store, and it cannot be
> done at the merge-tree level, it'd require replumbing some bits of
> merge-ort (and making some already complex codepaths a bit more
> complex, but that's a price I'm willing to pay for significant
> performance wins).

Yes, let's leave things as-are. It is not worth the trouble.

> We could make use of the tmp_objdir API that recently merged (see
> b3cecf49ea ("tmp-objdir: new API for creating temporary writable
> databases", 2021-12-06)).  We could put that tmp-objdir on /dev/shm or
> other ramdisk, and if the merge is clean, migrate the contents into
> the real object store.  Perhaps we could even pack those objects first
> if there are a large number of them, but If it's not clean, we can
> just discard the tmp-objdir. Also, as a further variant on this
> alternative... packing these objects before migrating if there are a
> sufficient number of them.  Now, this is rather unlikely to be needed
> in general by merge-tree, because you only need to write new objects
> (thus representing files modified on both sides, or whatever leading
> trees are needed to reference the updated paths).  However, it might
> matter for big enough repos with large enough numbers of changes on
> both sides.  And it'd align nicely with my idea for server-side
> rebases (where implementing this is on my TODO list), because
> server-side rebases are much more likely to generate a large number of
> objects.
>
> But if you really want to learn about avoiding object writes...
>
> If you really want to only write tree and blob objects when the merge
> is clean, then as far as I can tell you have two options in regards to
> the blobs: (1) you'll need to keep all files from three-way content
> merges simultaneously in memory until you've determined if the result
> is clean, so that you can then write the merged contents out as blobs
> at the end.  Or (2) doing all the three-way content merges and keeping
> track of whether the result for each is clean, and if they all turn
> out to be clean, then redo every single one of those three-way content
> merges afterwards so that you can actually write out the merged-result
> to disk that time.
>
> I think (2) would cost you a lot more work than you'd save, and I
> worry that (1) might risk using large amounts of memory in the big
> repositories if there are lots of changes on both sides.  While that
> may be uncommon, I've seen folks try to merge things with lots of
> changes on both sides, and you do have the server side to worry about
> after all.
>
> There are similar issues with the fact that trees are written as they
> are processed as well.  Those would also require re-running afterwards
> to re-generate the trees from the list of relevant-files-and-trees we
> operate on.
>
> However, if you are really curious about trying this out despite the
> fact that I think you might be causing more work than you're avoiding
> (or potentially requiring a lot more RAM), look for calls to
> write_tree() (there are precisely two in merge-ort.c, one for
> intermediate trees and one for the toplevel tree) and
> write_object_file() (there are precisely two in merge-ort.c, one
> within write_tree() for writing tree objects, and one in
> handle_content_merge() for writing blob objects).

Thank you for this thorough analysis.

I am a big fan of crossing bridges when they are reached, and not miles
before that. So _iff_ it turns out that the speed, or the potential
cluttering with objects, should present a problem in the future, I am
inclined to follow the tmp-objdir route you described above (thank you for
pointing it out, I had not made the connection to this here scenario).

> > Footnote *1*: I did not _quite_ get to the point of comparing the
> > `merge-ort` merges to the libgit2 ones, unfortunately. I was on my way to
> > add code to respect `merge.renames = false` so that we could _truly_
> > compare the `merge-ort` merges to the libgit2 merges (we really will want
> > to verify that the output is identical, before even considering to enable
> > recursive merges on the server side, and then only after studying the time
> > difference), and then had to take off due to the holidays. If you already
> > have that need to be able to turn off rename-detection on your radar, even
> > if only for a transitional period, I would be _so_ delighted.
>
> Well, I had no intention of submitting it (and still don't), but I did
> implement it a while back for folks who have needs for it in a
> transitional period.  It's a pretty simple change.
>
> https://github.com/newren/git/commit/5330a9de77f56f20e546acc65c924fc783f092e6
> https://github.com/newren/git/commit/2e7e8d79b0995d352558608b6308060fbc055fd1

Thank you _so_ much for that. It was super helpful, and it allowed me to
gather enough evidence to justify continuing to work on this code.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts
  2022-01-08  1:28       ` Elijah Newren
@ 2022-02-22 13:05         ` Johannes Schindelin
  0 siblings, 0 replies; 57+ messages in thread
From: Johannes Schindelin @ 2022-02-22 13:05 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Taylor Blau, Johannes Altmanninger

Hi Elijah,

On Fri, 7 Jan 2022, Elijah Newren wrote:

> On Fri, Jan 7, 2022 at 11:36 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> ...
> > Mind you, I did not even get to the point of analyzing things even more
> > deeply. My partner in crime and I only got to comparing the `merge-ort`
> > way to the libgit2-based way, trying to get them to compare as much
> > apples-to-apples as possible [*1*], and we found that even the time to
> > spawn the Git process (~1-3ms, with all overhead counted in) is _quite_
> > noticeable, at server-side scale.
> >
> > Of course, the `merge-ort` performance was _really_ nice when doing
> > anything remotely complex, then `merge-ort` really blew the libgit2-based
> > merge out of the water. But that's not the common case. The common case
> > are merges that involve very few modified files, a single merge base, and
> > they don't conflict. And those can be processed by the libgit2-based
> > method within a fraction of the time it takes to even only so much as
> > spawn `git` (libgit2-based merges can complete in less than a fifth
> > millisecond, that's at most a fifth of the time it takes to merely run
> > `git merge-tree`).
>
> Out of curiosity, are you only doing merges, or are you also
> attempting server-side rebases in some fashion?

One step after another. For now, I am focusing on merges.

But yes, rebases are on my radar, too, and I am very grateful for the
head-start you provided in `t/helper/test-fast-rebase.c` (and for the pun
therein).

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 57+ messages in thread

end of thread, other threads:[~2022-02-22 13:05 UTC | newest]

Thread overview: 57+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
2021-12-31  5:03 ` [PATCH 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
2021-12-31  5:03 ` [PATCH 2/8] merge-tree: move logic for existing merge into new function Elijah Newren via GitGitGadget
2022-01-01 20:11   ` Johannes Altmanninger
2022-01-01 20:17     ` Elijah Newren
2021-12-31  5:03 ` [PATCH 3/8] merge-tree: add option parsing and initial shell for real merge function Elijah Newren via GitGitGadget
2021-12-31  5:04 ` [PATCH 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
2022-01-01 20:08   ` Johannes Altmanninger
2022-01-01 21:11     ` Elijah Newren
2022-01-03 12:23   ` Fabian Stelzer
2022-01-03 16:37     ` Elijah Newren
2021-12-31  5:04 ` [PATCH 5/8] merge-ort: split out a separate display_update_messages() function Elijah Newren via GitGitGadget
2022-01-03 12:15   ` Fabian Stelzer
2022-01-03 12:25     ` Fabian Stelzer
2021-12-31  5:04 ` [PATCH 6/8] merge-ort: allow update messages to be written to different file stream Elijah Newren via GitGitGadget
2022-01-01 20:08   ` Johannes Altmanninger
2022-01-01 20:19     ` Elijah Newren
2021-12-31  5:04 ` [PATCH 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
2022-01-03 12:31   ` Fabian Stelzer
2022-01-03 16:51     ` Elijah Newren
2022-01-03 17:22       ` Fabian Stelzer
2022-01-03 19:46         ` Elijah Newren
2022-01-04 13:05           ` Fabian Stelzer
2022-01-03 12:35   ` Fabian Stelzer
2022-01-03 16:55     ` Elijah Newren
2021-12-31  5:04 ` [PATCH 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 2/8] merge-tree: move logic for existing merge into new function Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 3/8] merge-tree: add option parsing and initial shell for real merge function Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
2022-01-07 15:30     ` Johannes Schindelin
2022-01-07 17:26       ` Elijah Newren
2022-01-07 18:22         ` Johannes Schindelin
2022-01-07 19:15           ` Elijah Newren
2022-01-07 20:56         ` Junio C Hamano
2022-01-11 13:39           ` Johannes Schindelin
2022-01-07 18:12     ` Christian Couder
2022-01-07 19:09       ` Elijah Newren
2022-01-05 17:27   ` [PATCH v2 5/8] merge-ort: split out a separate display_update_messages() function Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 6/8] merge-ort: allow update messages to be written to different file stream Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
2022-01-07 18:07     ` Johannes Schindelin
2022-01-08  1:02       ` Elijah Newren
2022-01-05 17:27   ` [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
2022-01-05 19:09     ` Ramsay Jones
2022-01-05 19:17       ` Elijah Newren
2022-01-07 19:36     ` Johannes Schindelin
2022-01-07 22:12       ` Elijah Newren
2022-02-22 13:03         ` Johannes Schindelin
2022-01-08  1:28       ` Elijah Newren
2022-02-22 13:05         ` Johannes Schindelin
2022-01-05 20:18   ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Junio C Hamano
2022-01-05 22:35     ` Elijah Newren
2022-01-07 18:46   ` Christian Couder
2022-01-07 19:59     ` Elijah Newren
2022-01-07 21:26       ` René Scharfe

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).