From: Ben Peart Date: Fri, 11 May 2018 12:50:27 GMT Subject: Re: [PATCH v2] add status config and command line options for rename detection Message-ID: <276c981b-3b6b-4034-7aaa-dbfcba4ae3f1@gmail.com> In-Reply-To: On 5/10/2018 6:31 PM, Elijah Newren wrote: > Hi Ben, > > On Thu, May 10, 2018 at 12:09 PM, Ben Peart wrote: >> On 5/10/2018 12:19 PM, Elijah Newren wrote: >>> On Thu, May 10, 2018 at 7:16 AM, Ben Peart >>> 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! >