Web lists-archives.com

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






On 5/10/2018 6:31 PM, Elijah Newren wrote:
Hi Ben,

On Thu, May 10, 2018 at 12:09 PM, Ben Peart <peartben@xxxxxxxxx> wrote:
On 5/10/2018 12:19 PM, Elijah Newren wrote:
On Thu, May 10, 2018 at 7:16 AM, Ben Peart <Ben.Peart@xxxxxxxxxxxxx>
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!