From: Ben Peart Date: Thu, 10 May 2018 19:09:23 GMT Subject: Re: [PATCH v2] add status config and command line options for rename detection Message-ID: In-Reply-To: On 5/10/2018 12:19 PM, Elijah Newren wrote: > Hi Ben, > > On Thu, May 10, 2018 at 7:16 AM, Ben Peart 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[=] 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@gmail.com/ > > 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. >