Re: [PATCH v1 1/2] merge: Add merge.renames config setting

From: Elijah Newren <newren@gmail.com>
Date: Apr 20, 2018, 18:34 UTC
Message-ID: <CABPp-BFqj2TFiHUDsysafq0NHC4MV-QYZVxOZe1TNRrXMOQfng@mail.gmail.com>
In-Reply-To: <cd49481c-9665-124a-5f94-791f1a16657d@gmail.com>

Hi Ben,

On Fri, Apr 20, 2018 at 10:59 AM, Ben Peart <peartben@gmail.com> wrote:

Show 20 quoted lines

>
> On 4/20/2018 1:02 PM, Elijah Newren wrote:
>>
>> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart <Ben.Peart@microsoft.com>
>> wrote:
>>>
>>> --- a/Documentation/merge-config.txt
>>> +++ b/Documentation/merge-config.txt
>>> @@ -37,6 +37,11 @@ merge.renameLimit::
>>>          during a merge; if not specified, defaults to the value of
>>>          diff.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. This is the default.
>>
>>
>> One can already control o->detect_rename via the -Xno-renames and
>> -Xfind-renames options.

This statement wasn't meant to be independent of the sentence that followed it...

Show 8 quoted lines

> Yes, but that requires people to know they need to do that and then remember
> to pass it on the command line every time.  We've found that doesn't
> typically happen, we just get someone complaining about slow merges. :)
>
> That is why we added them as config options which change the default. That
> way we can then set them on the repo and the default behavior gives them
> better performance.  They can still always override the config setting with
> the command line options.

Sorry, I think I wasn't being clear. The documentation for the config options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and merge.ff all mention the equivalent command line parameters. Your patch doesn't do that for merge.renames, but I think it would be helpful if it did.

Also, a link in the documentation the other way, from Documentation/merge-strategies.txt under the entries for -Xno-renames and -Xfind-renames should probably mention this new merge.renames config setting (much like the -Xno-renormalize flag mentions the merge.renomralize config option).

(In general, I think having this as a configuration option makes sense, though I hope my other performance patches would be enough to make people consider switching back to the defaults and use rename detection again.)

<snip>

> I'm of the opinion that we shouldn't bother adding features that we aren't
> sure someone will want/use.  If it comes up, we can certainly add it at a
> later date.

Works for me; I was mostly throwing it out there for thought.

> Yes, command line options override the config settings.

Good.  :-)

Show 8 quoted lines

>> Also, if someone sets merge.renameLimit (to anything) and sets
>> merge.renames to false, then they've got a contradictory setup.  Does
>> it make sense to check and warn about that anywhere?
>
> I don't think we need to.  The merge.renameLimit is only used if
> detect_rename it turned on no matter how that gets turned on (default,
> config setting, command line option) so there isn't really a change in
> behavior here.

I agree that's the pre-existing behavior, but prior to this patch turning off rename detection could only be done manually with every invocation. I'm slightly concerned that users might be confused if merge.renames was set to false somewhere -- perhaps even in a global /etc/gitconfig that they had no knowledge of or control over -- and in an attempt to get rename detection to work they started passing larger and larger values for renameLimit all to no avail.

The easy fix here may just be documenting the diff.renameLimit and merge.renameLimit options that they have no effect if rename detection is turned off.

Or maybe I'm just worrying too much, but we (folks at $dayjob) were bit pretty hard by renameLimit silently being capped at a value less than the user specified and in a way that wasn't documented anywhere.