Re:

[Ben Peart <peartben@gmail.com>]

On 4/27/2018 2:19 PM, Elijah Newren wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart <peartben@gmail.com> wrote:
> 
>> Can you write the documentation that clearly explains the exact behavior you
>> want?  That would kill two birds with one stone... :)
> 
> Sure, something like the following is what I envision, and I've tried to
> include the suggestion from Junio to document the copy behavior in the
> merge-recursive documentation.
> 
> -- 8< --
> Subject: [PATCH] fixup! merge: Add merge.renames config setting
> 
> ---
>   Documentation/merge-config.txt     | 3 +--
>   Documentation/merge-strategies.txt | 5 +++--
>   merge-recursive.c                  | 8 ++++++++
>   3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
> index 59848e5634..662c2713ca 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -41,8 +41,7 @@ merge.renameLimit::
>   merge.renames::
>   	Whether and how Git detects renames.  If set to "false",
>   	rename detection is disabled. If set to "true", basic rename
> -	detection is enabled.  If set to "copies" or "copy", Git will
> -	detect copies, as well.  Defaults to the value of diff.renames.
> +	detection is enabled.  Defaults to the value of diff.renames.
>   
>   merge.renormalize::
>   	Tell Git that canonical representation of files in the
> diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
> index 1e0728aa12..aa66cbe41e 100644
> --- a/Documentation/merge-strategies.txt
> +++ b/Documentation/merge-strategies.txt
> @@ -23,8 +23,9 @@ recursive::
>   	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.  This is the default merge strategy when
> -	pulling or merging one branch.
> +	renames, but currently cannot make use of detected
> +	copies.  This is the default merge strategy when pulling
> +	or merging one branch.
>   +
>   The 'recursive' strategy can take the following options:
>   
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 6cc4404144..b618f134d2 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -564,6 +564,14 @@ static struct string_list *get_renames(struct merge_options *o,
>   	opts.flags.recursive = 1;
>   	opts.flags.rename_empty = 0;
>   	opts.detect_rename = merge_detect_rename(o);
> +	/*
> +	 * We do not have logic to handle the detection of copies.  In
> +	 * fact, it may not even make sense to add such logic: would we
> +	 * really want a change to a base file to be propagated through
> +	 * multiple other files by a merge?
> +	 */
> +	if (opts.detect_rename > DIFF_DETECT_RENAME)
> +		opts.detect_rename = DIFF_DETECT_RENAME;
>   	opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
>   			    o->diff_rename_limit >= 0 ? o->diff_rename_limit :
>   			    1000;
> 

Thanks Elijah. I've applied this patch and reviewed and tested it.  It 
works and addresses the concerns around the settings inheritance from 
diff.renames.  I still _prefer_ the simpler model that doesn't do the 
partial inheritance but I can use this model as well.

I'm unsure on the protocol here.  Should I incorporate this patch and 
submit a reroll or can it just be applied as is?