Re: [PATCH v2] add status config and command line options for rename detection

[Ben Peart <peartben@gmail.com>]

On 5/10/2018 6:31 PM, Elijah Newren wrote:
> Hi Ben,
> 
> On Thu, May 10, 2018 at 12:09 PM, Ben Peart <peartben@gmail.com> wrote:
>> On 5/10/2018 12:19 PM, Elijah Newren wrote:
>>> On Thu, May 10, 2018 at 7:16 AM, Ben Peart <Ben.Peart@microsoft.com>
>>> wrote:
> 
>> Given the example perf impact is arbitrary (the actual example that
>> triggered this patch took status from 2+ hours to seconds) and can't be
>> replicated using the current performance tools in git, I'm just going drop
>> the specific numbers.  I believe the patch is worth while just to give users
>> the flexibility to control these behaviors.
> 
> Your parenthetical statement of timing going from hours to seconds I
> think would be great; I don't think we need precise numbers.
> 
>>>> +       if ((intptr_t)rename_score_arg != -1) {
>>>> +               s.detect_rename = DIFF_DETECT_RENAME;
>>>
>>>
>>> I'd still prefer this was a
>>>           if (s.detect_rename < DIFF_DETECT_RENAME)
>>>                   s.detect_rename = DIFF_DETECT_RENAME;
>>>
>>> If a user specifies they are willing to pay for copy detection, but
>>> then just passes --find-renames=40% because they want to find more
>>> renames, it seems odd to disable copy detection to me.
>>>
>>
>> I agree and will change it. It is unfortunate this will behave differently
>> than it does with merge.  Fixing the merge behavior to match is outside the
>> scope of this patch.
> 
> I agree that changing merge is outside the scope of this patch, but
> I'm curious what change you have in mind for it to "make it match".
> In particular, merge-recursive.c already has (or will shortly have)
> +       if (opts.detect_rename > DIFF_DETECT_RENAME)
> +               opts.detect_rename = DIFF_DETECT_RENAME;
> from your commit 85b460305ce7 ("merge: add merge.renames config
> setting", 2018-05-02), 

This is a good point that I missed.  With that recent change to merge, 
it no longer matters that the settings parsing code caps detect_rename 
at DIFF_DETECT_RENAME because it will cap it later anyway so there is no 
need to change the merge option behavior.

> The one place copy detection does make sense inside a merge is for the
> diffstat shown at the end (from builtin/merge.c), but it currently
> isn't controlled by any configuration setting at all.  When it is
> hooked up, it'd probably store the value separately from
> merge-recursive's internal o->{diff,merge}_detect_rename anyway,
> because builtin/merge.c's diffstat should be controlled by the
> relevant confiig settings and flags (merge.renames, diff.renames,
> -Xfind-renames, etc.) regardless of which merge strategy (recursive,
> resolve, octopus, ours, ort) is employed.  And when that is hooked up,
> I agree with you that it should look like what you've done with
> status.renames here.  In fact, if you'd like to take a crack at it, I
> think you'd do a great job.  :-)  If not, it's on my list of things to
> do.
> 

Thanks but I'll leave that to you. :)  I have a large backlog of patches 
I would like to see pushed through the mailing list into master.  We've 
been sitting on this one for over a year.  If the current rate is any 
indication, it will take man years to get caught up.

>>> Testcases look good.  It'd be nice to also add a few testcases where
>>> copy detection is turned on -- in particular, I'd like to see one with
>>> --find-renames=$DIFFERENT_THAN_DEFAULT being passed when
>>> merge.renames=copies.
>>>
>>
>> OK.  I also added tests to verify the settings correctly impact commit.
> 
> Nice!
>