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

From: Elijah Newren <newren@gmail.com>
Date: May 10, 2018, 22:31 UTC
Message-ID: <CABPp-BGL2Fcnj0QNu+D3wfOU5q8MuizUGJyzrEc7FXy9Q9aA_A@mail.gmail.com>
In-Reply-To: <bc81823e-7b7d-c516-dfc2-cd47bedb5a5a@gmail.com>

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:

Show 5 quoted lines

> 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.

Show 16 quoted lines

>>> +       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), so I'm not sure why we'd want to carefully
propagate a larger value for o->{diff,merge}_detect_rename prior to
this point.  If it's just "future proofing" because you suspect that
copy information could be useful to the merging algorithm and we'll
eventually get rid of these two lines of code, then I could get behind
such a change, though color me skeptical that copy information would
ever turn out to be useful in that context.

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.

Show 7 quoted lines

>> 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!