Re: [PATCH v2 0/2] add additional config settings for merge

From: Ben Peart <peartben@gmail.com>
To: Junio C Hamano <gitster@pobox.com>, Ben Peart <Ben.Peart@microsoft.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	"newren@gmail.com" <newren@gmail.com>,
	"peff@peff.net" <peff@peff.net>,
	"pclouds@gmail.com" <pclouds@gmail.com>,
	"vmiklos@frugalware.org" <vmiklos@frugalware.org>,
	Kevin Willford <kewillf@microsoft.com>,
	"Johannes.Schindelin@gmx.de" <Johannes.Schindelin@gmx.de>,
	"eckhard.s.maass@googlemail.com" <eckhard.s.maass@googlemail.com>
Subject: Re: [PATCH v2 0/2] add additional config settings for merge
Date: Wed, 25 Apr 2018 11:22:33 -0400	[thread overview]
Message-ID: <365838dc-d988-b72c-ef29-20369a7f54a2@gmail.com> (raw)
In-Reply-To: <xmqqd0yo5ejb.fsf@gitster-ct.c.googlers.com>

On 4/24/2018 8:13 PM, Junio C Hamano wrote:
> Ben Peart <Ben.Peart@microsoft.com> writes:
> 
>>   diff.renameLimit::
>>   	The number of files to consider when performing the copy/rename
>> -	detection; equivalent to the 'git diff' option `-l`.
>> +	detection; equivalent to the 'git diff' option `-l`. This setting
>> +	has no effect if rename detection is turned off.
> 
> You mean "turned off via diff.renames"?
> 
> This is not meant as a suggestion to rewrite this paragraph
> further---but if the answer is "no", then that might be an
> indication that the sentence is inviting a misunderstanding.
> 

Yes, this is referring to turned off via the config setting 
"diff.renames" but it could also be turned off by passing "--no-renames" 
on the command line.

To be clear, this documentation change isn't trying to document any 
changes to the code or behavior - it is just an attempt to clarify what 
the existing behavior is.  If it isn't helping, I can remove it.

>>   diff.renames::
>>   	Whether and how Git detects renames.  If set to "false",
>> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
>> index 5a9ab969db..38492bcb98 100644
>> --- a/Documentation/merge-config.txt
>> +++ b/Documentation/merge-config.txt
>> @@ -39,7 +39,8 @@ include::fmt-merge-msg-config.txt[]
>>   merge.renameLimit::
>>   	The number of files to consider when performing rename detection
>>   	during a merge; if not specified, defaults to the value of
>> -	diff.renameLimit.
>> +	diff.renameLimit. This setting has no effect if rename detection
>> +	is turned off.
> 
> Ditto.  If your design is to make the merge machinery completely
> ignore diff.renames and only pay attention to merge.renames [*1*],
> then it probably is a good idea to be more specific here, by saying
> "... is turned off via ...", though.
> 
>>   merge.renames::
>>   	Whether and how Git detects renames.  If set to "false",
> 
> [Footnote]
> 
> *1* ...which I do not think is such a good idea, by the way.  I'd
> personally expect merge.renames to allow overriding and falling back
> to diff.renames, just like the {merge,diff}.renameLimit pair does.
> 

It looks like I'm in the minority on whether the merge settings should 
inherit from the corresponding diff settings so I will submit a new 
patch series that does it the same way as the {merge,diff}.renameLimit 
pair works.

I'll leave it as an exercise for someone else [1] to change any other 
merge settings that should behave that way.

[1] 
https://public-inbox.org/git/20180420133632.17580-1-benpeart@microsoft.com/T/#m52a3dbd0945360bfb873fd3b553472558ef3b796