Hi Ben, On Thu, May 10, 2018 at 7:16 AM, Ben Peart <ben.pe...@microsoft.com> 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. > 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). > 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). > 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.ma...@gmail.com/ I'd just drop these lines from the commit message, and instead mention that your patch depends on em/status-rename-config. > Original-Patch-by: Alejandro Pauly <alpa...@microsoft.com> > Signed-off-by: Ben Peart <ben.pe...@microsoft.com> > --- > > Notes: > Base Ref: master This patch does not apply to master; it has conflicts. > Web-Diff: https://github.com/benpeart/git/commit/823212725b This web diff shows em/status-rename-config as the parent commit, not master. Since your commit message mentions you want the change to break detection provided by that series, just listing it as the explicit base seems like the right way to go. > ### Interdiff (v1..v2): Thanks. > + 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. > +++ 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. > +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? Everything else in the patch looked good to me.
