| Message ID | pull.1384.v3.git.1667424467505.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] status: long status advice adapted to recent capabilities | expand |
On Wed, Nov 02, 2022 at 09:27:47PM +0000, Rudy Rigot via GitGitGadget wrote: > From: Rudy Rigot <rudy.rigot@gmail.com> > > Improve the advice displayed when `git status` is slow because > of excessive numbers of untracked files. Update the `git status` > man page to explain the various configuration options. This one is looking good to me. Jeff: do you agree? If so, I'm ready to start merging this one down. Thanks, Taylor
On 11/2/22 5:27 PM, Rudy Rigot via GitGitGadget wrote:> +UNTRACKED FILES AND STATUS SPEED > +-------------------------------- > + > +`git status` can be very slow in large worktrees if/when it > +needs to search for untracked files and directories. There are > +many configuration options available to speed this up by either > +avoiding the work or making use of cached results from previous > +Git commands. Since we all work in different ways, there is no > +single optimum set of settings right for everyone. Here is a > +brief summary of the relevant options to help you choose which > +is right for you. Each of these settings is independently > +documented elsewhere in more detail, so please refer to them > +for complete details. Sorry I'm late to this series. This new section is a great idea. > +* `-uno` or `status.showUntrackedFiles=false` : just don't search Two nits: 1. Is it clear that `-uno` is an option to `git status`? Should we say "The `-uno` flag or the `status.showUntrackedfiles=false` config"? 2. Drop the "just" as it implies simplicity but is unnecessary. In fact, I'd replace the sentence with: Indicate that `git status` should not report untracked files. > + and don't report on untracked files. This is the fastest. "This is the fastest option." ? > + `git status` will not list the untracked files, so you need > + to be careful to remember if you create any new files and > + manually `git add` them. > + > +* `advice.statusUoption=false` : search, but don't complain if it > + takes too long. Perhaps: This config option disables a warning message when the search for untracked files takes longer than desired. and maybe even describe why: In some large repositories, this message may appear frequently and not be a helpful signal. > +* `core.untrackedCache=true` : enable the untracked cache feature > + and only search directories that have been modified since the > + previous `git status` command. Git remembers the set of > + untracked files within each directory and assumes that if a > + directory has not been modified, then the set of untracked > + file within has not changed. This is much faster than > + enumerating the contents of every directory, but still not > + without cost, because Git still has to search for the set of > + modified directories. It might be helpful to mention that the untracked cache is stored in the .git/index file. The reduced cost searching for untracked files is offset slightly by the increased size of the index and the cost of keeping it up-to-date. That reduced search time is usually worth the additional size. > +* `core.untrackedCache=true` and `core.fsmonitor=true` or > + `core.fsmonitor=<hook_command_pathname>` : enable both the > + untracked cache and FSMonitor features and only search > + directories that have been modified since the previous > + `git status` command. This is faster than using just the > + untracked cache alone because Git can also avoid searching > + for modified directories. Git only has to enumerate the > + exact set of directories that have changed recently. It might be worth explicitly mentioning that while the FSMonitor feature can be enabled without the untracked cache, the benefits are greatly reduced in that case. > +Note that after you turn on the untracked cache and/or FSMonitor > +features it may take a few `git status` commands for the various > +caches to warm up before you see improved command times. This is > +normal. > + > SEE ALSO > -------- > linkgit:gitignore[5] > diff --git a/t/t7065-wtstatus-slow.sh b/t/t7065-wtstatus-slow.sh > new file mode 100755 > index 00000000000..23c37ea71e7 > --- /dev/null > +++ b/t/t7065-wtstatus-slow.sh > @@ -0,0 +1,40 @@ > +#!/bin/sh > + > +test_description='test status when slow untracked files' > + > +. ./test-lib.sh > + > +DATA="$TEST_DIRECTORY/t7065" > + > +GIT_TEST_UF_DELAY_WARNING=1 > +export GIT_TEST_UF_DELAY_WARNING > + > +test_expect_success setup ' > + git checkout -b test > +' > + > +test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' > + test_must_fail git config --get core.untrackedCache && > + test_must_fail git config --get core.fsmonitor && > + git status | sed "s/[0-9]\.[0-9][0-9]/X/g" >../actual && > + test_cmp "$DATA/no_untrackedcache_no_fsmonitor" ../actual && > + rm -fr ../actual > +' > + > +test_expect_success 'when core.untrackedCache true, but not fsmonitor' ' > + git config core.untrackedCache true && > + test_must_fail git config --get core.fsmonitor && > + git status | sed "s/[0-9]\.[0-9][0-9]/X/g" >../actual && > + test_cmp "$DATA/with_untrackedcache_no_fsmonitor" ../actual && > + rm -fr ../actual > +' > + > +test_expect_success 'when core.untrackedCache true, and fsmonitor' ' > + git config core.untrackedCache true && > + git config core.fsmonitor true && > + git status | sed "s/[0-9]\.[0-9][0-9]/X/g" >../actual && > + test_cmp "$DATA/with_untrackedcache_with_fsmonitor" ../actual && > + rm -fr ../actual > +' > + > +test_done > diff --git a/t/t7065/no_untrackedcache_no_fsmonitor b/t/t7065/no_untrackedcache_no_fsmonitor > diff --git a/t/t7065/with_untrackedcache_no_fsmonitor b/t/t7065/with_untrackedcache_no_fsmonitor > diff --git a/t/t7065/with_untrackedcache_with_fsmonitor b/t/t7065/with_untrackedcache_with_fsmonitor These files are small enough that I think I'd rather see them be created in the test script. You can use this kind of syntax: cat >expect <<-\EOF <file contents here> EOF && and then the test is self-contained. This is particularly helpful when a test fails due to some future change in this message. > +static inline int uf_was_slow(uint32_t untracked_in_ms) > +{ > + const char *x; > + x = getenv("GIT_TEST_UF_DELAY_WARNING"); > + if (x) { > + untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; > + } > + > + return UF_DELAY_WARNING_IN_MS < untracked_in_ms; > +} > + > static void show_merge_in_progress(struct wt_status *s, > const char *color) > { > @@ -1814,6 +1827,7 @@ static void wt_longstatus_print(struct wt_status *s) > { > const char *branch_color = color(WT_STATUS_ONBRANCH, s); > const char *branch_status_color = color(WT_STATUS_HEADER, s); > + enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(s->repo); > > if (s->branch) { > const char *on_what = _("On branch "); > @@ -1870,13 +1884,23 @@ static void wt_longstatus_print(struct wt_status *s) > wt_longstatus_print_other(s, &s->untracked, _("Untracked files"), "add"); > if (s->show_ignored_mode) > wt_longstatus_print_other(s, &s->ignored, _("Ignored files"), "add -f"); > - if (advice_enabled(ADVICE_STATUS_U_OPTION) && 2000 < s->untracked_in_ms) { > - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > - status_printf_ln(s, GIT_COLOR_NORMAL, > - _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n" > - "may speed it up, but you have to be careful not to forget to add\n" > - "new files yourself (see 'git help status')."), > - s->untracked_in_ms / 1000.0); > + if (uf_was_slow(s->untracked_in_ms)) { > + if (advice_enabled(ADVICE_STATUS_U_OPTION)) { Since there isn't an "else" for either of these cases, they can be combined with "&&" in the top-level if, reducing the tab depth for the body. > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > + if (fsm_mode > FSMONITOR_MODE_DISABLED) { > + status_printf_ln(s, GIT_COLOR_NORMAL, > + _("It took %.2f seconds to enumerate untracked files,\n" > + "but this is currently being cached."), > + s->untracked_in_ms / 1000.0); > + } else { > + status_printf_ln(s, GIT_COLOR_NORMAL, > + _("It took %.2f seconds to enumerate untracked files."), > + s->untracked_in_ms / 1000.0); > + } > + status_printf_ln(s, GIT_COLOR_NORMAL, > + _("See 'git help status' for information on how to improve this.")); > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > + } > } other than that nit, the code looks good to me. Thanks for working on this! -Stolee
On 11/4/22 5:40 PM, Taylor Blau wrote: > On Wed, Nov 02, 2022 at 09:27:47PM +0000, Rudy Rigot via GitGitGadget wrote: >> From: Rudy Rigot <rudy.rigot@gmail.com> >> >> Improve the advice displayed when `git status` is slow because >> of excessive numbers of untracked files. Update the `git status` >> man page to explain the various configuration options. > > This one is looking good to me. Jeff: do you agree? If so, I'm ready to > start merging this one down. Sorry, I came in with some late review. I think one more round would be helpful. Thanks, -Stolee
On Mon, Nov 7, 2022 at 3:07 PM Derrick Stolee <derrickstolee@github.com> wrote: > On 11/2/22 5:27 PM, Rudy Rigot via GitGitGadget wrote:> +UNTRACKED FILES AND STATUS SPEED > > +test_expect_success 'when core.untrackedCache true, and fsmonitor' ' > > + git config core.untrackedCache true && > > + git config core.fsmonitor true && > > + git status | sed "s/[0-9]\.[0-9][0-9]/X/g" >../actual && > > + test_cmp "$DATA/with_untrackedcache_with_fsmonitor" ../actual && > > + rm -fr ../actual > > +' > > + > > diff --git a/t/t7065/no_untrackedcache_no_fsmonitor b/t/t7065/no_untrackedcache_no_fsmonitor > > diff --git a/t/t7065/with_untrackedcache_no_fsmonitor b/t/t7065/with_untrackedcache_no_fsmonitor > > diff --git a/t/t7065/with_untrackedcache_with_fsmonitor b/t/t7065/with_untrackedcache_with_fsmonitor > > These files are small enough that I think I'd rather see them > be created in the test script. You can use this kind of syntax: > > cat >expect <<-\EOF > <file contents here> > EOF && > > and then the test is self-contained. This is particularly > helpful when a test fails due to some future change in this > message. I've been meaning to respond to say the exact same thing about using here-doc instead. Also, the new tests seem to have an oddball mixture of indentation using spaces and TAB. They should be using TAB only.
Thanks to both of you Derrick and Eric, this is all great feedback. I've been through all of it, it's all crystal clear, and I'm planning to just integrate all of it as is. I'll have a new iteration later this week (it's small stuff, I know, but I'm volunteering as a poll worker today and tomorrow). Thanks again!
On Mon, Nov 07, 2022 at 03:02:09PM -0500, Derrick Stolee wrote: > On 11/4/22 5:40 PM, Taylor Blau wrote: > > On Wed, Nov 02, 2022 at 09:27:47PM +0000, Rudy Rigot via GitGitGadget wrote: > >> From: Rudy Rigot <rudy.rigot@gmail.com> > >> > >> Improve the advice displayed when `git status` is slow because > >> of excessive numbers of untracked files. Update the `git status` > >> man page to explain the various configuration options. > > > > This one is looking good to me. Jeff: do you agree? If so, I'm ready to > > start merging this one down. > > Sorry, I came in with some late review. I think one more round > would be helpful. Thanks, all. Will wait for one more round before we start merging this down. Thanks, Taylor
On 11/4/22 5:40 PM, Taylor Blau wrote: > On Wed, Nov 02, 2022 at 09:27:47PM +0000, Rudy Rigot via GitGitGadget wrote: >> From: Rudy Rigot <rudy.rigot@gmail.com> >> >> Improve the advice displayed when `git status` is slow because >> of excessive numbers of untracked files. Update the `git status` >> man page to explain the various configuration options. > > This one is looking good to me. Jeff: do you agree? If so, I'm ready to > start merging this one down. > > Thanks, > Taylor This series is looking good to me too. I think V5 has addressed all of the concerns. Jeff
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 54a4b29b473..95f4ed95e96 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -457,6 +457,53 @@ during the write may conflict with other simultaneous processes, causing them to fail. Scripts running `status` in the background should consider using `git --no-optional-locks status` (see linkgit:git[1] for details). +UNTRACKED FILES AND STATUS SPEED +-------------------------------- + +`git status` can be very slow in large worktrees if/when it +needs to search for untracked files and directories. There are +many configuration options available to speed this up by either +avoiding the work or making use of cached results from previous +Git commands. Since we all work in different ways, there is no +single optimum set of settings right for everyone. Here is a +brief summary of the relevant options to help you choose which +is right for you. Each of these settings is independently +documented elsewhere in more detail, so please refer to them +for complete details. + +* `-uno` or `status.showUntrackedFiles=false` : just don't search + and don't report on untracked files. This is the fastest. + `git status` will not list the untracked files, so you need + to be careful to remember if you create any new files and + manually `git add` them. + +* `advice.statusUoption=false` : search, but don't complain if it + takes too long. + +* `core.untrackedCache=true` : enable the untracked cache feature + and only search directories that have been modified since the + previous `git status` command. Git remembers the set of + untracked files within each directory and assumes that if a + directory has not been modified, then the set of untracked + file within has not changed. This is much faster than + enumerating the contents of every directory, but still not + without cost, because Git still has to search for the set of + modified directories. + +* `core.untrackedCache=true` and `core.fsmonitor=true` or + `core.fsmonitor=<hook_command_pathname>` : enable both the + untracked cache and FSMonitor features and only search + directories that have been modified since the previous + `git status` command. This is faster than using just the + untracked cache alone because Git can also avoid searching + for modified directories. Git only has to enumerate the + exact set of directories that have changed recently. + +Note that after you turn on the untracked cache and/or FSMonitor +features it may take a few `git status` commands for the various +caches to warm up before you see improved command times. This is +normal. + SEE ALSO -------- linkgit:gitignore[5] diff --git a/t/t7065-wtstatus-slow.sh b/t/t7065-wtstatus-slow.sh new file mode 100755 index 00000000000..23c37ea71e7 --- /dev/null +++ b/t/t7065-wtstatus-slow.sh @@ -0,0 +1,40 @@ +#!/bin/sh + +test_description='test status when slow untracked files' + +. ./test-lib.sh + +DATA="$TEST_DIRECTORY/t7065" + +GIT_TEST_UF_DELAY_WARNING=1 +export GIT_TEST_UF_DELAY_WARNING + +test_expect_success setup ' + git checkout -b test +' + +test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' + test_must_fail git config --get core.untrackedCache && + test_must_fail git config --get core.fsmonitor && + git status | sed "s/[0-9]\.[0-9][0-9]/X/g" >../actual && + test_cmp "$DATA/no_untrackedcache_no_fsmonitor" ../actual && + rm -fr ../actual +' + +test_expect_success 'when core.untrackedCache true, but not fsmonitor' ' + git config core.untrackedCache true && + test_must_fail git config --get core.fsmonitor && + git status | sed "s/[0-9]\.[0-9][0-9]/X/g" >../actual && + test_cmp "$DATA/with_untrackedcache_no_fsmonitor" ../actual && + rm -fr ../actual +' + +test_expect_success 'when core.untrackedCache true, and fsmonitor' ' + git config core.untrackedCache true && + git config core.fsmonitor true && + git status | sed "s/[0-9]\.[0-9][0-9]/X/g" >../actual && + test_cmp "$DATA/with_untrackedcache_with_fsmonitor" ../actual && + rm -fr ../actual +' + +test_done diff --git a/t/t7065/no_untrackedcache_no_fsmonitor b/t/t7065/no_untrackedcache_no_fsmonitor new file mode 100644 index 00000000000..91dc3719cda --- /dev/null +++ b/t/t7065/no_untrackedcache_no_fsmonitor @@ -0,0 +1,9 @@ +On branch test + +No commits yet + + +It took X seconds to enumerate untracked files. +See 'git help status' for information on how to improve this. + +nothing to commit (create/copy files and use "git add" to track) diff --git a/t/t7065/with_untrackedcache_no_fsmonitor b/t/t7065/with_untrackedcache_no_fsmonitor new file mode 100644 index 00000000000..91dc3719cda --- /dev/null +++ b/t/t7065/with_untrackedcache_no_fsmonitor @@ -0,0 +1,9 @@ +On branch test + +No commits yet + + +It took X seconds to enumerate untracked files. +See 'git help status' for information on how to improve this. + +nothing to commit (create/copy files and use "git add" to track) diff --git a/t/t7065/with_untrackedcache_with_fsmonitor b/t/t7065/with_untrackedcache_with_fsmonitor new file mode 100644 index 00000000000..89d2dd5c2e7 --- /dev/null +++ b/t/t7065/with_untrackedcache_with_fsmonitor @@ -0,0 +1,10 @@ +On branch test + +No commits yet + + +It took X seconds to enumerate untracked files, +but this is currently being cached. +See 'git help status' for information on how to improve this. + +nothing to commit (create/copy files and use "git add" to track) diff --git a/wt-status.c b/wt-status.c index 5813174896c..4dfc8a8969b 100644 --- a/wt-status.c +++ b/wt-status.c @@ -18,8 +18,10 @@ #include "worktree.h" #include "lockfile.h" #include "sequencer.h" +#include "fsmonitor-settings.h" #define AB_DELAY_WARNING_IN_MS (2 * 1000) +#define UF_DELAY_WARNING_IN_MS (2 * 1000) static const char cut_line[] = "------------------------ >8 ------------------------\n"; @@ -1205,6 +1207,17 @@ static void wt_longstatus_print_tracking(struct wt_status *s) strbuf_release(&sb); } +static inline int uf_was_slow(uint32_t untracked_in_ms) +{ + const char *x; + x = getenv("GIT_TEST_UF_DELAY_WARNING"); + if (x) { + untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; + } + + return UF_DELAY_WARNING_IN_MS < untracked_in_ms; +} + static void show_merge_in_progress(struct wt_status *s, const char *color) { @@ -1814,6 +1827,7 @@ static void wt_longstatus_print(struct wt_status *s) { const char *branch_color = color(WT_STATUS_ONBRANCH, s); const char *branch_status_color = color(WT_STATUS_HEADER, s); + enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(s->repo); if (s->branch) { const char *on_what = _("On branch "); @@ -1870,13 +1884,23 @@ static void wt_longstatus_print(struct wt_status *s) wt_longstatus_print_other(s, &s->untracked, _("Untracked files"), "add"); if (s->show_ignored_mode) wt_longstatus_print_other(s, &s->ignored, _("Ignored files"), "add -f"); - if (advice_enabled(ADVICE_STATUS_U_OPTION) && 2000 < s->untracked_in_ms) { - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); - status_printf_ln(s, GIT_COLOR_NORMAL, - _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n" - "may speed it up, but you have to be careful not to forget to add\n" - "new files yourself (see 'git help status')."), - s->untracked_in_ms / 1000.0); + if (uf_was_slow(s->untracked_in_ms)) { + if (advice_enabled(ADVICE_STATUS_U_OPTION)) { + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + if (fsm_mode > FSMONITOR_MODE_DISABLED) { + status_printf_ln(s, GIT_COLOR_NORMAL, + _("It took %.2f seconds to enumerate untracked files,\n" + "but this is currently being cached."), + s->untracked_in_ms / 1000.0); + } else { + status_printf_ln(s, GIT_COLOR_NORMAL, + _("It took %.2f seconds to enumerate untracked files."), + s->untracked_in_ms / 1000.0); + } + status_printf_ln(s, GIT_COLOR_NORMAL, + _("See 'git help status' for information on how to improve this.")); + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + } } } else if (s->committable) status_printf_ln(s, GIT_COLOR_NORMAL, _("Untracked files not listed%s"),