git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-01  0:07 [PATCH 0/2] [RFC] " Elijah Newren via GitGitGadget
@ 2021-08-01  0:07 ` Elijah Newren via GitGitGadget
  2021-08-02 15:55   ` Ævar Arnfjörð Bjarmason
                     ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-08-01  0:07 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason, Elijah Newren,
	Elijah Newren

From: Elijah Newren <newren@gmail.com>

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 <newren@gmail.com>
---
 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


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

* Re: [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-01  0:07 ` [PATCH 1/2] Change " Elijah Newren via GitGitGadget
@ 2021-08-02 15:55   ` Ævar Arnfjörð Bjarmason
  2021-08-02 16:33     ` Elijah Newren
  2021-08-02 22:46   ` Johannes Schindelin
  2021-08-03  2:56   ` Philippe Blain
  2 siblings, 1 reply; 14+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-08-02 15:55 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Derrick Stolee, Emily Shaffer,
	Eric Sunshine, Jeff King, Johannes Schindelin, Jonathan Nieder,
	Jonathan Tan, Junio C Hamano, Phillip Wood, René Scharfe,
	Taylor Blau, Elijah Newren


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

> From: Elijah Newren <newren@gmail.com>
> [...]
> @@ -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.

And perhaps this in sequencer.c:

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

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

* Re: [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-02 15:55   ` Ævar Arnfjörð Bjarmason
@ 2021-08-02 16:33     ` Elijah Newren
  0 siblings, 0 replies; 14+ messages in thread
From: Elijah Newren @ 2021-08-02 16:33 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau

On Mon, Aug 2, 2021 at 9:56 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Sun, Aug 01 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> > [...]
> > @@ -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.

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

* Re: [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-01  0:07 ` [PATCH 1/2] Change " Elijah Newren via GitGitGadget
  2021-08-02 15:55   ` Ævar Arnfjörð Bjarmason
@ 2021-08-02 22:46   ` Johannes Schindelin
  2021-08-03  1:04     ` Elijah Newren
  2021-08-03  2:56   ` Philippe Blain
  2 siblings, 1 reply; 14+ messages in thread
From: Johannes Schindelin @ 2021-08-02 22:46 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, Christian Couder, Derrick Stolee, Emily Shaffer,
	Eric Sunshine, Jeff King, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason, Elijah Newren,
	Elijah Newren

Hi Elijah,

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

> From: Elijah Newren <newren@gmail.com>
>
> 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`:

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

>  		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

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

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

* Re: [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-02 22:46   ` Johannes Schindelin
@ 2021-08-03  1:04     ` Elijah Newren
  0 siblings, 0 replies; 14+ messages in thread
From: Elijah Newren @ 2021-08-03  1:04 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Jonathan Nieder, Jonathan Tan, Junio C Hamano,
	Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason

On Mon, Aug 2, 2021 at 4:46 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Sun, 1 Aug 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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`:
>
> > 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")) {
>
> >               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
>
> >               /*
> >                * 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!

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

* Re: [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-01  0:07 ` [PATCH 1/2] Change " Elijah Newren via GitGitGadget
  2021-08-02 15:55   ` Ævar Arnfjörð Bjarmason
  2021-08-02 22:46   ` Johannes Schindelin
@ 2021-08-03  2:56   ` Philippe Blain
  2021-08-03  3:39     ` Elijah Newren
  2 siblings, 1 reply; 14+ messages in thread
From: Philippe Blain @ 2021-08-03  2:56 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget, git
  Cc: Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason, Elijah Newren

Hi Elijah,

Le 2021-07-31 à 20:07, Elijah Newren via GitGitGadget a écrit :
> From: Elijah Newren <newren@gmail.com>
> 
> 
>    * `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.

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

* Re: [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-03  2:56   ` Philippe Blain
@ 2021-08-03  3:39     ` Elijah Newren
  0 siblings, 0 replies; 14+ messages in thread
From: Elijah Newren @ 2021-08-03  3:39 UTC (permalink / raw)
  To: Philippe Blain
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason

Hi Philippe,

On Mon, Aug 2, 2021 at 8:56 PM Philippe Blain
<levraiphilippeblain@gmail.com> wrote:
>
> Hi Elijah,
>
> Le 2021-07-31 à 20:07, Elijah Newren via GitGitGadget a écrit :
> > From: Elijah Newren <newren@gmail.com>
> >
> >
> >    * `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....

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

* [PATCH 0/2] Switch default merge backend from recursive to ort
@ 2021-08-04  5:38 Elijah Newren via GitGitGadget
  2021-08-04  5:38 ` [PATCH 1/2] Change " Elijah Newren via GitGitGadget
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-08-04  5:38 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason, Elijah Newren

This is a reroll of my RFC series[1] to switch the default merge backend
from recursive to ort. The consensus seems to be that we should do this
immediately after the 2.33 release.

Note that folks who want to get the old merge backend after this series can
simply set pull.twohead=recursive. (And, similarly, before this series,
those who want to try out ort in Git 2.32 or Git 2.33 can set
pull.twohead=ort.)

Changes since the RFC version of this series:

 * Now depends on the reroll of en/merge-strategy-docs I just submitted[2].
 * Made tweaks to code and documentation suggested by Stolee, Dscho, and
   Ævar

[1]
https://lore.kernel.org/git/pull.1055.git.git.1627776461.gitgitgadget@gmail.com/
[2]
https://lore.kernel.org/git/pull.1059.git.git.1628004920.gitgitgadget@gmail.com/

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

 Documentation/git-rebase.txt       | 28 ++++-----
 Documentation/gitfaq.txt           |  2 +-
 Documentation/merge-options.txt    |  2 +-
 Documentation/merge-strategies.txt | 93 ++++++++++++++++--------------
 Documentation/user-manual.txt      |  2 +-
 builtin/merge.c                    | 10 +++-
 builtin/rebase.c                   |  2 +-
 sequencer.c                        |  4 +-
 8 files changed, 78 insertions(+), 65 deletions(-)


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

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

* [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-04  5:38 [PATCH 0/2] Switch default merge backend from recursive to ort Elijah Newren via GitGitGadget
@ 2021-08-04  5:38 ` Elijah Newren via GitGitGadget
  2021-08-09 17:38   ` Phillip Wood
  2021-08-04  5:38 ` [PATCH 2/2] Update docs for change of default merge backend Elijah Newren via GitGitGadget
  2021-08-04 14:27 ` [PATCH 0/2] Switch default merge backend from recursive to ort Derrick Stolee
  2 siblings, 1 reply; 14+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-08-04  5:38 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason, Elijah Newren,
	Elijah Newren

From: Elijah Newren <newren@gmail.com>

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 <newren@gmail.com>
---
 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 d7b14bf4a7f..fbe21d413c1 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 a4e5c43fcf2..d08ddae52fa 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, "ort")) {
 		memset(&result, 0, sizeof(result));
 		merge_incore_nonrecursive(&o, base_tree, head_tree, next_tree,
 					    &result);
@@ -3988,7 +3988,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, "ort")) {
 		/*
 		 * TODO: Should use merge_incore_recursive() and
 		 * merge_switch_to_result(), skipping the call to
-- 
gitgitgadget


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

* [PATCH 2/2] Update docs for change of default merge backend
  2021-08-04  5:38 [PATCH 0/2] Switch default merge backend from recursive to ort Elijah Newren via GitGitGadget
  2021-08-04  5:38 ` [PATCH 1/2] Change " Elijah Newren via GitGitGadget
@ 2021-08-04  5:38 ` Elijah Newren via GitGitGadget
  2021-08-04 14:26   ` Derrick Stolee
  2021-08-04 14:27 ` [PATCH 0/2] Switch default merge backend from recursive to ort Derrick Stolee
  2 siblings, 1 reply; 14+ messages in thread
From: Elijah Newren via GitGitGadget @ 2021-08-04  5:38 UTC (permalink / raw)
  To: git
  Cc: Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason, Elijah Newren,
	Elijah Newren

From: Elijah Newren <newren@gmail.com>

Make it clear that `ort` is the default merge strategy now rather than
`recursive`, including moving `ort` to the front of the list of merge
strategies.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt       | 28 ++++-----
 Documentation/gitfaq.txt           |  2 +-
 Documentation/merge-options.txt    |  2 +-
 Documentation/merge-strategies.txt | 93 ++++++++++++++++--------------
 Documentation/user-manual.txt      |  2 +-
 5 files changed, 67 insertions(+), 60 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 73d49ec8d91..3f1030df70e 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -352,8 +352,8 @@ See also INCOMPATIBLE OPTIONS below.
 
 -s <strategy>::
 --strategy=<strategy>::
-	Use the given merge strategy, instead of the default
-	`recursive`.  This implies `--merge`.
+	Use the given merge strategy, instead of the default `ort`.
+	This implies `--merge`.
 +
 Because 'git rebase' replays each commit from the working branch
 on top of the <upstream> branch using the given strategy, using
@@ -366,7 +366,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.
@@ -527,7 +527,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.
@@ -1216,16 +1216,16 @@ 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.
 
-By default, the `merge` command will use the `recursive` merge
-strategy for regular merges, and `octopus` for octopus merges.  One
-can specify a default strategy for all merges using the `--strategy`
-argument when invoking rebase, or can override specific merges in the
-interactive list of commands by using an `exec` command to call `git
-merge` explicitly with a `--strategy` argument.  Note that when
-calling `git merge` explicitly like this, you can make use of the fact
-that the labels are worktree-local refs (the ref `refs/rewritten/onto`
-would correspond to the label `onto`, for example) in order to refer
-to the branches you want to merge.
+By default, the `merge` command will use the `ort` merge strategy for
+regular merges, and `octopus` for octopus merges.  One can specify a
+default strategy for all merges using the `--strategy` argument when
+invoking rebase, or can override specific merges in the interactive
+list of commands by using an `exec` command to call `git merge`
+explicitly with a `--strategy` argument.  Note that when calling `git
+merge` explicitly like this, you can make use of the fact that the
+labels are worktree-local refs (the ref `refs/rewritten/onto` would
+correspond to the label `onto`, for example) in order to refer to the
+branches you want to merge.
 
 Note: the first command (`label onto`) labels the revision onto which
 the commits are rebased; The name `onto` is just a convention, as a nod
diff --git a/Documentation/gitfaq.txt b/Documentation/gitfaq.txt
index afdaeab8503..8c1f2d56751 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 f819bd8dd68..72b53188504 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -112,7 +112,7 @@ 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 (`recursive` when merging a single head,
+	is used instead (`ort` when merging a single head,
 	`octopus` otherwise).
 
 -X <option>::
diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 210f0f850b2..34c6c31d8e7 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -6,21 +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`.
 
-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.  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
@@ -36,16 +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;;
-	Deprecated synonym 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`.  Defaults to the
-	`diff.algorithm` config setting.
-
 ignore-space-change;;
 ignore-all-space;;
 ignore-space-at-eol;;
@@ -74,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
@@ -95,19 +82,39 @@ subtree[=<path>];;
 	is prefixed (or stripped from the beginning) to make the shape of
 	two trees to match.
 
-ort::
-	This is meant as a drop-in replacement for the `recursive`
-	algorithm (as reflected in its acronym -- "Ostensibly
-	Recursive's Twin"), and will likely replace it in the future.
-	It fixes corner cases that the `recursive` strategy handles
-	suboptimally, and is significantly faster in large
-	repositories -- especially when many renames are involved.
+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.  This was
+	the default strategy for resolving two heads from Git v0.99.9k
+	until v2.33.0.
 +
-The `ort` strategy takes all the same options as `recursive`.
-However, it ignores three of those options: `no-renames`,
-`patience` and `diff-algorithm`.  It always runs with rename
-detection (it handles it much faster than `recursive` does), and
-it specifically uses `diff-algorithm=histogram`.
+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 synonym 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. 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
@@ -131,13 +138,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/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

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

* Re: [PATCH 2/2] Update docs for change of default merge backend
  2021-08-04  5:38 ` [PATCH 2/2] Update docs for change of default merge backend Elijah Newren via GitGitGadget
@ 2021-08-04 14:26   ` Derrick Stolee
  0 siblings, 0 replies; 14+ messages in thread
From: Derrick Stolee @ 2021-08-04 14:26 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget, git
  Cc: Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason, Elijah Newren

On 8/4/2021 1:38 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>

These updates look good to me. Mostly whole paragraphs moving and some
word replacements.

>  subtree::
> -	This is a modified recursive strategy. When merging trees A and
> +	This is a modified ort strategy. When merging trees A and

Here is one more case where the old docs could have used quotes around
"recursive" and the new ones could use quotes around "ort". Not worth
a re-roll on its own.

Thanks,
-Stolee

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

* Re: [PATCH 0/2] Switch default merge backend from recursive to ort
  2021-08-04  5:38 [PATCH 0/2] Switch default merge backend from recursive to ort Elijah Newren via GitGitGadget
  2021-08-04  5:38 ` [PATCH 1/2] Change " Elijah Newren via GitGitGadget
  2021-08-04  5:38 ` [PATCH 2/2] Update docs for change of default merge backend Elijah Newren via GitGitGadget
@ 2021-08-04 14:27 ` Derrick Stolee
  2 siblings, 0 replies; 14+ messages in thread
From: Derrick Stolee @ 2021-08-04 14:27 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget, git
  Cc: Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason, Elijah Newren

On 8/4/2021 1:38 AM, Elijah Newren via GitGitGadget wrote:
> This is a reroll of my RFC series[1] to switch the default merge backend
> from recursive to ort. The consensus seems to be that we should do this
> immediately after the 2.33 release.
> 
> Note that folks who want to get the old merge backend after this series can
> simply set pull.twohead=recursive. (And, similarly, before this series,
> those who want to try out ort in Git 2.32 or Git 2.33 can set
> pull.twohead=ort.)
> 
> Changes since the RFC version of this series:
> 
>  * Now depends on the reroll of en/merge-strategy-docs I just submitted[2].
>  * Made tweaks to code and documentation suggested by Stolee, Dscho, and
>    Ævar

I'm happy with this version (modulo a super tiny nitpick that doesn't merit
a re-roll) and the docs that preceded it. I'm going to adapt my version for
the microsoft/git fork to take this version and put it in our v2.33.0
release.

Thanks,
-Stolee

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

* Re: [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-04  5:38 ` [PATCH 1/2] Change " Elijah Newren via GitGitGadget
@ 2021-08-09 17:38   ` Phillip Wood
  2021-08-09 21:01     ` Elijah Newren
  0 siblings, 1 reply; 14+ messages in thread
From: Phillip Wood @ 2021-08-09 17:38 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget, git
  Cc: Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, Phillip Wood, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason, Elijah Newren

Hi Elijah

On 04/08/2021 06:38, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> 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

I've not followed your patches too closely but I did notice you seem to 
have put a lot of effort into improving the test coverage which is 
fantastic.

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

Rebasing without updating the worktree for each commit is exciting.

I agree with others that this should be merged into next sooner rather 
than later. I also agree with Peff's point about moving it into master 
to get more people using it rather than sitting in next for too long.

I think the sequencer changes below are easier to follow in this 
version. One thing I did wonder is whether there needs to be any change 
to the CI scripts to ensure we keep testing both merge implementations.

Best wishes and congratulations on an impressive achievement

Phillip

>    * `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 <newren@gmail.com>
> ---
>   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 d7b14bf4a7f..fbe21d413c1 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 a4e5c43fcf2..d08ddae52fa 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, "ort")) {
>   		memset(&result, 0, sizeof(result));
>   		merge_incore_nonrecursive(&o, base_tree, head_tree, next_tree,
>   					    &result);
> @@ -3988,7 +3988,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, "ort")) {
>   		/*
>   		 * TODO: Should use merge_incore_recursive() and
>   		 * merge_switch_to_result(), skipping the call to
> 


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

* Re: [PATCH 1/2] Change default merge backend from recursive to ort
  2021-08-09 17:38   ` Phillip Wood
@ 2021-08-09 21:01     ` Elijah Newren
  0 siblings, 0 replies; 14+ messages in thread
From: Elijah Newren @ 2021-08-09 21:01 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Elijah Newren via GitGitGadget, Git Mailing List,
	Christian Couder, Derrick Stolee, Emily Shaffer, Eric Sunshine,
	Jeff King, Johannes Schindelin, Jonathan Nieder, Jonathan Tan,
	Junio C Hamano, René Scharfe, Taylor Blau,
	Ævar Arnfjörð Bjarmason

On Mon, Aug 9, 2021 at 10:38 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
...
> > === 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.
>
> Rebasing without updating the worktree for each commit is exciting.

Yeah, there appear to be some interesting twists from my initial
investigations so far.  I may need to start some interesting
discussions in the next cycle or two about what we should do here (and
also for merges in bare repositories.)

> I agree with others that this should be merged into next sooner rather
> than later. I also agree with Peff's point about moving it into master
> to get more people using it rather than sitting in next for too long.

:-)

> I think the sequencer changes below are easier to follow in this
> version.

Thanks for taking a look.

> One thing I did wonder is whether there needs to be any change
> to the CI scripts to ensure we keep testing both merge implementations.

There did need to be such a change, but it was made previously in
commit f3b964a07e ("Add testing with merge-ort merge strategy",
2021-03-20).  That commit changed the default merge backend *for
testing* to be merge-ort, and modified the linux-gcc job to explicitly
specify recursive.  This commit doesn't change the testing story, so
the recursive backend will still continue to be tested with the
linux-gcc tests, or whenever someone requests it with
GIT_TEST_MERGE_ALGORITHM=recursive, and otherwise merge-ort will be
used.

> Best wishes and congratulations on an impressive achievement

Thanks!

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

end of thread, other threads:[~2021-08-09 21:01 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-04  5:38 [PATCH 0/2] Switch default merge backend from recursive to ort Elijah Newren via GitGitGadget
2021-08-04  5:38 ` [PATCH 1/2] Change " Elijah Newren via GitGitGadget
2021-08-09 17:38   ` Phillip Wood
2021-08-09 21:01     ` Elijah Newren
2021-08-04  5:38 ` [PATCH 2/2] Update docs for change of default merge backend Elijah Newren via GitGitGadget
2021-08-04 14:26   ` Derrick Stolee
2021-08-04 14:27 ` [PATCH 0/2] Switch default merge backend from recursive to ort Derrick Stolee
  -- strict thread matches above, loose matches on Subject: below --
2021-08-01  0:07 [PATCH 0/2] [RFC] " Elijah Newren via GitGitGadget
2021-08-01  0:07 ` [PATCH 1/2] Change " Elijah Newren via GitGitGadget
2021-08-02 15:55   ` Ævar Arnfjörð Bjarmason
2021-08-02 16:33     ` Elijah Newren
2021-08-02 22:46   ` Johannes Schindelin
2021-08-03  1:04     ` Elijah Newren
2021-08-03  2:56   ` Philippe Blain
2021-08-03  3:39     ` Elijah Newren

Code repositories for project(s) associated with this public 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).