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!
