[PATCH 0/2] [RFC] Switch default merge backend from recursive to ort

This is an RFC series designed to spur feedback about switching the default
merge backend (reviewing the patches is of secondary importance at this
point). Some questions:

 * Are there things others want before this series is considered for
   inclusion?
 * What kind of timeline do others think is reasonable?
 * Would it be beneficial to let this series sit in 'next' for an extended
   duration to gain more feedback?

Some potentially useful context in relation to the above:

 * I've personally used the ort backend for well over a year
 * I have ~50 testers using ort as the default merge backend since Nov.
   2020.
 * ort fixes known bugs in recursive, and there are no known regressions
   (more testers may change that)
 * ort is significantly faster than recursive
 * ort provides one new feature already, and enables more that are on the
   way
 * The commit message of patch 1 has more details about the last three items
   above

So...thoughts?

Elijah Newren (2):
  Change default merge backend from recursive to ort
  Update docs for change of default merge backend

 Documentation/git-rebase.txt                  | 17 ++--
 Documentation/gitfaq.txt                      |  2 +-
 Documentation/merge-options.txt               |  4 +-
 Documentation/merge-strategies.txt            | 98 +++++++++++--------
 .../technical/directory-rename-detection.txt  | 14 +--
 Documentation/user-manual.txt                 |  2 +-
 builtin/merge.c                               | 10 +-
 builtin/rebase.c                              |  2 +-
 sequencer.c                                   |  4 +-
 9 files changed, 89 insertions(+), 64 deletions(-)


base-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1055%2Fnewren%2Fort-default-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1055/newren/ort-default-v1
Pull-Request: https://github.com/git/git/pull/1055
-- 
gitgitgadget

From: Elijah Newren <redacted>

There are a few reasons to switch the default:
  * Correctness
  * Extensibility
  * Performance

I'll provide some summaries about each.

=== Correctness ===

The original impetus for a new merge backend was to fix issues that were
difficult to fix within recursive's design.  The success with this goal
is perhaps most easily demonstrated by running the following:

  $ git grep -2 KNOWN_FAILURE t/ | grep -A 4 GIT_TEST_MERGE_ALGORITHM
  $ git grep test_expect_merge_algorithm.failure.success t/
  $ git grep test_expect_merge_algorithm.success.failure t/

In order, these greps show:

  * Seven sets of submodule tests (10 total tests) that fail with
    recursive but succeed with ort
  * 22 other tests that fail with recursive, but succeed with ort
  * 0 tests that pass with recursive, but fail with ort

=== Extensibility ===

Being able to perform merges without touching the working tree or index
makes it possible to create new features that were difficult with the
old backend:

  * Merging, cherry-picking, rebasing, reverting in bare repositories...
    or just on branches that aren't checked out.

  * `git diff AUTO_MERGE` -- ability to see what changes the user has
    made to resolve conflicts so far (see commit 5291828df8 ("merge-ort:
    write $GIT_DIR/AUTO_MERGE whenever we hit a conflict", 2021-03-20)

  * A --remerge-diff option for log/show, used to show diffs for merges
    that display the difference between what an automatic merge would
    have created and what was recorded in the merge.  (This option will
    often result in an empty diff because many merges are clean, but for
    the non-clean ones it will show how conflicts were fixed including
    the removal of conflict markers, and also show additional changes
    made outside of conflict regions to e.g. fix semantic conflicts.)

  * A --remerge-diff-only option for log/show, similar to --remerge-diff
    but also showing how cherry-picks or reverts differed from what an
    automatic cherry-pick or revert would provide.

The last three have been implemented already (though only one has been
submitted upstream so far; the others were waiting for performance work
to complete), and I still plan to implement the first one.

=== Performance ===

I'll quote from the summary of my final optimization for merge-ort
(while fixing the testcase name from 'no-renames' to 'few-renames'):

                               Timings

                                          Infinite
                 merge-       merge-     Parallelism
                recursive    recursive    of rename    merge-ort
                 v2.30.0      current     detection     current
                ----------   ---------   -----------   ---------
few-renames:      18.912 s    18.030 s     11.699 s     198.3 ms
mega-renames:   5964.031 s   361.281 s    203.886 s     661.8 ms
just-one-mega:   149.583 s    11.009 s      7.553 s     264.6 ms

                           Speedup factors

                                          Infinite
                 merge-       merge-     Parallelism
                recursive    recursive    of rename
                 v2.30.0      current     detection    merge-ort
                ----------   ---------   -----------   ---------
few-renames:        1           1.05         1.6           95
mega-renames:       1          16.5         29           9012
just-one-mega:      1          13.6         20            565

And, for partial clone users:

             Factor reduction in number of objects needed

                                          Infinite
                 merge-       merge-     Parallelism
                recursive    recursive    of rename
                 v2.30.0      current     detection    merge-ort
                ----------   ---------   -----------   ---------
mega-renames:       1            1            1          181.3

Signed-off-by: Elijah Newren <redacted>
---
 builtin/merge.c  | 10 ++++++++--
 builtin/rebase.c |  2 +-
 sequencer.c      |  4 ++--
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f54..217c5061eb6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -88,9 +88,9 @@ static int autostash;
 static int no_verify;
 
 static struct strategy all_strategy[] = {
-	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
+	{ "recursive",  NO_TRIVIAL },
 	{ "octopus",    DEFAULT_OCTOPUS },
-	{ "ort",        NO_TRIVIAL },
+	{ "ort",        DEFAULT_TWOHEAD | NO_TRIVIAL },
 	{ "resolve",    0 },
 	{ "ours",       NO_FAST_FORWARD | NO_TRIVIAL },
 	{ "subtree",    NO_FAST_FORWARD | NO_TRIVIAL },

@@ -1484,6 +1484,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			fast_forward = FF_NO;
 	}
 
+	if (!use_strategies && !pull_twohead &&
+	    remoteheads && !remoteheads->next) {
+		char *default_strategy = getenv("GIT_TEST_MERGE_ALGORITHM");
+		if (default_strategy)
+			append_strategy(get_strategy(default_strategy));
+	}
 	if (!use_strategies) {
 		if (!remoteheads)
 			; /* already up-to-date */

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 12f093121d9..587a2f1d299 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -1713,7 +1713,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		int i;
 
 		if (!options.strategy)
-			options.strategy = "recursive";
+			options.strategy = "ort";
 
 		strbuf_reset(&buf);
 		for (i = 0; i < strategy_options.nr; i++)

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..a98de9a8d15 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -636,7 +636,7 @@ static int do_recursive_merge(struct repository *r,
 	for (i = 0; i < opts->xopts_nr; i++)
 		parse_merge_opt(&o, opts->xopts[i]);
 
-	if (opts->strategy && !strcmp(opts->strategy, "ort")) {
+	if (!opts->strategy || strcmp(opts->strategy, "recursive")) {
 		memset(&result, 0, sizeof(result));
 		merge_incore_nonrecursive(&o, base_tree, head_tree, next_tree,
 					    &result);

@@ -3968,7 +3968,7 @@ static int do_merge(struct repository *r,
 	o.branch2 = ref_name.buf;
 	o.buffer_output = 2;
 
-	if (opts->strategy && !strcmp(opts->strategy, "ort")) {
+	if (!opts->strategy || strcmp(opts->strategy, "recursive")) {
 		/*
 		 * TODO: Should use merge_incore_recursive() and
 		 * merge_switch_to_result(), skipping the call to

-- 
gitgitgadget

From: Elijah Newren <redacted>

Make multiple documentation updates to bring things up to date as we
change the default merge backend...

Add a section for `ort` within merge-strategies.txt; while it accepts
the same flags as `recursive` (even if it ignores three of them) and is
meant as a drop in replacement, it still makes sense to explicitly cover
it.

Change several locations in the docs that referred to `recursive` as the
default merge backend, and fix a few that said or implied that only
`recursive` had certain abilities (such as rename detection).

Fix up some wording in directory-rename-detection.txt due to some
restructurings performed while optimizing both the ort backend and the
rename detection machinery.

Drop the "is considered generally safe and fast" from the description of
the `resolve` strategy, since that implies the other strategies are not.
While such an implication may have been true in 2005 when written, it
may well be that `ort` is faster today (since it does not need to
recurse into all directories).  Also, since `resolve` was the default
for less than a year while `recursive` has been the default for a decade
and a half, I think `recursive` is more battle-tested than `resolve` is.
Move the description of `resolve` near `octopus` and `ours` while at it
since it is no longer the default merge algorithm and hasn't been for a
very long time.

Signed-off-by: Elijah Newren <redacted>
---
 Documentation/git-rebase.txt                  | 17 ++--
 Documentation/gitfaq.txt                      |  2 +-
 Documentation/merge-options.txt               |  4 +-
 Documentation/merge-strategies.txt            | 98 +++++++++++--------
 .../technical/directory-rename-detection.txt  | 14 +--
 Documentation/user-manual.txt                 |  2 +-
 6 files changed, 78 insertions(+), 59 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 55af6fd24e2..41b68423444 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt

@@ -340,9 +340,10 @@ See also INCOMPATIBLE OPTIONS below.
 
 -m::
 --merge::
-	Use merging strategies to rebase.  When the recursive (default) merge
-	strategy is used, this allows rebase to be aware of renames on the
-	upstream side.  This is the default.
+	Use merging strategies to rebase.  When either the `ort`
+	(default) or `recursive` merge strategy is used, this allows
+	rebase to be aware of renames on the upstream side.  This is the
+	default.
 +
 Note that a rebase merge works by replaying each commit from the working
 branch on top of the <upstream> branch.  Because of this, when a merge

@@ -355,8 +356,8 @@ See also INCOMPATIBLE OPTIONS below.
 -s <strategy>::
 --strategy=<strategy>::
 	Use the given merge strategy.
-	If there is no `-s` option 'git merge-recursive' is used
-	instead.  This implies --merge.
+	If there is no `-s` option the `ort` strategy is the default.
+	This implies --merge.
 +
 Because 'git rebase' replays each commit from the working branch
 on top of the <upstream> branch using the given strategy, using

@@ -369,7 +370,7 @@ See also INCOMPATIBLE OPTIONS below.
 --strategy-option=<strategy-option>::
 	Pass the <strategy-option> through to the merge strategy.
 	This implies `--merge` and, if no strategy has been
-	specified, `-s recursive`.  Note the reversal of 'ours' and
+	specified, `-s ort`.  Note the reversal of 'ours' and
 	'theirs' as noted above for the `-m` option.
 +
 See also INCOMPATIBLE OPTIONS below.

@@ -530,7 +531,7 @@ The `--rebase-merges` mode is similar in spirit to the deprecated
 where commits can be reordered, inserted and dropped at will.
 +
 It is currently only possible to recreate the merge commits using the
-`recursive` merge strategy; Different merge strategies can be used only via
+`ort` merge strategy; different merge strategies can be used only via
 explicit `exec git merge -s <strategy> [...]` commands.
 +
 See also REBASING MERGES and INCOMPATIBLE OPTIONS below.

@@ -1219,7 +1220,7 @@ successful merge so that the user can edit the message.
 If a `merge` command fails for any reason other than merge conflicts (i.e.
 when the merge operation did not even start), it is rescheduled immediately.
 
-At this time, the `merge` command will *always* use the `recursive`
+At this time, the `merge` command will *always* use the `ort`
 merge strategy for regular merges, and `octopus` for octopus merges,
 with no way to choose a different one. To work around
 this, an `exec` command can be used to call `git merge` explicitly,

diff --git a/Documentation/gitfaq.txt b/Documentation/gitfaq.txt
index afdaeab8503..072bf84fa8a 100644
--- a/Documentation/gitfaq.txt
+++ b/Documentation/gitfaq.txt

@@ -275,7 +275,7 @@ best to always use a regular merge commit.
 
 [[merge-two-revert-one]]
 If I make a change on two branches but revert it on one, why does the merge of those branches include the change?::
-	By default, when Git does a merge, it uses a strategy called the recursive
+	By default, when Git does a merge, it uses a strategy called the ort
 	strategy, which does a fancy three-way merge.  In such a case, when Git
 	performs the merge, it considers exactly three points: the two heads and a
 	third point, called the _merge base_, which is usually the common ancestor of

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index eb0aabd396f..72b53188504 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt

@@ -112,8 +112,8 @@ With --squash, --commit is not allowed, and will fail.
 	Use the given merge strategy; can be supplied more than
 	once to specify them in the order they should be tried.
 	If there is no `-s` option, a built-in list of strategies
-	is used instead ('git merge-recursive' when merging a single
-	head, 'git merge-octopus' otherwise).
+	is used instead (`ort` when merging a single head,
+	`octopus` otherwise).
 
 -X <option>::
 --strategy-option=<option>::

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 2912de706bf..6caf3fd6486 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt

@@ -6,28 +6,23 @@ backend 'merge strategies' to be chosen with `-s` option.  Some strategies
 can also take their own options, which can be passed by giving `-X<option>`
 arguments to `git merge` and/or `git pull`.
 
-resolve::
-	This can only resolve two heads (i.e. the current branch
-	and another branch you pulled from) using a 3-way merge
-	algorithm.  It tries to carefully detect criss-cross
-	merge ambiguities and is considered generally safe and
-	fast.
-
-recursive::
-	This can only resolve two heads using a 3-way merge
-	algorithm.  When there is more than one common
-	ancestor that can be used for 3-way merge, it creates a
-	merged tree of the common ancestors and uses that as
-	the reference tree for the 3-way merge.  This has been
-	reported to result in fewer merge conflicts without
-	causing mismerges by tests done on actual merge commits
-	taken from Linux 2.6 kernel development history.
-	Additionally this can detect and handle merges involving
-	renames, but currently cannot make use of detected
-	copies.  This is the default merge strategy when pulling
-	or merging one branch.
+ort::
+	This is the default merge strategy when pulling or merging one
+	branch.  This strategy can only resolve two heads using a
+	3-way merge algorithm.  When there is more than one common
+	ancestor that can be used for 3-way merge, it creates a merged
+	tree of the common ancestors and uses that as the reference
+	tree for the 3-way merge.  This has been reported to result in
+	fewer merge conflicts without causing mismerges by tests done
+	on actual merge commits taken from Linux 2.6 kernel
+	development history.  Additionally this strategy can detect
+	and handle merges involving renames.  It does not make use of
+	detected copies.  The name for this algorithm is an acronym
+	("Ostensibly Recursive's Twin") and came from the fact that it
+	was written as a replacement for the previous default
+	algorithm, recursive.
 +
-The 'recursive' strategy can take the following options:
+The 'ort' strategy can take the following options:
 
 ours;;
 	This option forces conflicting hunks to be auto-resolved cleanly by

@@ -43,19 +38,6 @@ theirs;;
 	This is the opposite of 'ours'; note that, unlike 'ours', there is
 	no 'theirs' merge strategy to confuse this merge option with.
 
-patience;;
-	With this option, 'merge-recursive' spends a little extra time
-	to avoid mismerges that sometimes occur due to unimportant
-	matching lines (e.g., braces from distinct functions).  Use
-	this when the branches to be merged have diverged wildly.
-	See also linkgit:git-diff[1] `--patience`.
-
-diff-algorithm=[patience|minimal|histogram|myers];;
-	Tells 'merge-recursive' to use a different diff algorithm, which
-	can help avoid mismerges that occur due to unimportant matching
-	lines (such as braces from distinct functions).  See also
-	linkgit:git-diff[1] `--diff-algorithm`.
-
 ignore-space-change;;
 ignore-all-space;;
 ignore-space-at-eol;;

@@ -84,11 +66,6 @@ no-renormalize;;
 	Disables the `renormalize` option.  This overrides the
 	`merge.renormalize` configuration variable.
 
-no-renames;;
-	Turn off rename detection. This overrides the `merge.renames`
-	configuration variable.
-	See also linkgit:git-diff[1] `--no-renames`.
-
 find-renames[=<n>];;
 	Turn on rename detection, optionally setting the similarity
 	threshold.  This is the default. This overrides the

@@ -105,6 +82,45 @@ subtree[=<path>];;
 	is prefixed (or stripped from the beginning) to make the shape of
 	two trees to match.
 
+recursive::
+	This can only resolve two heads using a 3-way merge
+	algorithm.  When there is more than one common
+	ancestor that can be used for 3-way merge, it creates a
+	merged tree of the common ancestors and uses that as
+	the reference tree for the 3-way merge.  This has been
+	reported to result in fewer merge conflicts without
+	causing mismerges by tests done on actual merge commits
+	taken from Linux 2.6 kernel development history.
+	Additionally this can detect and handle merges involving
+	renames.  It does not make use of detected copies.
++
+The 'recursive' strategy takes the same options as 'ort'.  However,
+there are three additional options that 'ort' ignores (not documented
+above) that are potentially useful with the 'recursive' strategy:
+
+patience;;
+	Deprecated shorthand for diff-algorithm=patience.
+
+diff-algorithm=[patience|minimal|histogram|myers];;
+	Use a different diff algorithm while merging, which can help
+	avoid mismerges that occur due to unimportant matching lines
+	(such as braces from distinct functions).  See also
+	linkgit:git-diff[1] `--diff-algorithm`.  Note that `ort`
+	specifically uses diff-algorithm=histogram, while `recursive`
+	defaults to the `diff.algorithm` config setting.
+
+no-renames;;
+	Turn off rename detection, which can be computationally much
+	more expensive for the `recursive` strategy than for `ort`.
+	This overrides the `merge.renames` configuration variable.  See
+	also linkgit:git-diff[1] `--no-renames`.
+
+resolve::
+	This can only resolve two heads (i.e. the current branch
+	and another branch you pulled from) using a 3-way merge
+	algorithm.  It tries to carefully detect criss-cross
+	merge ambiguities.  It cannot handle renames.
+
 octopus::
 	This resolves cases with more than two heads, but refuses to do
 	a complex merge that needs manual resolution.  It is

@@ -121,13 +137,13 @@ ours::
 	the 'recursive' merge strategy.
 
 subtree::
-	This is a modified recursive strategy. When merging trees A and
+	This is a modified ort strategy. When merging trees A and
 	B, if B corresponds to a subtree of A, B is first adjusted to
 	match the tree structure of A, instead of reading the trees at
 	the same level. This adjustment is also done to the common
 	ancestor tree.
 
-With the strategies that use 3-way merge (including the default, 'recursive'),
+With the strategies that use 3-way merge (including the default, 'ort'),
 if a change is made on both branches, but later reverted on one of the
 branches, that change will be present in the merged result; some people find
 this behavior confusing.  It occurs because only the heads and the merge base

diff --git a/Documentation/technical/directory-rename-detection.txt b/Documentation/technical/directory-rename-detection.txt
index 49b83ef3cc4..029ee2cedc5 100644
--- a/Documentation/technical/directory-rename-detection.txt
+++ b/Documentation/technical/directory-rename-detection.txt

@@ -2,9 +2,9 @@ Directory rename detection
 ==========================
 
 Rename detection logic in diffcore-rename that checks for renames of
-individual files is aggregated and analyzed in merge-recursive for cases
-where combinations of renames indicate that a full directory has been
-renamed.
+individual files is also aggregated there and then analyzed in either
+merge-ort or merge-recursive for cases where combinations of renames
+indicate that a full directory has been renamed.
 
 Scope of abilities
 ------------------

@@ -88,9 +88,11 @@ directory rename detection support in:
     Folks have requested in the past that `git diff` detect directory
     renames and somehow simplify its output.  It is not clear whether this
     would be desirable or how the output should be simplified, so this was
-    simply not implemented.  Further, to implement this, directory rename
-    detection logic would need to move from merge-recursive to
-    diffcore-rename.
+    simply not implemented.  Also, while diffcore-rename has most of the
+    logic for detecting directory renames, some of the logic is still found
+    within merge-ort and merge-recursive.  Fully supporting directory
+    rename detection in diffs would require copying or moving the remaining
+    bits of logic to the diff machinery.
 
   * am
 

diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index 96240598e3f..865074bed4e 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt

@@ -3190,7 +3190,7 @@ that *updated* thing--the old state that you added originally ends up
 not being pointed to by any commit or tree, so it's now a dangling blob
 object.
 
-Similarly, when the "recursive" merge strategy runs, and finds that
+Similarly, when the "ort" merge strategy runs, and finds that
 there are criss-cross merges and thus more than one merge base (which is
 fairly unusual, but it does happen), it will generate one temporary
 midway tree (or possibly even more, if you had lots of criss-crossing

-- 
gitgitgadget

On 7/31/2021 8:07 PM, Elijah Newren via GitGitGadget wrote:

From: Elijah Newren <redacted>

...

nit: I feel like quotes around "ort" would be beneficial here. It would have
also helped the previous version, too, in my opinion.

nit: Quotes around "recursive" might be useful here, too.

 +
-The 'recursive' strategy can take the following options:
+The 'ort' strategy can take the following options:

(Like these quotes.)

Other than my nits, these doc updates are solid.

Thanks,
-Stolee

On 7/31/2021 8:07 PM, Elijah Newren via GitGitGadget wrote:

This is an RFC series designed to spur feedback about switching the default
merge backend (reviewing the patches is of secondary importance at this
point). Some questions:

 * Are there things others want before this series is considered for
   inclusion?
 * What kind of timeline do others think is reasonable?
 * Would it be beneficial to let this series sit in 'next' for an extended
   duration to gain more feedback?

Some potentially useful context in relation to the above:

 * I've personally used the ort backend for well over a year
 * I have ~50 testers using ort as the default merge backend since Nov.
   2020.
 * ort fixes known bugs in recursive, and there are no known regressions
   (more testers may change that)
 * ort is significantly faster than recursive
 * ort provides one new feature already, and enables more that are on the
   way
 * The commit message of patch 1 has more details about the last three items
   above

So...thoughts?

I fully endorse this change as soon as possible. I've applied the patches
you supplied here and submitted a PR to microsoft/git [1] to take them.

[1] https://github.com/microsoft/git/pull/404

I've done my own share of testing on some of our private monorepos to see
how ORT compares to the recursive algorithm. My data is not perhaps as
rigorous as yours, but I did notice that the ORT algorithm was consistently
within the 5-6 second range while the recursive algorithm would vary within
the 7-20 second range (and some outliers near 30s).

Of course, I'm particularly excited about the benefits to the sparse index
work. I also have a prototype of a 'git merge' integration with sparse
index which was not very hard because the ORT strategy does not use the
index as a data structure. With that change, my tests dropped to between
0.5s and 1.5 seconds. (This shows just how much my earlier timings were
stuck on index reads and writes.) Now, the largest indicator of time is
how long it takes to resolve text conflicts.

Thanks! What a monumental effort.

-Stolee

On Sun, Aug 01 2021, Elijah Newren via GitGitGadget wrote:

I might spot more tiny issues, but it looks like our error messaging
needs updating for 14c4586c2df (merge,rebase,revert: select ort or
recursive by config or environment, 2020-11-02).

I.e. we die on "Unknown option for merge-recursive", presumably that
should be updated to indicate that we might call one of
merge_recursive() or merge_ort_recursive() now.

And perhaps this in sequencer.c:

    that represents the "current" state for merge-recursive[...]

On Mon, Aug 2, 2021 at 9:05 AM Derrick Stolee [off-list ref] wrote:

On 7/31/2021 8:07 PM, Elijah Newren via GitGitGadget wrote:

quoted

This is an RFC series designed to spur feedback about switching the default
merge backend (reviewing the patches is of secondary importance at this
point). Some questions:

 * Are there things others want before this series is considered for
   inclusion?
 * What kind of timeline do others think is reasonable?
 * Would it be beneficial to let this series sit in 'next' for an extended
   duration to gain more feedback?

Some potentially useful context in relation to the above:

 * I've personally used the ort backend for well over a year
 * I have ~50 testers using ort as the default merge backend since Nov.
   2020.
 * ort fixes known bugs in recursive, and there are no known regressions
   (more testers may change that)
 * ort is significantly faster than recursive
 * ort provides one new feature already, and enables more that are on the
   way
 * The commit message of patch 1 has more details about the last three items
   above

So...thoughts?

I fully endorse this change as soon as possible. I've applied the patches
you supplied here and submitted a PR to microsoft/git [1] to take them.

[1] https://github.com/microsoft/git/pull/404

I've done my own share of testing on some of our private monorepos to see
how ORT compares to the recursive algorithm. My data is not perhaps as
rigorous as yours, but I did notice that the ORT algorithm was consistently
within the 5-6 second range while the recursive algorithm would vary within
the 7-20 second range (and some outliers near 30s).

Of course, I'm particularly excited about the benefits to the sparse index
work. I also have a prototype of a 'git merge' integration with sparse
index which was not very hard because the ORT strategy does not use the
index as a data structure. With that change, my tests dropped to between
0.5s and 1.5 seconds. (This shows just how much my earlier timings were
stuck on index reads and writes.) Now, the largest indicator of time is
how long it takes to resolve text conflicts.

Thanks! What a monumental effort.

It's certainly been a long haul.  And you jumped after all my
preparatory patches when I started submitting actual merge-ort
patches.  Since that time about a year ago, you have reviewed nearly
every single one of these patches, and there have been well over a
hundred of them and they've been far from trivial.  The code is better
today because of your comments and suggestions.  Thanks for the
monumental review effort.

One quick question on your timings here, though: in the past, when you
compared timings, you compared merge-recursive *without* rename
detection to merge-ort *with* rename detection.  Are you doing the
same here (just so others have a better sense of what is being
compared)?

Thanks again!
Elijah

On Mon, Aug 2, 2021 at 9:56 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Sun, Aug 01 2021, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>
[...]

@@ -3968,7 +3968,7 @@ static int do_merge(struct repository *r,
      o.branch2 = ref_name.buf;
      o.buffer_output = 2;

-     if (opts->strategy && !strcmp(opts->strategy, "ort")) {
+     if (!opts->strategy || strcmp(opts->strategy, "recursive")) {
              /*
               * TODO: Should use merge_incore_recursive() and
               * merge_switch_to_result(), skipping the call to

I might spot more tiny issues, but it looks like our error messaging
needs updating for 14c4586c2df (merge,rebase,revert: select ort or
recursive by config or environment, 2020-11-02).

I.e. we die on "Unknown option for merge-recursive", presumably that
should be updated to indicate that we might call one of
merge_recursive() or merge_ort_recursive() now.

Ooh, good catch.  I think I'd prefer to reword this to "Unknown
strategy option: -X%s"

And perhaps this in sequencer.c:

    that represents the "current" state for merge-recursive[...]

Yeah, it's just a comment but it should still be updated.  I'll
s/merge-recursive/the merge machinery/ for this one.

I tried to look for other error messages or comments similar to these
two but didn't find anything.  I might have missed something, though.

I'll get these fixed up with the next submission.

On 8/2/2021 12:27 PM, Elijah Newren wrote:

On Mon, Aug 2, 2021 at 9:05 AM Derrick Stolee [off-list ref] wrote:

quoted

I've done my own share of testing on some of our private monorepos to see
how ORT compares to the recursive algorithm. My data is not perhaps as
rigorous as yours, but I did notice that the ORT algorithm was consistently
within the 5-6 second range while the recursive algorithm would vary within
the 7-20 second range (and some outliers near 30s).

...

One quick question on your timings here, though: in the past, when you
compared timings, you compared merge-recursive *without* rename
detection to merge-ort *with* rename detection.  Are you doing the
same here (just so others have a better sense of what is being
compared)?

Yes, I continue to test with merge.renames=0 in the config. That makes
the data more impressive.

Thanks,
-Stolee

Hi Elijah,

On Sun, 1 Aug 2021, Elijah Newren via GitGitGadget wrote:

From: Elijah Newren <redacted>

There are a few reasons to switch the default:
[...]

I think it would be really fantastic to change to the new default right
after v2.33.0.

As to the patch, I only struggled slightly with the changes to
`sequencer.c`:

At this stage, we're in `do_recursive_merge()`, and there is only one
caller, `do_pick_commit()`, and the caller is guarded by the following
condition:

        else if (!opts->strategy ||
                 !strcmp(opts->strategy, "recursive") ||
                 !strcmp(opts->strategy, "ort") ||
                 command == TODO_REVERT) {

The issue I see is with `git revert` allowing custom merge strategies. I
_think_ we need a slightly different patch here, something like this:

-	if (opts->strategy && !strcmp(opts->strategy, "ort")) {
+	if (!opts->strategy || !strcmp(opts->strategy, "ort")) {

It took me a while to convince myself that this is correct. At least now I
_think_ it is correct: `do_merge()` defines:

        const char *strategy = !opts->xopts_nr &&
                (!opts->strategy ||
                 !strcmp(opts->strategy, "recursive") ||
                 !strcmp(opts->strategy, "ort")) ?
                NULL : opts->strategy;

and then hands off to `git merge -s <strategy>` if `strategy` is set,
_before_ this hunk. Therefore we can be pretty certain that
`opts->strategy` is either not set, or "ort", or "recursive" at that
stage.

However, I think we could use the same idea I outlined in the previous
hunk, to make things more obvious:

-	if (opts->strategy && !strcmp(opts->strategy, "ort")) {
+	if (!opts->strategy || !strcmp(opts->strategy, "ort")) {

Thank you,
Dscho

 		/*
 		 * TODO: Should use merge_incore_recursive() and
 		 * merge_switch_to_result(), skipping the call to
--
gitgitgadget

On Mon, Aug 2, 2021 at 4:46 PM Johannes Schindelin
[off-list ref] wrote:

Hi Elijah,

On Sun, 1 Aug 2021, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

There are a few reasons to switch the default:
[...]

I think it would be really fantastic to change to the new default right
after v2.33.0.

As to the patch, I only struggled slightly with the changes to
`sequencer.c`:

quoted

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..a98de9a8d15 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -636,7 +636,7 @@ static int do_recursive_merge(struct repository *r,
      for (i = 0; i < opts->xopts_nr; i++)
              parse_merge_opt(&o, opts->xopts[i]);

-     if (opts->strategy && !strcmp(opts->strategy, "ort")) {
+     if (!opts->strategy || strcmp(opts->strategy, "recursive")) {

At this stage, we're in `do_recursive_merge()`, and there is only one
caller, `do_pick_commit()`, and the caller is guarded by the following
condition:

        else if (!opts->strategy ||
                 !strcmp(opts->strategy, "recursive") ||
                 !strcmp(opts->strategy, "ort") ||
                 command == TODO_REVERT) {

The issue I see is with `git revert` allowing custom merge strategies. I
_think_ we need a slightly different patch here, something like this:

-       if (opts->strategy && !strcmp(opts->strategy, "ort")) {
+       if (!opts->strategy || !strcmp(opts->strategy, "ort")) {

quoted

              memset(&result, 0, sizeof(result));
              merge_incore_nonrecursive(&o, base_tree, head_tree, next_tree,
                                          &result);

@@ -3968,7 +3968,7 @@ static int do_merge(struct repository *r,
      o.branch2 = ref_name.buf;
      o.buffer_output = 2;

-     if (opts->strategy && !strcmp(opts->strategy, "ort")) {
+     if (!opts->strategy || strcmp(opts->strategy, "recursive")) {

It took me a while to convince myself that this is correct. At least now I
_think_ it is correct: `do_merge()` defines:

        const char *strategy = !opts->xopts_nr &&
                (!opts->strategy ||
                 !strcmp(opts->strategy, "recursive") ||
                 !strcmp(opts->strategy, "ort")) ?
                NULL : opts->strategy;

and then hands off to `git merge -s <strategy>` if `strategy` is set,
_before_ this hunk. Therefore we can be pretty certain that
`opts->strategy` is either not set, or "ort", or "recursive" at that
stage.

However, I think we could use the same idea I outlined in the previous
hunk, to make things more obvious:

-       if (opts->strategy && !strcmp(opts->strategy, "ort")) {
+       if (!opts->strategy || !strcmp(opts->strategy, "ort")) {

Thank you,
Dscho

quoted

              /*
               * TODO: Should use merge_incore_recursive() and
               * merge_switch_to_result(), skipping the call to
--
gitgitgadget

I'll include both suggestions in my next re-roll.  Thanks for the feedback!

Hi Elijah,

Le 2021-07-31 à 20:07, Elijah Newren via GitGitGadget a écrit :

From: Elijah Newren <redacted>


   * `git diff AUTO_MERGE` -- ability to see what changes the user has
     made to resolve conflicts so far (see commit 5291828df8 ("merge-ort:
     write $GIT_DIR/AUTO_MERGE whenever we hit a conflict", 2021-03-20)


The last three have been implemented already (though only one has been
submitted upstream so far; 

 From what I could find this indeed only refers to your 5291828df8 (merge-ort:
write $GIT_DIR/AUTO_MERGE whenever we hit a conflict, 2021-03-20).
This is a very nice improvement, but I noticed it is not mentioned in the doc.
Do you plan to update the 'git diff' doc to mention that special ref ?
(And maybe also gitrevisions(5), where most of the special refs are listed ?)

Do you plan to implement a new '--auto-merge' option to 'git diff' as a shortcut
to 'git diff AUTO_MERGE', in order to hide a bit the special ref from users ?

Thanks a lot for your work,

Philippe.

Hi Philippe,

On Mon, Aug 2, 2021 at 8:56 PM Philippe Blain
[off-list ref] wrote:

Hi Elijah,

Le 2021-07-31 à 20:07, Elijah Newren via GitGitGadget a écrit :

quoted

From: Elijah Newren <redacted>


   * `git diff AUTO_MERGE` -- ability to see what changes the user has
     made to resolve conflicts so far (see commit 5291828df8 ("merge-ort:
     write $GIT_DIR/AUTO_MERGE whenever we hit a conflict", 2021-03-20)


The last three have been implemented already (though only one has been
submitted upstream so far;

 From what I could find this indeed only refers to your 5291828df8 (merge-ort:
write $GIT_DIR/AUTO_MERGE whenever we hit a conflict, 2021-03-20).
This is a very nice improvement, but I noticed it is not mentioned in the doc.
Do you plan to update the 'git diff' doc to mention that special ref ?
(And maybe also gitrevisions(5), where most of the special refs are listed ?)

Do you plan to implement a new '--auto-merge' option to 'git diff' as a shortcut
to 'git diff AUTO_MERGE', in order to hide a bit the special ref from users ?

Fair point, it probably does deserve to be documented somewhere, at
least once ort is the default merge algorithm.

I don't think it'd make sense to include a reference to it in
gitrevisions(5), since $GIT_DIR/AUTO_MERGE is a reference to a tree
rather than to a revision.  But documenting that special ref in
Documentation/git-diff.txt, and perhaps linking to it from other
conflict-related options (e.g. --base, --ours, --theirs) may make
sense.  Your --auto-merge idea may also make sense, and it'd be
somewhat similar to how git-rebase has a --show-current-patch option
that is shorthand for `git show REBASE_HEAD` and is documented as
such.  However, it might be confusing to users how to combine
--auto-merge with paths, whereas `git diff AUTO_MERGE -- pathname` is
pretty clear once you know that AUTO_MERGE is a tree you can diff
against.  Hmmm....

On Sat, Jul 31, 2021 at 6:07 PM Elijah Newren via GitGitGadget
[off-list ref] wrote:

From: Elijah Newren <redacted>

Make multiple documentation updates to bring things up to date as we
change the default merge backend...

Some of these changes are specific to the change in default backend,
but many of them are probably useful updates prior to such a switch.
I'm going to break those out into several small patches and submit
them separately so they can be included in Git v2.33.0 if wanted.
I'll then take the remaining changes and resubmit them after 2.33.0
along with the switch of default.

On Sun, Aug 01, 2021 at 12:07:39AM +0000, Elijah Newren via GitGitGadget wrote:

This is an RFC series designed to spur feedback about switching the default
merge backend (reviewing the patches is of secondary importance at this
point). Some questions:

 * Are there things others want before this series is considered for
   inclusion?
 * What kind of timeline do others think is reasonable?
 * Would it be beneficial to let this series sit in 'next' for an extended
   duration to gain more feedback?

It looks like others gave some more specific review on the patches, but
on the meta-topic of "do we switch, and when", my response is: yes, and
soon. :)

Having watched the development of merge-ort, plus all of the weird
corner cases in merge-recursive we've seen over the years (many of which
you found and added tests for while working on merge-ort!), my gut
feeling is that the switch is _much_ more likely to fix problems people
might see in the wild rather than cause them.

It would make sense to me to do the switch in 'next' early in the
post-v2.33 cycle. It can cook there for a bit, but I think we have found
that it's much more likely to see actual use once it hits 'master'. So I
don't see a particular reason to have it sit in 'next' for a long time.
We should get as much exposure in 'master' during the v2.34 cycle as
possible.

The nice thing is that the two strategies can co-exist. So if it does
turn out to have any regressions, it's an easy revert to switch back,
and even post-release users can switch at runtime. We have pull.twohead,
but I don't think we have an equivalent that would impact a bare "git
merge" or "git rebase -m". Maybe it would be worth adding those as an
escape hatch?

-Peff

On Tue, Aug 3, 2021 at 9:56 AM Jeff King [off-list ref] wrote:

On Sun, Aug 01, 2021 at 12:07:39AM +0000, Elijah Newren via GitGitGadget wrote:

quoted

This is an RFC series designed to spur feedback about switching the default
merge backend (reviewing the patches is of secondary importance at this
point). Some questions:

 * Are there things others want before this series is considered for
   inclusion?
 * What kind of timeline do others think is reasonable?
 * Would it be beneficial to let this series sit in 'next' for an extended
   duration to gain more feedback?

It looks like others gave some more specific review on the patches, but
on the meta-topic of "do we switch, and when", my response is: yes, and
soon. :)

Having watched the development of merge-ort, plus all of the weird
corner cases in merge-recursive we've seen over the years (many of which
you found and added tests for while working on merge-ort!), my gut
feeling is that the switch is _much_ more likely to fix problems people
might see in the wild rather than cause them.

It would make sense to me to do the switch in 'next' early in the
post-v2.33 cycle. It can cook there for a bit, but I think we have found
that it's much more likely to see actual use once it hits 'master'. So I
don't see a particular reason to have it sit in 'next' for a long time.
We should get as much exposure in 'master' during the v2.34 cycle as
possible.

The nice thing is that the two strategies can co-exist. So if it does
turn out to have any regressions, it's an easy revert to switch back,
and even post-release users can switch at runtime. We have pull.twohead,
but I don't think we have an equivalent that would impact a bare "git
merge" or "git rebase -m". Maybe it would be worth adding those as an
escape hatch?

Actually, pull.twohead is not pull specific; it already affects merge,
rebase (-m is the default for rebase, btw), cherry-pick, and revert.
pull.twohead has affected a bare "git merge" since 1c7b76be7d ("Build
in merge", 2008-07-07).  I thought it was weird that "merge strategy"
for the merge command was specified via a config option under "pull",
and included my misgivings about it in the commit message of
14c4586c2d ("merge,rebase,revert: select ort or recursive by config or
environment", 2020-11-02) when I made sequencer.c pay attention to
that config option as well:

"""
    Also, allow folks to pick the new algorithm via config setting.  It
    turns out builtin/merge.c already had a way to allow users to specify a
    different default merge algorithm: pull.twohead.  Rather odd
    configuration name (especially to be in the 'pull' namespace rather than
    'merge') but it's there.  Add that same configuration to rebase,
    cherry-pick, and revert.
"""

But no one had an alternate suggestion or opinion on attempting to
migrate the configuration to a different name, so it has just stuck.
Anyway, if folks want to try out 'ort' with the 2.32 or 2.33 releases,
they can set pull.twohead=ort.  Once we switch the default, they can
set pull.twohead=recursive to get the old default.

On Tue, Aug 03, 2021 at 10:57:12AM -0600, Elijah Newren wrote:

quoted

The nice thing is that the two strategies can co-exist. So if it does
turn out to have any regressions, it's an easy revert to switch back,
and even post-release users can switch at runtime. We have pull.twohead,
but I don't think we have an equivalent that would impact a bare "git
merge" or "git rebase -m". Maybe it would be worth adding those as an
escape hatch?

Actually, pull.twohead is not pull specific; it already affects merge,
rebase (-m is the default for rebase, btw), cherry-pick, and revert.
pull.twohead has affected a bare "git merge" since 1c7b76be7d ("Build
in merge", 2008-07-07).  I thought it was weird that "merge strategy"
for the merge command was specified via a config option under "pull",
and included my misgivings about it in the commit message of
14c4586c2d ("merge,rebase,revert: select ort or recursive by config or
environment", 2020-11-02) when I made sequencer.c pay attention to
that config option as well:

"""
    Also, allow folks to pick the new algorithm via config setting.  It
    turns out builtin/merge.c already had a way to allow users to specify a
    different default merge algorithm: pull.twohead.  Rather odd
    configuration name (especially to be in the 'pull' namespace rather than
    'merge') but it's there.  Add that same configuration to rebase,
    cherry-pick, and revert.
"""

But no one had an alternate suggestion or opinion on attempting to
migrate the configuration to a different name, so it has just stuck.
Anyway, if folks want to try out 'ort' with the 2.32 or 2.33 releases,
they can set pull.twohead=ort.  Once we switch the default, they can
set pull.twohead=recursive to get the old default.

Ah, thanks for clarifying. I think we are in good shape, then. We could
possibly introduce merge.twohead as a synonym, but given that most
people would probably not ever even look at this, it may not be worth
worrying about.

-Peff