Re: [PATCH 2/3] merge-ort: allow rename detection to be disabled
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 12, 2025, 08:06 UTC
- Message-ID
- <Z9FAix-VKGte8UKk@pks.im>
- In-Reply-To
- <4292b22723f759c3e0f84ac1000992187a9c7f7c.1741362522.git.gitgitgadget@gmail.com>
On Fri, Mar 07, 2025 at 03:48:41PM +0000, Elijah Newren via GitGitGadget wrote:
Show 15 quoted lines
> From: Elijah Newren <newren@gmail.com> > > When merge-ort was written, I did not at first allow rename detection to > be disabled, because I suspected that most folks disabling rename > detection were doing so solely for performance reasons. Since I put a > lot of working into providing dramatic speedups for rename detection > performance as used by the merge machinery, I wanted to know if there > were still real world repositories where rename detection was > problematic from a performance perspective. We have had years now to > collect such information, and while we never received one, waiting > longer with the option disabled seems unlikely to help surface such > issues at this point. Also, there has been at least one request to > allow rename detection to be disabled for behavioral rather than > performance reasons, so let's start heeding the config and command line > settings.
It might be nice to provide a link to that request for more context.
Show 37 quoted lines
> diff --git a/Documentation/merge-strategies.adoc b/Documentation/merge-strategies.adoc > index 93822ebc4e8..59f5ae36ccb 100644 > --- a/Documentation/merge-strategies.adoc > +++ b/Documentation/merge-strategies.adoc > @@ -82,6 +82,11 @@ find-renames[=<n>];; > rename-threshold=<n>;; > Deprecated synonym for `find-renames=<n>`. > > +no-renames;; > + Turn off rename detection. This overrides the `merge.renames` > + configuration variable. > + See also linkgit:git-diff[1] `--no-renames`. > + > subtree[=<path>];; > This option is a more advanced form of 'subtree' strategy, where > the strategy makes a guess on how two trees must be shifted to > @@ -107,7 +112,7 @@ For a path that is a submodule, the same caution as 'ort' applies to this > strategy. > + > The 'recursive' strategy takes the same options as 'ort'. However, > -there are three additional options that 'ort' ignores (not documented > +there are two additional options that 'ort' ignores (not documented > above) that are potentially useful with the 'recursive' strategy: > > patience;; > @@ -121,11 +126,6 @@ diff-algorithm=[patience|minimal|histogram|myers];; > 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 > and another branch you pulled from) using a 3-way merge
Makes sense.
Show 16 quoted lines
> diff --git a/merge-ort.c b/merge-ort.c
> index b4ff24403a1..a6960b6a1b4 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3448,6 +3448,11 @@ static int detect_and_process_renames(struct merge_options *opt)
>
> if (!possible_renames(renames))
> goto cleanup;
> + if (opt->detect_renames == 0) {
> + renames->redo_after_renames = 0;
> + renames->cached_pairs_valid_side = 0;
> + goto cleanup;
> + }
>
> trace2_region_enter("merge", "regular renames", opt->repo);
> detection_run |= detect_regular_renames(opt, MERGE_SIDE1);Do we want to add a test that demonstrates that the option works as expected?
Patrick