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

From: Ben Peart <peartben@gmail.com>
Date: Apr 23, 2018, 16:00 UTC
Message-ID: <1fb11850-4c20-5327-a63a-6d1f5aa18ea4@gmail.com>
In-Reply-To: <xmqqwox19ohw.fsf@gitster-ct.c.googlers.com>

On 4/21/2018 12:23 AM, Junio C Hamano wrote:

Show 21 quoted lines

> Elijah Newren <newren@gmail.com> writes:
> 
>>>>> +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.
>> ...
>> 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.
> 
> Yes, and if we are adding a new configuration, we should do so in
> such a way that we do not have to come back and extend it when we
> know what the command line option does and the configuration being
> proposed is less capable already.

Between all the different command line options, config settings, merge strategies and the interactions between the diff and merge versions, I was trying to keep things as simple and consistent as possible. To that end 'merge.renames' was modeled after the existing 'diff.renames.'

I wonder if we can just add a

Show 8 quoted lines

> single configuration whose value can be "never" to pretend as if
> "--Xno-renames" were given, and some similarity score like "50" to
> pretend as if "--Xfind-renames=50" were given.
> 
> That is, merge.renames does not have to a simple "yes-no to control
> the --Xno-renames option".  And it would of course be better to
> document it.
>

With the existing differences in how these options are passed on the command line, I'm hesitant to add yet another pattern in the config settings that combines 'renames' and '--find-renames[=<n>]'.

I _have_ wondered why this all isn't configured via find-renames with find-renames=0 meaning renames=false (instead of mapping 0 to 32K). I think that could have eliminated the need for splitting rename across two different settings (which is what I think you are proposing above). I'd then want the config setting and command line option to be the same syntax and behavior.

Moving the existing settings to this model and updating the config and command line options to be consistent without breaking backwards compatibility is outside the intended scope of this patch.

> I also had to wonder how "merge -s resolve" faired, if the project
> is not interested in renamed paths at all.
>

To be clear, it isn't that we're not interested in detecting renamed files and paths. We're just opposed to it taking an hour to figure that out!

> Thanks.
>