On 5/10/2018 12:19 PM, Elijah Newren wrote:
Hi Ben,
On Thu, May 10, 2018 at 7:16 AM, Ben Peart <Ben.Peart@xxxxxxxxxxxxx> wrote:
After performing a merge that has conflicts, git status will by default attempt
to detect renames which causes many objects to be examined. In a virtualized
repo, those objects do not exist locally so the rename logic triggers them to be
fetched from the server. This results in the status call taking hours to
complete on very large repos. Even in a small repo (the GVFS repo) turning off
break and rename detection has a significant impact:
It'd be nice if you could show that impact by comparing 'git status'
to 'git status --no-renames', for some repo. Showing only the latter
gives us no way to assess the impact.
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.
git status --no-renames:
31 secs., 105 loose object downloads
git status --no-breaks
7 secs., 17 loose object downloads
git status --no-breaks --no-renames
1 sec., 1 loose object download
This patch doesn't add a --no-breaks option and it doesn't exist
previously, so adding it to the commit message serves to confuse
rather than help. I'd just drop the last two of these (and redo the
timing for --no-renames assuming you are built on
em/status-rename-config).
OK
Add a new config status.renames setting to enable turning off rename detection
during status. This setting will default to the value of diff.renames.
Add a new config status.renamelimit setting to to enable bounding the time spent
finding out inexact renames during status. This setting will default to the
value of diff.renamelimit.
It may be worth mentioning that these config settings also affect 'git
commit' (and it does, in my testing, which I think is a good thing).
I agree this is a good thing as the other status settings behave the
same way. I'll update the documentation to reflect this as well.
Add status --no-renames command line option that enables overriding the config
setting from the command line. Add --find-renames[=<n>] to enable detecting
renames and optionally setting the similarity index from the command line.
The command line options are specific to 'git status'. I don't really
have a strong opinion on whether they should also be added to
git-commit; I suspect users would be more likely to use the config
options in order to set it once and forget about it and that users
would be more likely to want to override their config setting for
status than for commit.
Note: I removed the --no-breaks command line option from the original patch as
it will no longer be needed once the default has been changed [1] to turn it off.
[1] https://public-inbox.org/git/20180430093421.27551-2-eckhard.s.maass@xxxxxxxxx/
I'd just drop these lines from the commit message, and instead mention
that your patch depends on em/status-rename-config.
OK
+ 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.
+++ b/t/t7525-status-rename.sh
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.
+test_expect_success 'setup' '
+ echo 1 >original &&
+ git add . &&
+ git commit -m"Adding original file." &&
+ mv original renamed &&
+ echo 2 >> renamed &&
+ git add .
+'
+cat >.gitignore <<\EOF
+.gitignore
+expect*
+actual*
+EOF
Can this just be included in the setup?
OK
Everything else in the patch looked good to me.
