Re: [PATCH 2/3] merge-ort: allow rename detection to be disabled

[Patrick Steinhardt <ps@pks.im>]

On Fri, Mar 07, 2025 at 03:48:41PM +0000, Elijah Newren via GitGitGadget wrote:
> 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.

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

> 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