* [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case @ 2022-10-15 13:04 Rudy Rigot via GitGitGadget 2022-10-15 13:08 ` Rudy Rigot ` (2 more replies) 0 siblings, 3 replies; 58+ messages in thread From: Rudy Rigot via GitGitGadget @ 2022-10-15 13:04 UTC (permalink / raw) To: git; +Cc: Rudy Rigot, Rudy Rigot From: Rudy Rigot <rudy.rigot@gmail.com> Currently, if git-status takes more than 2 seconds for enumerating untracked files, a piece of advice is given to the user to consider ignoring untracked files. This is somewhat at odds with the UX upsides from having fsmonitor enabled, since fsmonitor will be here to take care of mitigating the performance downsides from those untracked files. I considered just suppressing that piece of advice entirely for repositories with fsmonitor disabled, but I decided to replace it with another piece of advice instead, letting the user know that this run may have been slow, but the next ones should be faster. Of course, please let me know if the phrasing can be improved. To keep consistent with other pieces of advice, this new one can be hidden with a new advice.statusFsmonitor config. If the repository does not have fsmonitor enabled, or if the new piece of advice is hidden by config, the behavior falls back to today's behavior: show the message advising to ignore untracked files, as long as it wasn't disabled with the existing advice.statusUoption config. Test-wise, I tried to figure out ways to mock the behavior of a slow git-status, but I couldn't figure it out, so I could use some advice. I tracked down Commit 6a38ef2ced (status: advise to consider use of -u when read_directory takes too long, 2013-03-13), and it also didn't have tests, so I'm questioning whether it can in fact be reasonably done. Thanks in advance for any guidance. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> --- fsmonitor: long status advice adapted to the fsmonitor use case Currently, if git-status takes more than 2 seconds for enumerating untracked files, a piece of advice is given to the user to consider ignoring untracked files. This is somewhat at odds with the UX upsides from having fsmonitor enabled, since fsmonitor will be here to take care of mitigating the performance downsides from those untracked files. I considered just suppressing that piece of advice entirely for repositories with fsmonitor disabled, but I decided to replace it with another piece of advice instead, letting the user know that this run may have been slow, but the next ones should be faster. Of course, please let me know if the phrasing can be improved. To keep consistent with other pieces of advice, this new one can be hidden with a new advice.statusFsmonitor config. If the repository does not have fsmonitor enabled, or if the new piece of advice is hidden by config, the behavior falls back to today's behavior: show the message advising to ignore untracked files, as long as it wasn't disabled with the existing advice.statusUoption config. Test-wise, I tried to figure out ways to mock the behavior of a slow git-status, but I couldn't figure it out, so I could use some advice. I tracked down Commit 6a38ef2 (status: advise to consider use of -u when read_directory takes too long, 2013-03-13), and it also didn't have tests, so I'm questioning whether it can in fact be reasonably done. Thanks in advance for any guidance. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1384%2Frudyrigot%2Fadvice_statusFsmonitor-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1384/rudyrigot/advice_statusFsmonitor-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1384 Documentation/config/advice.txt | 3 +++ advice.c | 1 + advice.h | 1 + wt-status.c | 23 ++++++++++++++++------- 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index a00d0100a82..e8ebcf1b023 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -57,6 +57,9 @@ advice.*:: and that calculation takes longer than expected. Will not appear if `status.aheadBehind` is false or the option `--no-ahead-behind` is given. + statusFsmonitor:: + Shown when the linkgit:git-status[1] command takes more than + 2 seconds to enumerate untracked files, and fsmonitor is enabled. statusHints:: Show directions on how to proceed from the current state in the output of linkgit:git-status[1], in diff --git a/advice.c b/advice.c index fd189689437..448b11ef273 100644 --- a/advice.c +++ b/advice.c @@ -69,6 +69,7 @@ static struct { [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure", 1 }, [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1 }, [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning", 1 }, + [ADVICE_STATUS_FSMONITOR] = { "statusFsmonitor", 1 }, [ADVICE_STATUS_HINTS] = { "statusHints", 1 }, [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 }, [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 }, diff --git a/advice.h b/advice.h index 07e0f76833e..a458619a160 100644 --- a/advice.h +++ b/advice.h @@ -43,6 +43,7 @@ struct string_list; ADVICE_SEQUENCER_IN_USE, ADVICE_SET_UPSTREAM_FAILURE, ADVICE_STATUS_AHEAD_BEHIND_WARNING, + ADVICE_STATUS_FSMONITOR, ADVICE_STATUS_HINTS, ADVICE_STATUS_U_OPTION, ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE, diff --git a/wt-status.c b/wt-status.c index 5813174896c..d6c7f0fa21a 100644 --- a/wt-status.c +++ b/wt-status.c @@ -18,6 +18,7 @@ #include "worktree.h" #include "lockfile.h" #include "sequencer.h" +#include "fsmonitor-settings.h" #define AB_DELAY_WARNING_IN_MS (2 * 1000) @@ -1814,6 +1815,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 +1872,20 @@ 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 (2000 < s->untracked_in_ms) { + if (advice_enabled(ADVICE_STATUS_FSMONITOR) && fsm_mode > FSMONITOR_MODE_DISABLED) { + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + status_printf_ln(s, GIT_COLOR_NORMAL, + _("It took a while to check your git status this time, but the results\n" + "were cached, and your next runs should be faster.")); + } else if (advice_enabled(ADVICE_STATUS_U_OPTION)) { + 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); + } } } else if (s->committable) status_printf_ln(s, GIT_COLOR_NORMAL, _("Untracked files not listed%s"), base-commit: d420dda0576340909c3faff364cfbd1485f70376 -- gitgitgadget ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case 2022-10-15 13:04 [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case Rudy Rigot via GitGitGadget @ 2022-10-15 13:08 ` Rudy Rigot 2022-10-17 15:39 ` Jeff Hostetler 2022-10-29 0:06 ` [PATCH v2] status: long status advice adapted to recent capabilities Rudy Rigot via GitGitGadget 2 siblings, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-10-15 13:08 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget; +Cc: git > I considered just suppressing that piece of advice entirely for repositories with fsmonitor disabled Sorry, I just noticed that I misspoke here. I meant repositories with fsmonitor *enabled*, of course. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case 2022-10-15 13:04 [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case Rudy Rigot via GitGitGadget 2022-10-15 13:08 ` Rudy Rigot @ 2022-10-17 15:39 ` Jeff Hostetler 2022-10-17 16:59 ` Rudy Rigot 2022-10-29 0:06 ` [PATCH v2] status: long status advice adapted to recent capabilities Rudy Rigot via GitGitGadget 2 siblings, 1 reply; 58+ messages in thread From: Jeff Hostetler @ 2022-10-17 15:39 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget, git; +Cc: Rudy Rigot On 10/15/22 9:04 AM, Rudy Rigot via GitGitGadget wrote: > From: Rudy Rigot <rudy.rigot@gmail.com> > > Currently, if git-status takes more than 2 seconds for enumerating > untracked files, a piece of advice is given to the user to consider > ignoring untracked files. This is somewhat at odds with the UX > upsides from having fsmonitor enabled, since fsmonitor will be > here to take care of mitigating the performance downsides from > those untracked files. Yes, the original advice needs some updating. Thanks! We should be careful here, FSMonitor only helps with untracked files if the untracked cache (UC) is also turned on. They do work well together and they greatly speed up things, but if either is turned off, `git status` will still need to scan. (Small nerd detail: the UC by itself does speed things up -- it only needs to lstat known directories most of the time, rather than actually readdir them. When both are on, we don't even need to do that.) The original message suggested turning off the untracked file (UF) scan completely. Perhaps, we could have advice say to turn off UF scan -or- turn on FSM & UC ? > > I considered just suppressing that piece of advice entirely for > repositories with fsmonitor disabled, but I decided to replace > it with another piece of advice instead, letting the user know > that this run may have been slow, but the next ones should be faster. > Of course, please let me know if the phrasing can be improved. To > keep consistent with other pieces of advice, this new one can be > hidden with a new advice.statusFsmonitor config. > > If the repository does not have fsmonitor enabled, or if the new > piece of advice is hidden by config, the behavior falls back to > today's behavior: show the message advising to ignore untracked > files, as long as it wasn't disabled with the existing advice.statusUoption > config. We also need to be careful here. With FSMonitor the "first one is slow, the second one is fast" happens because `git status` is secretly updating the index (despite looking like a read-only command). That causes the index to have an updated FSM token (so that subsequent FSM responses are relative to a more recent checkpoint). See [1]. So it isn't always correct that the second status will be faster. It usually is, but it just depends on the threshold -- and more importantly, that the UC is turned on. (So I guess what I'm saying is that again, we should advise to turn UF or turn on both FSM & UC.) [1] 26b9f34ab3 (fsmonitor: force update index after large responses, 2022-03-25) > > Test-wise, I tried to figure out ways to mock the behavior of a > slow git-status, but I couldn't figure it out, so I could use some > advice. I tracked down Commit 6a38ef2ced (status: advise to consider > use of -u when read_directory takes too long, 2013-03-13), and it > also didn't have tests, so I'm questioning whether it can in fact > be reasonably done. Thanks in advance for any guidance. [...] WRT testing, you might do something like: #define UF_DELAY_WARNING_IN_MS (2 * 1000) static inline int uf_was_slow(uint32_t untracked_in_ms) { #ifdef GIT_TEST_UF_DELAY_WARNING // or maybe use getenv() here untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; #endif return UF_DELAY_WARNING_IN_MS < untracked_in_ms; } And then you can set the GIT_TEST_ symbol (or env var) during the test scripts and confirm that we get the messages that you expect. Hope this helps, Jeff ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case 2022-10-17 15:39 ` Jeff Hostetler @ 2022-10-17 16:59 ` Rudy Rigot 2022-10-20 12:56 ` Jeff Hostetler 0 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot @ 2022-10-17 16:59 UTC (permalink / raw) To: Jeff Hostetler; +Cc: Rudy Rigot via GitGitGadget, git > We should be careful here, FSMonitor only helps with untracked files if the untracked cache (UC) is also turned on. They do work well together and they greatly speed up things, but if either is turned off, `git status` will still need to scan. Oh, thanks, I didn't realize! With that, I agree that the messaging I'm proposing is not technically correct, and needs to be fixed. I agree with your advice about the message when FSMonitor and untracked cache are both turned off. I'm also trying to think about what advice to give when they are turned on, and git status was slow because the update-index was building the cache on that call. Importantly, I'm trying to think of ways to keep the messaging accessible even when the user is not familiar with those concepts. I'm thinking a user may not even know what is currently enabled or not in their environment, so there's probably value in detecting their situation, and best adapting the messaging to it. For context, in our case, we set core.fsmonitor and core.untrackedCache as part of our dev environment setup script, because we don't expect our least advanced developers to ramp up on what they are. And yet, it is useful to all of them to have them enabled, our git status is about 30 seconds long without FSMonitor and UC. As a result, we have been receiving negative feedback that git status is slow, but when we inform the user that it is cached, they run it again and confirm that it is fine like this. The problem being about educating users and not a technical issue, of course we're adding the info to our setup doc, but I figured other large repos may hit this usability issue, so here I am. What do you think of those phrasings? - If neither FSMonitor nor the untracked cache are turned on, changing the current advice to: "It took %.2f seconds to enumerate untracked files. You may want to skip that part with 'status -uno' but you have to be careful not to forget to add new files yourself (see 'git help status'). Otherwise, you can enable the core.untrackedCache config to have it be cached, and potentially the core.fsmonitor config to further improve the cache's performance." - If only the untracked cache is turned on, since you said it could already improve some: "It took %.2f seconds to enumerate untracked files. Your untracked cache is enabled, but you may want to enable the core.fsmonitor config to further improve the cache's performance. Otherwise, you may want to skip that part with 'status -uno' but you have to be careful not to forget to add new files yourself (see 'git help status')." - If they're both turned on: "It took %.2f seconds to enumerate untracked files. Your untracked cache is enabled and fsmonitor is on, so your next calls may be faster. Otherwise, you may want to skip that part with 'status -uno' but you have to be careful not to forget to add new files yourself (see 'git help status')." I intentionally phrased it all in a way that manages expectations, "may be faster", since you said the state of the FSM token could be impacted by a variety of things, so we can't exactly commit to anything for sure about the next call. Let me know what you think, I definitely want to do this right. > WRT testing Oh, I didn't realize I could do this! Alright, I need time to look into it. With that, it means I can test the existing advice message use case, which I don't believe is currently tested, so I'm double-glad! Thanks a lot for all the insight. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case 2022-10-17 16:59 ` Rudy Rigot @ 2022-10-20 12:56 ` Jeff Hostetler 2022-10-20 20:17 ` Rudy Rigot 0 siblings, 1 reply; 58+ messages in thread From: Jeff Hostetler @ 2022-10-20 12:56 UTC (permalink / raw) To: Rudy Rigot; +Cc: Rudy Rigot via GitGitGadget, git On 10/17/22 12:59 PM, Rudy Rigot wrote: >> We should be careful here, FSMonitor only helps with untracked > files if the untracked cache (UC) is also turned on. They do work > well together and they greatly speed up things, but if either is > turned off, `git status` will still need to scan. > > Oh, thanks, I didn't realize! With that, I agree that the messaging I'm > proposing is not technically correct, and needs to be fixed. > > I agree with your advice about the message when FSMonitor and > untracked cache are both turned off. I'm also trying to think about > what advice to give when they are turned on, and git status was > slow because the update-index was building the cache on that call. > > Importantly, I'm trying to think of ways to keep the messaging > accessible even when the user is not familiar with those concepts. Agreed. We sometimes forget that not everyone is an expert in the subtleties and terms of Git. Even the original message "...enumerate untracked files..." can be a little obscure to the casual user. > I'm thinking a user may not even know what is currently enabled or > not in their environment, so there's probably value in detecting their > situation, and best adapting the messaging to it. > > For context, in our case, we set core.fsmonitor and core.untrackedCache > as part of our dev environment setup script, because we don't expect > our least advanced developers to ramp up on what they are. And yet, > it is useful to all of them to have them enabled, our git status is about > 30 seconds long without FSMonitor and UC. > > As a result, we have been receiving negative feedback that git status is > slow, but when we inform the user that it is cached, they run it again and > confirm that it is fine like this. The problem being about educating > users and not a technical issue, of course we're adding the info to our > setup doc, but I figured other large repos may hit this usability issue, so > here I am. > > What do you think of those phrasings? > > - If neither FSMonitor nor the untracked cache are turned on, changing > the current advice to: "It took %.2f seconds to enumerate untracked files. > You may want to skip that part with 'status -uno' but you have to be > careful not to forget to add new files yourself (see 'git help status'). > Otherwise, you can enable the core.untrackedCache config to have > it be cached, and potentially the core.fsmonitor config to further improve > the cache's performance." > > - If only the untracked cache is turned on, since you said it could already > improve some: "It took %.2f seconds to enumerate untracked files. > Your untracked cache is enabled, but you may want to enable the > core.fsmonitor config to further improve the cache's performance. > Otherwise, you may want to skip that part with 'status -uno' but you have > to be careful not to forget to add new files yourself (see 'git help status')." > > - If they're both turned on: "It took %.2f seconds to enumerate untracked > files. Your untracked cache is enabled and fsmonitor is on, > so your next calls may be faster. Otherwise, you may want to skip that part > with 'status -uno' but you have to be careful not to forget to add new > files yourself (see 'git help status')." All three of these are a bit wordy and/or awkward -- but I'm not sure how to say it either, so don't feel bad. I think it is just the quality of the corner that we've been backed into -- there are just too many knobs and not enough space to do them justice. And I've always found the "turn this on, but be careful..." advice/warning a bit odd. As an alternative, and definitely just thinking out loud here. Would it be better to add a "Untracked Files and Status Speed" section to the bottom of https://git-scm.com/docs/git-status (aka `Documentation/git-status.txt`) near https://git-scm.com/docs/git-status#_background_refresh Describe the various combination of config options in more detail and discuss the pros/cons of each, what the defaults are, and how to set them, how to tell what they are currently set to, and so on. Then have the advice message reference that new section. We might also reword the large paragraph ("When `-u` option is not...") in the `--untracked-files` argument to reference the new section. It is a little stale and just as awkward. Just a thought... Jeff ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case 2022-10-20 12:56 ` Jeff Hostetler @ 2022-10-20 20:17 ` Rudy Rigot 2022-10-24 14:55 ` Jeff Hostetler 0 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot @ 2022-10-20 20:17 UTC (permalink / raw) To: Jeff Hostetler; +Cc: Rudy Rigot via GitGitGadget, git Oooh, I definitely like where your mind is with this. I think it makes a lot of sense, there used to be one way to act upon a slow status, now there are a few, so I can see how any in-depth explanation here would add to the confusion for a user in a terminal who is just trying to get things done. And I see how the current messaging already kinda infringes on that. Alright, I can write the first draft of the documentation changes you were mentioning. Heads up though: I'm going to need your tight review of it, because I'm not as completely comfortable with what each option does as I wish I was, so I worry I may write something accidentally inaccurate. I'll take the time to read up on it though, and then I'll try my hand at it and update it here, and let's take it from there if that sounds good. With that, I'm thinking the "slow status" advice could be turned into something as simple as: > Your git status run was slow, here are some ways to optimize it. > https://git-scm.com/docs/git-status#_performance_optimizations (To be clear, I'm very un-opinionated about phrasing.) There's one thing I'm still worried about though, which is what you mentioned earlier, and which brought me here: the fact that git-status feels like a read-only command, but secretly isn't. I'm thinking the confusing use case is when the repository was in fact set in a way that things are cached, and yet git-status is still slow because it was generating that cache, and the user doesn't have a way to know that. Adding to the murkiness, it might not have been the reason, so I understand we can't say things in such a confident manner as "it will be faster now", because of course it depends. So I'm thinking, after the message above when git-status was slow, in the specific case where the untracked cache is on (whether FSMonitor is on or not, since that sounds more like under-the-hood detail), we could display something like the additional line here: > Your git status run was slow, here are some ways to optimize it. > https://git-scm.com/docs/git-status#_performance_optimizations > > Your git status run was cached. If the untracked cache is on, I'm assuming that would be always accurate information, is that correct? If you're concerned that users may not understand what it means for them, we could also make it more obvious without over-committing about it: > Your git status run was slow, here are some ways to optimize it. > https://git-scm.com/docs/git-status#_performance_optimizations > > Your git status run was cached, it may be faster on your next runs. What do you think? To summarize, next steps for me: - Make the first draft for the doc updates. - Change the advice messaging based on our discussion above and what you think of it. - I still need to look into your test-related advice from last time, I haven't yet. I really would like to give tests to all this. Thanks a lot for all this, it helps tremendously! ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case 2022-10-20 20:17 ` Rudy Rigot @ 2022-10-24 14:55 ` Jeff Hostetler 0 siblings, 0 replies; 58+ messages in thread From: Jeff Hostetler @ 2022-10-24 14:55 UTC (permalink / raw) To: Rudy Rigot; +Cc: Rudy Rigot via GitGitGadget, git On 10/20/22 4:17 PM, Rudy Rigot wrote: > Oooh, I definitely like where your mind is with this. I think it makes a > lot of sense, there used to be one way to act upon a slow status, now there > are a few, so I can see how any in-depth explanation here would add to the > confusion for a user in a terminal who is just trying to get things done. > And I see how the current messaging already kinda infringes on that. > > Alright, I can write the first draft of the documentation changes you were > mentioning. Heads up though: I'm going to need your tight review of it, > because I'm not as completely comfortable with what each option does as I > wish I was, so I worry I may write something accidentally inaccurate. I'll > take the time to read up on it though, and then I'll try my hand at it and > update it here, and let's take it from there if that sounds good. > [...] yeah, i'm happy to help. just let me know when you have a first draft. Jeff ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v2] status: long status advice adapted to recent capabilities 2022-10-15 13:04 [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case Rudy Rigot via GitGitGadget 2022-10-15 13:08 ` Rudy Rigot 2022-10-17 15:39 ` Jeff Hostetler @ 2022-10-29 0:06 ` Rudy Rigot via GitGitGadget 2022-11-02 19:45 ` Jeff Hostetler 2022-11-02 21:27 ` [PATCH v3] " Rudy Rigot via GitGitGadget 2 siblings, 2 replies; 58+ messages in thread From: Rudy Rigot via GitGitGadget @ 2022-10-29 0:06 UTC (permalink / raw) To: git; +Cc: Rudy Rigot, Rudy Rigot From: Rudy Rigot <rudy.rigot@gmail.com> Currently, if git-status takes more than 2 seconds for enumerating untracked files, a piece of advice is given to the user to consider ignoring untracked files. But Git now offers more possibilities to resolve that situation (untracked cache, fsmonitor) with different downsides. This change is about refreshing that advice message. A new section in the documentation is introduced to present the possibilities, and the advice message links to it. I'm also introducing tests for this advice message, which was untested so far. One of the downsides of untracked cache / fsmonitor, is that the first call may be long in order to generate the cache, but the user may not know what their current configuration is. When collecting feedback from users of our very large repo, that's the most common point of confusion that keeps coming back: people complain about git status being slow, but are satisfied when we inform them that it's being cached and they should run it again to check. As a result, the advice message tries to keep them informed of their current configuration. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> --- status: long status advice adapted to recent capabilities Here is version 2 for this patch to change the long status advice to adapt to recent capabilities (untracked cache, FSMonitor). Clerical note: I /preview'd this with GitGitGadget, and this seems to be expressed as a patch on top of my first patch ("Range-diff vs v1") first, before showing the actual patch as refreshed. I'm assuming that is what is expected, please let me know if I'm doing this wrong. Changes since v1: * Introduced a new section in git status's docs to explain the various options, that the new advice can link to. I realized there's already solid details about untracked cache and fsmonitor in the update-index's docs, so I kept the new section very high-level and strategic, linking to update-index's docs for more details. I don't think anything's inaccurate, but I really could use some eyes on this anyway to be sure. * Changed the advice message depending on context (see test files to see every possible phrasing the end user can meet). I am very open to feedback, of course. One of my priorities was to keep the already-optimized user in the loop about what optimizations they may already have, in case they're only seeing this message because it's the git status run that did the first caching, since that's what's been confusing our users when using fsmonitor. * Introduced tests for all this. I chose to simulate the slowness thanks to a an environment variable rather than a symbol, just because it was also helpful to set it from the outside at dev time, but I'm not strongly opinionated about whether it should be one or the other. * Removed the advice.statusFsmonitor config I had introduced in v1, since it's all one short consolidated advice message now. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1384%2Frudyrigot%2Fadvice_statusFsmonitor-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1384/rudyrigot/advice_statusFsmonitor-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1384 Range-diff vs v1: 1: cc372e7c9d0 ! 1: 9ef7f1834b7 fsmonitor: long status advice adapted to the fsmonitor use case @@ Metadata Author: Rudy Rigot <rudy.rigot@gmail.com> ## Commit message ## - fsmonitor: long status advice adapted to the fsmonitor use case + status: long status advice adapted to recent capabilities - Currently, if git-status takes more than 2 seconds for enumerating - untracked files, a piece of advice is given to the user to consider - ignoring untracked files. This is somewhat at odds with the UX - upsides from having fsmonitor enabled, since fsmonitor will be - here to take care of mitigating the performance downsides from - those untracked files. + Currently, if git-status takes more than 2 seconds for enumerating untracked + files, a piece of advice is given to the user to consider ignoring untracked + files. But Git now offers more possibilities to resolve that situation + (untracked cache, fsmonitor) with different downsides. - I considered just suppressing that piece of advice entirely for - repositories with fsmonitor disabled, but I decided to replace - it with another piece of advice instead, letting the user know - that this run may have been slow, but the next ones should be faster. - Of course, please let me know if the phrasing can be improved. To - keep consistent with other pieces of advice, this new one can be - hidden with a new advice.statusFsmonitor config. + This change is about refreshing that advice message. A new section in the + documentation is introduced to present the possibilities, and the advice + message links to it. I'm also introducing tests for this advice message, + which was untested so far. - If the repository does not have fsmonitor enabled, or if the new - piece of advice is hidden by config, the behavior falls back to - today's behavior: show the message advising to ignore untracked - files, as long as it wasn't disabled with the existing advice.statusUoption - config. - - Test-wise, I tried to figure out ways to mock the behavior of a - slow git-status, but I couldn't figure it out, so I could use some - advice. I tracked down Commit 6a38ef2ced (status: advise to consider - use of -u when read_directory takes too long, 2013-03-13), and it - also didn't have tests, so I'm questioning whether it can in fact - be reasonably done. Thanks in advance for any guidance. + One of the downsides of untracked cache / fsmonitor, is that the first call + may be long in order to generate the cache, but the user may not know what + their current configuration is. When collecting feedback from users of our + very large repo, that's the most common point of confusion that keeps coming + back: people complain about git status being slow, but are satisfied when + we inform them that it's being cached and they should run it again to check. + As a result, the advice message tries to keep them informed of their current + configuration. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> - ## Documentation/config/advice.txt ## -@@ Documentation/config/advice.txt: advice.*:: - and that calculation takes longer than expected. Will not - appear if `status.aheadBehind` is false or the option - `--no-ahead-behind` is given. -+ statusFsmonitor:: -+ Shown when the linkgit:git-status[1] command takes more than -+ 2 seconds to enumerate untracked files, and fsmonitor is enabled. - statusHints:: - Show directions on how to proceed from the current - state in the output of linkgit:git-status[1], in + ## Documentation/git-status.txt ## +@@ Documentation/git-status.txt: 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 ++-------------------------------- ++ ++If your untracked files take an unusual amount of time to enumerate, your ++repository certainly has a lot of them, and an advice message will display ++about it. Here are some configurations to consider in order to improve the ++situation: ++ ++* Setting the `core.untrackedCache` configuration as `true` will allow for ++`git status` to keep track of the mtime of folders, in order to cache past ++`status` results and be sure to only browse folders that changed on subsequent ++runs, for filesystems that can support it (see linkgit:git-update-index[1] ++for details). ++* Used in conjonction with `core.untrackedCache`, setting the `core.fsmonitor` ++configuration as `true` will allow for `git status` to keep track of what ++files recently changed, in order to cache past `status` results and be sure ++to only focus on those files on subsequent runs (see linkgit:git-update-index[1] ++for details). ++* If none of the above options are satisfactory, setting the ++`status.showUntrackedFiles` configuration as `no` will cause `git status` ++to not attempt to list untracked files anymore, in which case you have to be ++careful not to forget to add new files yourself. ++ ++If none of the above solutions are satisfactory, and you are bothered with ++the advice message, you can disable it by setting the `advice.statusUoption` ++configuration to `false`. ++ + SEE ALSO + -------- + linkgit:gitignore[5] + + ## t/t7065-wtstatus-slow.sh (new) ## +@@ ++#!/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 + \ No newline at end of file - ## advice.c ## -@@ advice.c: static struct { - [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure", 1 }, - [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1 }, - [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning", 1 }, -+ [ADVICE_STATUS_FSMONITOR] = { "statusFsmonitor", 1 }, - [ADVICE_STATUS_HINTS] = { "statusHints", 1 }, - [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 }, - [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 }, + ## t/t7065/no_untrackedcache_no_fsmonitor (new) ## +@@ ++On branch test ++ ++No commits yet ++ ++ ++It took X seconds to enumerate untracked files. ++See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed ++for configuration options that may improve that time. ++ ++nothing to commit (create/copy files and use "git add" to track) - ## advice.h ## -@@ advice.h: struct string_list; - ADVICE_SEQUENCER_IN_USE, - ADVICE_SET_UPSTREAM_FAILURE, - ADVICE_STATUS_AHEAD_BEHIND_WARNING, -+ ADVICE_STATUS_FSMONITOR, - ADVICE_STATUS_HINTS, - ADVICE_STATUS_U_OPTION, - ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE, + ## t/t7065/with_untrackedcache_no_fsmonitor (new) ## +@@ ++On branch test ++ ++No commits yet ++ ++ ++It took X seconds to enumerate untracked files, ++but this is currently being cached, with fsmonitor OFF. ++See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed ++for configuration options that may improve that time. ++ ++nothing to commit (create/copy files and use "git add" to track) + + ## t/t7065/with_untrackedcache_with_fsmonitor (new) ## +@@ ++On branch test ++ ++No commits yet ++ ++ ++It took X seconds to enumerate untracked files, ++but this is currently being cached, with fsmonitor ON. ++See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed ++for configuration options that may improve that time. ++ ++nothing to commit (create/copy files and use "git add" to track) ## wt-status.c ## @@ @@ wt-status.c +#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"; +@@ wt-status.c: 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) + { @@ wt-status.c: static void wt_longstatus_print(struct wt_status *s) { const char *branch_color = color(WT_STATUS_ONBRANCH, s); @@ wt-status.c: static void wt_longstatus_print(struct wt_status *s) - "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 (2000 < s->untracked_in_ms) { -+ if (advice_enabled(ADVICE_STATUS_FSMONITOR) && fsm_mode > FSMONITOR_MODE_DISABLED) { ++ if (uf_was_slow(s->untracked_in_ms)) { ++ if (advice_enabled(ADVICE_STATUS_U_OPTION)) { + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); ++ if (s->repo->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE) { ++ status_printf_ln(s, GIT_COLOR_NORMAL, ++ _("It took %.2f seconds to enumerate untracked files,\n" ++ "but this is currently being cached, with fsmonitor %s."), ++ s->untracked_in_ms / 1000.0, ++ (fsm_mode > FSMONITOR_MODE_DISABLED) ? "ON" : "OFF"); ++ } 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, -+ _("It took a while to check your git status this time, but the results\n" -+ "were cached, and your next runs should be faster.")); -+ } else if (advice_enabled(ADVICE_STATUS_U_OPTION)) { ++ _("See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed\n" ++ "for configuration options that may improve that time.")); + 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); + } } } else if (s->committable) Documentation/git-status.txt | 27 +++++++++++++++ t/t7065-wtstatus-slow.sh | 40 ++++++++++++++++++++++ t/t7065/no_untrackedcache_no_fsmonitor | 10 ++++++ t/t7065/with_untrackedcache_no_fsmonitor | 11 ++++++ t/t7065/with_untrackedcache_with_fsmonitor | 11 ++++++ wt-status.c | 40 ++++++++++++++++++---- 6 files changed, 132 insertions(+), 7 deletions(-) create mode 100755 t/t7065-wtstatus-slow.sh create mode 100644 t/t7065/no_untrackedcache_no_fsmonitor create mode 100644 t/t7065/with_untrackedcache_no_fsmonitor create mode 100644 t/t7065/with_untrackedcache_with_fsmonitor diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 54a4b29b473..3d92e5fd018 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -457,6 +457,33 @@ 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 +-------------------------------- + +If your untracked files take an unusual amount of time to enumerate, your +repository certainly has a lot of them, and an advice message will display +about it. Here are some configurations to consider in order to improve the +situation: + +* Setting the `core.untrackedCache` configuration as `true` will allow for +`git status` to keep track of the mtime of folders, in order to cache past +`status` results and be sure to only browse folders that changed on subsequent +runs, for filesystems that can support it (see linkgit:git-update-index[1] +for details). +* Used in conjonction with `core.untrackedCache`, setting the `core.fsmonitor` +configuration as `true` will allow for `git status` to keep track of what +files recently changed, in order to cache past `status` results and be sure +to only focus on those files on subsequent runs (see linkgit:git-update-index[1] +for details). +* If none of the above options are satisfactory, setting the +`status.showUntrackedFiles` configuration as `no` will cause `git status` +to not attempt to list untracked files anymore, in which case you have to be +careful not to forget to add new files yourself. + +If none of the above solutions are satisfactory, and you are bothered with +the advice message, you can disable it by setting the `advice.statusUoption` +configuration to `false`. + 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..92c053eaa64 --- /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 \ No newline at end of file diff --git a/t/t7065/no_untrackedcache_no_fsmonitor b/t/t7065/no_untrackedcache_no_fsmonitor new file mode 100644 index 00000000000..e346deaa1db --- /dev/null +++ b/t/t7065/no_untrackedcache_no_fsmonitor @@ -0,0 +1,10 @@ +On branch test + +No commits yet + + +It took X seconds to enumerate untracked files. +See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed +for configuration options that may improve that time. + +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..a649d367493 --- /dev/null +++ b/t/t7065/with_untrackedcache_no_fsmonitor @@ -0,0 +1,11 @@ +On branch test + +No commits yet + + +It took X seconds to enumerate untracked files, +but this is currently being cached, with fsmonitor OFF. +See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed +for configuration options that may improve that time. + +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..d5e95d984f8 --- /dev/null +++ b/t/t7065/with_untrackedcache_with_fsmonitor @@ -0,0 +1,11 @@ +On branch test + +No commits yet + + +It took X seconds to enumerate untracked files, +but this is currently being cached, with fsmonitor ON. +See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed +for configuration options that may improve that time. + +nothing to commit (create/copy files and use "git add" to track) diff --git a/wt-status.c b/wt-status.c index 5813174896c..be903c6e294 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,25 @@ 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 (s->repo->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE) { + status_printf_ln(s, GIT_COLOR_NORMAL, + _("It took %.2f seconds to enumerate untracked files,\n" + "but this is currently being cached, with fsmonitor %s."), + s->untracked_in_ms / 1000.0, + (fsm_mode > FSMONITOR_MODE_DISABLED) ? "ON" : "OFF"); + } 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 https://git-scm.com/docs/git-status#_untracked_files_and_status_speed\n" + "for configuration options that may improve that time.")); + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + } } } else if (s->committable) status_printf_ln(s, GIT_COLOR_NORMAL, _("Untracked files not listed%s"), base-commit: bbe21b64a08f89475d8a3818e20c111378daa621 -- gitgitgadget ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v2] status: long status advice adapted to recent capabilities 2022-10-29 0:06 ` [PATCH v2] status: long status advice adapted to recent capabilities Rudy Rigot via GitGitGadget @ 2022-11-02 19:45 ` Jeff Hostetler 2022-11-02 20:34 ` Rudy Rigot 2022-11-02 23:59 ` Taylor Blau 2022-11-02 21:27 ` [PATCH v3] " Rudy Rigot via GitGitGadget 1 sibling, 2 replies; 58+ messages in thread From: Jeff Hostetler @ 2022-11-02 19:45 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget, git; +Cc: Rudy Rigot On 10/28/22 8:06 PM, Rudy Rigot via GitGitGadget wrote: > From: Rudy Rigot <rudy.rigot@gmail.com> > > Currently, if git-status takes more than 2 seconds for enumerating untracked > files, a piece of advice is given to the user to consider ignoring untracked > files. But Git now offers more possibilities to resolve that situation > (untracked cache, fsmonitor) with different downsides. > > This change is about refreshing that advice message. A new section in the > documentation is introduced to present the possibilities, and the advice > message links to it. I'm also introducing tests for this advice message, > which was untested so far. > > One of the downsides of untracked cache / fsmonitor, is that the first call > may be long in order to generate the cache, but the user may not know what > their current configuration is. When collecting feedback from users of our > very large repo, that's the most common point of confusion that keeps coming > back: people complain about git status being slow, but are satisfied when > we inform them that it's being cached and they should run it again to check. > As a result, the advice message tries to keep them informed of their current > configuration. Let me suggest an alternative commit message. We want to lead with a "command" -- as in: "make Git do this" or "teach Git to do this". Then explain why. Maybe something like: 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. `git status` can be slow when there are a large number of untracked files and directories, because Git must search the entire worktree to enumerate them. Previously, Git would print an advice message with the elapsed search time and a suggestion to disable the search using the `-uno` option. This suggestion also carried a warning that might scare off some users. Git can reduce the size and time of the untracked file search when the `core.untrackedCache` and `core.fsmonitor` features are enabled by caching results from previous `git status` invocations. Update the advice to explain the various combinations of additional configuration options and refer to (new) documentation in the man page that explains it in more detail than what can be printed in an advice message. Finally, add new tests to verify the new functionality. Or something like that. :-) [...] > diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt > index 54a4b29b473..3d92e5fd018 100644 > --- a/Documentation/git-status.txt > +++ b/Documentation/git-status.txt > @@ -457,6 +457,33 @@ 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 > +-------------------------------- > + > +If your untracked files take an unusual amount of time to enumerate, your > +repository certainly has a lot of them, and an advice message will display > +about it. Here are some configurations to consider in order to improve the > +situation: > + > +* Setting the `core.untrackedCache` configuration as `true` will allow for > +`git status` to keep track of the mtime of folders, in order to cache past > +`status` results and be sure to only browse folders that changed on subsequent > +runs, for filesystems that can support it (see linkgit:git-update-index[1] > +for details). > +* Used in conjonction with `core.untrackedCache`, setting the `core.fsmonitor` > +configuration as `true` will allow for `git status` to keep track of what > +files recently changed, in order to cache past `status` results and be sure > +to only focus on those files on subsequent runs (see linkgit:git-update-index[1] > +for details). > +* If none of the above options are satisfactory, setting the > +`status.showUntrackedFiles` configuration as `no` will cause `git status` > +to not attempt to list untracked files anymore, in which case you have to be > +careful not to forget to add new files yourself. > + > +If none of the above solutions are satisfactory, and you are bothered with > +the advice message, you can disable it by setting the `advice.statusUoption` > +configuration to `false`. > + I hate to suggest a complete rewrite as I myself struggle with how to phrase things all toooooo often, but let me offer another starting point and see what you think: `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. Something like that. Hope this helps. (And again, sorry for the rewrite.) [...] > diff --git a/t/t7065-wtstatus-slow.sh b/t/t7065-wtstatus-slow.sh > new file mode 100755 > index 00000000000..92c053eaa64 > --- /dev/null > +++ b/t/t7065-wtstatus-slow.sh > @@ -0,0 +1,40 @@ > +#!/bin/sh [...] > + > +test_done > \ No newline at end of file small nit: we should have a final LF at the end of the file. I'm going to skip over the test cases because I'm running short on time this afternoon. [...] > diff --git a/wt-status.c b/wt-status.c [...] > 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,25 @@ 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 (s->repo->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE) { > + status_printf_ln(s, GIT_COLOR_NORMAL, > + _("It took %.2f seconds to enumerate untracked files,\n" > + "but this is currently being cached, with fsmonitor %s."), > + s->untracked_in_ms / 1000.0, > + (fsm_mode > FSMONITOR_MODE_DISABLED) ? "ON" : "OFF"); > + } 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 https://git-scm.com/docs/git-status#_untracked_files_and_status_speed\n" > + "for configuration options that may improve that time.")); > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > + } I'm not sure I like the various mixture of messages here. Maybe it would be better with a single simple message: _("It took %.2f seconds to enumerate untracked files.\n" "See 'git help status' for information on how to improve this.") This keeps all of the information in the documentation rather than having part of it here in the code. Also, we should refer to the documentation via `git help` rather than as a link to the website. Thanks, Jeff ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2] status: long status advice adapted to recent capabilities 2022-11-02 19:45 ` Jeff Hostetler @ 2022-11-02 20:34 ` Rudy Rigot 2022-11-02 23:59 ` Taylor Blau 1 sibling, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-11-02 20:34 UTC (permalink / raw) To: Jeff Hostetler; +Cc: Rudy Rigot via GitGitGadget, git Thanks a lot for all that feedback. Honestly you're way more familiar than I am with both how the underlying features work, and how to best contribute this change, so I'm comfortable taking your advice pretty much blindly. > Let me suggest an alternative commit message. We want to lead with a > "command" -- as in: "make Git do this" or "teach Git to do this". Then > explain why. Oops, sorry for missing this. Well, your commit message suggestion is flawless, I will reuse this as is. Thanks a lot for spending time polishing it. > I hate to suggest a complete rewrite > [...] > Something like that. Hope this helps. (And again, sorry for the rewrite.) There's absolutely nothing to apologize about. This is great! I will also reuse as is. Here too, thanks a lot for the time spent on this! > small nit: we should have a final LF at the end of the file. Sounds good, will fix. > I'm going to skip over the test cases because I'm running > short on time this afternoon. That's all good, I need to align my submission to all this and resubmit anyway, so you got time! I'm pretty confident about them too, a lot more than in the phrasing of the user-facing content I had. > Also, we should refer to the documentation via `git help` rather > than as a link to the website. Oops, I didn't realize people were getting the same content from either. I will fix. > I'm not sure I like the various mixture of messages here. Maybe > it would be better with a single simple message: That's the one thing I'm a bit concerned about, that I would like to discuss more if that's ok. The current confusion we're seeing with users of our very large repo, is that they run git status the first time and notice it being slow (~30s), and then they see the current advice message advising that they're supposed to do something about it. What they don't know, is that untrackedcache and fsmonitor were already set for their environment, by the script setting their entire environment up. I don't think it is unusual for users to not necessarily know how their environment was configured (either because someone/something else did it for them, or because they forgot what they did for this specific repo, for instance). So with that, I worry about the phrasing "See 'git help status' for information on how to improve this." in that use case, because it implies that there is something they are expected to go improve, while that was already done. Here are some solution ideas: * Changing the wording for all use cases to not convey that they must do anything about it. For instance just "See 'git help status'". (I don't love this because I could imagine users being puzzled about why Git is telling them this, then.) * Informing the user of their current caching situation in ways that they can deduce whether or not they should be doing something about it. That's what I was attempting to do here, but reading your help content, I think I got something wrong: I didn't realize the cache would only need to warm up with untrackedcache + fsmonitor, and not with untrackedcache alone. So with that an improvement could be to only display "but this is currently being cached." when both untrackedcache + fsmonitor are on, and not display anything different when only untrackedcache is on then when it's not. * Not displaying this advice message when fsmonitor is on, since the best possible optimization was already applied. Not loving it, because some could also still want untracked files off on top of it. Also, it doesn't resolve the frustration of noticing git status being slow the first time. For now if you don't mind, I'll change things to the 2nd proposal up there, but this is not because I'm rejecting your guidance and insisting that adding a line here is the right solution to the situation of environments that were already optimized, and I'm not sure what is. But I do worry that telling those users that they should optimize things while they/someone already did will surely be confusing. I'll work on the other fixes, and I am deeply interested in your thoughts about this one. Thanks a lot for working through this with me. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2] status: long status advice adapted to recent capabilities 2022-11-02 19:45 ` Jeff Hostetler 2022-11-02 20:34 ` Rudy Rigot @ 2022-11-02 23:59 ` Taylor Blau 2022-11-03 14:28 ` Rudy Rigot 1 sibling, 1 reply; 58+ messages in thread From: Taylor Blau @ 2022-11-02 23:59 UTC (permalink / raw) To: Jeff Hostetler; +Cc: Rudy Rigot via GitGitGadget, git, Rudy Rigot On Wed, Nov 02, 2022 at 03:45:18PM -0400, Jeff Hostetler wrote: > Let me suggest an alternative commit message. We want to lead with a > "command" -- as in: "make Git do this" or "teach Git to do this". Then > explain why. Maybe something like: > > [...] Excellent suggestions, thank you. > > @@ -1870,13 +1884,25 @@ 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 (s->repo->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE) { > > + status_printf_ln(s, GIT_COLOR_NORMAL, > > + _("It took %.2f seconds to enumerate untracked files,\n" > > + "but this is currently being cached, with fsmonitor %s."), > > + s->untracked_in_ms / 1000.0, > > + (fsm_mode > FSMONITOR_MODE_DISABLED) ? "ON" : "OFF"); > > + } 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 https://git-scm.com/docs/git-status#_untracked_files_and_status_speed\n" > > + "for configuration options that may improve that time.")); > > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > > + } > > I'm not sure I like the various mixture of messages here. Maybe > it would be better with a single simple message: > > _("It took %.2f seconds to enumerate untracked files.\n" > "See 'git help status' for information on how to improve this.") > > This keeps all of the information in the documentation rather > than having part of it here in the code. > > Also, we should refer to the documentation via `git help` rather > than as a link to the website. I agree with your suggestion of not linking out to git-scm.com here, but I wonder if we could get by without mentioning 'git help' here, either. Presumably looking up an unknown configuration variable with 'man git-config' is easy enough. Thanks, Taylor ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2] status: long status advice adapted to recent capabilities 2022-11-02 23:59 ` Taylor Blau @ 2022-11-03 14:28 ` Rudy Rigot 2022-11-04 8:52 ` Ævar Arnfjörð Bjarmason 2022-11-04 21:38 ` Taylor Blau 0 siblings, 2 replies; 58+ messages in thread From: Rudy Rigot @ 2022-11-03 14:28 UTC (permalink / raw) To: Taylor Blau; +Cc: Jeff Hostetler, Rudy Rigot via GitGitGadget, git > looking up an unknown configuration variable with 'man > git-config' is easy enough. I'm not strongly opinionated, but I believe the initial idea behind redirecting them to the doc was because Git now comes with more configuration abilities to improve performance of git status, that may be more or less relevant depending on use cases, so there isn't really a single git-config key for them to look up any more. Their ideal solution could be core.untrackedCache=true, core.fsmonitor=true, advice.statusUoption=false, status.showUntrackedFiles=false, or even some combinations of those can be relevant. From there, the goal I believe we were going for with this new doc section is to let users know what configs exist for their git status slowness pains and why, so they can then go look those configs up for more details, which I agree would indeed be easy from there. Again, I'm not strongly opinionated, and I hope I accurately represented the inital thinking on this idea. One slightly stronger opinion I have, is that if the advice message was just > It took %.2f seconds to enumerate untracked files. and nothing else, I can definitely see a strong UX downside of not giving a hint of next steps for users. Basically, "you have a problem, and we're not helping you resolve it". Were you thinking more of something like this? > It took %.2f seconds to enumerate untracked files. > Please look up the core.untrackedCache, core.fsmonitor > advice.statusUoption, and status.showUntrackedFiles configs > for potential solutions. I'd say that's probably somewhat cryptic and a bit verbose (which is what we were trying to avoid by telling them to go see the doc), but we wouldn't be leaving the user stranded, so I can see how that would work out ok. I'm very interested in what you think. Thanks, ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2] status: long status advice adapted to recent capabilities 2022-11-03 14:28 ` Rudy Rigot @ 2022-11-04 8:52 ` Ævar Arnfjörð Bjarmason 2022-11-04 15:33 ` Rudy Rigot 2022-11-04 21:38 ` Taylor Blau 1 sibling, 1 reply; 58+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-04 8:52 UTC (permalink / raw) To: Rudy Rigot; +Cc: Taylor Blau, Jeff Hostetler, Rudy Rigot via GitGitGadget, git On Thu, Nov 03 2022, Rudy Rigot wrote: >> looking up an unknown configuration variable with 'man >> git-config' is easy enough. > > I'm not strongly opinionated, but I believe the initial idea behind > redirecting them to the doc was because Git now comes with more > configuration abilities to improve performance of git status, that may > be more or less relevant depending on use cases, so there > isn't really a single git-config key for them to look up any more. Their > ideal solution could be core.untrackedCache=true, core.fsmonitor=true, > advice.statusUoption=false, status.showUntrackedFiles=false, or even > some combinations of those can be relevant. > > From there, the goal I believe we were going for with this new doc > section is to let users know what configs exist for their git status > slowness pains and why, so they can then go look those configs up for > more details, which I agree would indeed be easy from there. > > Again, I'm not strongly opinionated, and I hope I accurately represented > the inital thinking on this idea. > > One slightly stronger opinion I have, is that if the advice message > was just > >> It took %.2f seconds to enumerate untracked files. > > and nothing else, I can definitely see a strong UX downside of not > giving a hint of next steps for users. Basically, "you have a problem, > and we're not helping you resolve it". Were you thinking more of > something like this? > >> It took %.2f seconds to enumerate untracked files. >> Please look up the core.untrackedCache, core.fsmonitor >> advice.statusUoption, and status.showUntrackedFiles configs >> for potential solutions. > > I'd say that's probably somewhat cryptic and a bit verbose (which is > what we were trying to avoid by telling them to go see the doc), but > we wouldn't be leaving the user stranded, so I can see how that would > work out ok. > > I'm very interested in what you think. On the topic in general: I think it's probably a good thing to show the advice, but I just want to point out that it's not without cost. Right now we're showing users a pretty basic command they can try, but now we're showing them other stuff that needs more complex setup. For some they're probably way better off, e.g. the untracked cache is pretty much an unambiguous win (we should probably turn it on on default, but we'd need to check on-the-fly if the FS supports it properly). But for e.g. fsmonitor the user may spend a lot of time fiddling with it, only to find it doesn't help their use-case much, if it all. Should we still point out these possibilities? Probably, but just say'n. One thing that I find glaringly omitted, which since you're working on this you might consider adding: Suggest to just try running the exact same command again, maybe it was just the FS cache. I.e. we're suggesting all this advanced stuff, but by far the biggest difference is made on e.g. a modern *nix box (particularly Linux) by just having all the repo's assets in the FS cache. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2] status: long status advice adapted to recent capabilities 2022-11-04 8:52 ` Ævar Arnfjörð Bjarmason @ 2022-11-04 15:33 ` Rudy Rigot 0 siblings, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-11-04 15:33 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Taylor Blau, Jeff Hostetler, Rudy Rigot via GitGitGadget, git > One thing that I find glaringly omitted, which since you're working on > this you might consider adding: Suggest to just try running the exact > same command again, maybe it was just the FS cache. I have to admit that would be by far my personal preference. We've been having a number of very great points made by several people on this thread, but a number of them contradicting each other across people, and yet clearly nobody's wrong, everybody makes very real points. I'm trying to turn this into actionable changes I should make, but I think I need guidance on that. This is my first ever contribution to the project, so I'm lacking the organizational awareness of the project to be able to drive this to a consensus on what we should do. Here's a proposal that tries to make opinionated moves towards what I understand to be the priorities that were expressed: 1- We keep the new paragraph doc, because I'd say why not, it's well-written (thanks Jeff!) and useful. When people are looking for ways to make git status faster, it's good that there's a reference about it, and I'd expect it to be a common need across all kinds of user situations. 2- When untracked cache is not on, if I understand Ævar's suggestion, it would say something like: > It took %.2f seconds to enumerate untracked files. > Try to enable untracked cache to see if it helps make it faster > for you: > git config core.untrackedCache true It would satisfy that the message gives concise advice with actionable next steps, without making assumptions about whether it will or won't work. (Untracked cache alone did not make much of a difference in our very large repo's case.) And it doesn't point to the help anymore, in order not to saturate the user with too much detail. 3- When untrackedcache is on but fsmonitor is off, and git status is still slow (that's the situation we had on our very large repo), it could say something like: > It took %.2f seconds to enumerate untracked files. > Try to enable FSMonitor to see if it helps make it faster for you: > git config core.fsmonitor true Same as before, concise, no assumptions. This setup is more advanced, but we are in a case where untracked cache is not helping, so I'm thining that should be very few repos. If the user feels a need to better understand what's up, the feature is mentioned by name, so they can look it up and dig in if they wish to. 4- When fsmonitor is on: > It took %.2f seconds to enumerate untracked files. > Your runs are being cached, try running git status again to see if > it's faster. Same as before, concise, no assumptions, and matches Ævar's suggestion above that was also my preference, as it would apply perfectly to our very large repo's use case and the grievances we've received. Please let me know what your thoughts are about it all. A downside with all that is the option to disable untracked files is not mentioned at all, but if we keep the doc as it is, and it gets painful enough that they search for other ways, I'm hopefuly the user would find it there. I want to say it again: I'm not very opinionated about any of this, just trying to collate feedback into an actionable plan. If I understood feedback wrong, or my plan is not the best based on the feedback, that is very fine, but I will need guidance to know what makes more sense. > the untracked cache is > pretty much an unambiguous win (we should probably turn it on on > default, but we'd need to check on-the-fly if the FS supports it > properly). I could take on the work to make untracked cache on by default after this, as another patch, if it sounds relevant to try. I feel I lack the technical understanding of what we need to check that you're mentioning here, so I'll have questions, but I'd be on board with trying. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2] status: long status advice adapted to recent capabilities 2022-11-03 14:28 ` Rudy Rigot 2022-11-04 8:52 ` Ævar Arnfjörð Bjarmason @ 2022-11-04 21:38 ` Taylor Blau 1 sibling, 0 replies; 58+ messages in thread From: Taylor Blau @ 2022-11-04 21:38 UTC (permalink / raw) To: Rudy Rigot; +Cc: Taylor Blau, Jeff Hostetler, Rudy Rigot via GitGitGadget, git On Thu, Nov 03, 2022 at 09:28:56AM -0500, Rudy Rigot wrote: > One slightly stronger opinion I have, is that if the advice message > was just > > > It took %.2f seconds to enumerate untracked files. > > and nothing else, I can definitely see a strong UX downside of not > giving a hint of next steps for users. Basically, "you have a problem, > and we're not helping you resolve it". Were you thinking more of > something like this? > > > It took %.2f seconds to enumerate untracked files. > > Please look up the core.untrackedCache, core.fsmonitor > > advice.statusUoption, and status.showUntrackedFiles configs > > for potential solutions. > > I'd say that's probably somewhat cryptic and a bit verbose (which is > what we were trying to avoid by telling them to go see the doc), but > we wouldn't be leaving the user stranded, so I can see how that would > work out ok. > > I'm very interested in what you think. I see what you're saying. On the one hand, it feels redundant to say, "we noticed 'git status' is running slowly for you, try running 'git help status' to figure out why". But on the other hand, enumerating all possible configuration values that you may or may not want to set given your particular circumstance isn't practical either. Thinking on it more, I think what you wrote originally is reasonable. Thanks, Taylor ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3] status: long status advice adapted to recent capabilities 2022-10-29 0:06 ` [PATCH v2] status: long status advice adapted to recent capabilities Rudy Rigot via GitGitGadget 2022-11-02 19:45 ` Jeff Hostetler @ 2022-11-02 21:27 ` Rudy Rigot via GitGitGadget 2022-11-04 21:40 ` Taylor Blau ` (2 more replies) 1 sibling, 3 replies; 58+ messages in thread From: Rudy Rigot via GitGitGadget @ 2022-11-02 21:27 UTC (permalink / raw) To: git; +Cc: Jeff Hostetler, Rudy Rigot, Rudy Rigot 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. `git status` can be slow when there are a large number of untracked files and directories, because Git must search the entire worktree to enumerate them. Previously, Git would print an advice message with the elapsed search time and a suggestion to disable the search using the `-uno` option. This suggestion also carried a warning that might scare off some users. Git can reduce the size and time of the untracked file search when the `core.untrackedCache` and `core.fsmonitor` features are enabled by caching results from previous `git status` invocations. Update the advice to explain the various combinations of additional configuration options and refer to (new) documentation in the man page that explains it in more detail than what can be printed in an advice message. Finally, add new tests to verify the new functionality. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> --- status: long status advice adapted to recent capabilities Here is version 3 for this patch. Changes since v2: * Replaced copy of the commit message with the better one suggested by Jeff. * Replaced copy of the doc and the default advice message with the better ones suggested by Jeff. * Fixed EOF. * Changed the approach for users who are already optimized, pending more conversation to see what makes most sense. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1384%2Frudyrigot%2Fadvice_statusFsmonitor-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1384/rudyrigot/advice_statusFsmonitor-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1384 Range-diff vs v2: 1: 9ef7f1834b7 ! 1: 3c98492cb82 status: long status advice adapted to recent capabilities @@ Metadata ## Commit message ## status: long status advice adapted to recent capabilities - Currently, if git-status takes more than 2 seconds for enumerating untracked - files, a piece of advice is given to the user to consider ignoring untracked - files. But Git now offers more possibilities to resolve that situation - (untracked cache, fsmonitor) with different downsides. + 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 change is about refreshing that advice message. A new section in the - documentation is introduced to present the possibilities, and the advice - message links to it. I'm also introducing tests for this advice message, - which was untested so far. + `git status` can be slow when there are a large number of untracked + files and directories, because Git must search the entire worktree + to enumerate them. Previously, Git would print an advice message + with the elapsed search time and a suggestion to disable the search + using the `-uno` option. This suggestion also carried a warning + that might scare off some users. - One of the downsides of untracked cache / fsmonitor, is that the first call - may be long in order to generate the cache, but the user may not know what - their current configuration is. When collecting feedback from users of our - very large repo, that's the most common point of confusion that keeps coming - back: people complain about git status being slow, but are satisfied when - we inform them that it's being cached and they should run it again to check. - As a result, the advice message tries to keep them informed of their current - configuration. + Git can reduce the size and time of the untracked file search when + the `core.untrackedCache` and `core.fsmonitor` features are enabled + by caching results from previous `git status` invocations. + + Update the advice to explain the various combinations of additional + configuration options and refer to (new) documentation in the man + page that explains it in more detail than what can be printed in an + advice message. + + Finally, add new tests to verify the new functionality. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> @@ Documentation/git-status.txt: during the write may conflict with other simultane +UNTRACKED FILES AND STATUS SPEED +-------------------------------- + -+If your untracked files take an unusual amount of time to enumerate, your -+repository certainly has a lot of them, and an advice message will display -+about it. Here are some configurations to consider in order to improve the -+situation: -+ -+* Setting the `core.untrackedCache` configuration as `true` will allow for -+`git status` to keep track of the mtime of folders, in order to cache past -+`status` results and be sure to only browse folders that changed on subsequent -+runs, for filesystems that can support it (see linkgit:git-update-index[1] -+for details). -+* Used in conjonction with `core.untrackedCache`, setting the `core.fsmonitor` -+configuration as `true` will allow for `git status` to keep track of what -+files recently changed, in order to cache past `status` results and be sure -+to only focus on those files on subsequent runs (see linkgit:git-update-index[1] -+for details). -+* If none of the above options are satisfactory, setting the -+`status.showUntrackedFiles` configuration as `no` will cause `git status` -+to not attempt to list untracked files anymore, in which case you have to be -+careful not to forget to add new files yourself. -+ -+If none of the above solutions are satisfactory, and you are bothered with -+the advice message, you can disable it by setting the `advice.statusUoption` -+configuration to `false`. ++`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 -------- @@ t/t7065-wtstatus-slow.sh (new) +' + +test_done - \ No newline at end of file ## t/t7065/no_untrackedcache_no_fsmonitor (new) ## @@ @@ t/t7065/no_untrackedcache_no_fsmonitor (new) + + +It took X seconds to enumerate untracked files. -+See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed -+for configuration options that may improve that time. ++See 'git help status' for information on how to improve this. + +nothing to commit (create/copy files and use "git add" to track) @@ t/t7065/with_untrackedcache_no_fsmonitor (new) +No commits yet + + -+It took X seconds to enumerate untracked files, -+but this is currently being cached, with fsmonitor OFF. -+See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed -+for configuration options that may improve that time. ++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) @@ t/t7065/with_untrackedcache_with_fsmonitor (new) + + +It took X seconds to enumerate untracked files, -+but this is currently being cached, with fsmonitor ON. -+See https://git-scm.com/docs/git-status#_untracked_files_and_status_speed -+for configuration options that may improve that time. ++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) @@ wt-status.c: static void wt_longstatus_print(struct wt_status *s) + if (uf_was_slow(s->untracked_in_ms)) { + if (advice_enabled(ADVICE_STATUS_U_OPTION)) { + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); -+ if (s->repo->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE) { ++ 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, with fsmonitor %s."), -+ s->untracked_in_ms / 1000.0, -+ (fsm_mode > FSMONITOR_MODE_DISABLED) ? "ON" : "OFF"); ++ "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 https://git-scm.com/docs/git-status#_untracked_files_and_status_speed\n" -+ "for configuration options that may improve that time.")); ++ _("See 'git help status' for information on how to improve this.")); + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + } } Documentation/git-status.txt | 47 ++++++++++++++++++++++ t/t7065-wtstatus-slow.sh | 40 ++++++++++++++++++ t/t7065/no_untrackedcache_no_fsmonitor | 9 +++++ t/t7065/with_untrackedcache_no_fsmonitor | 9 +++++ t/t7065/with_untrackedcache_with_fsmonitor | 10 +++++ wt-status.c | 38 +++++++++++++---- 6 files changed, 146 insertions(+), 7 deletions(-) create mode 100755 t/t7065-wtstatus-slow.sh create mode 100644 t/t7065/no_untrackedcache_no_fsmonitor create mode 100644 t/t7065/with_untrackedcache_no_fsmonitor create mode 100644 t/t7065/with_untrackedcache_with_fsmonitor 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"), base-commit: bbe21b64a08f89475d8a3818e20c111378daa621 -- gitgitgadget ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v3] status: long status advice adapted to recent capabilities 2022-11-02 21:27 ` [PATCH v3] " Rudy Rigot via GitGitGadget @ 2022-11-04 21:40 ` Taylor Blau 2022-11-07 20:02 ` Derrick Stolee 2022-11-15 16:38 ` Jeff Hostetler 2022-11-07 20:01 ` Derrick Stolee 2022-11-10 4:46 ` [PATCH v4] " Rudy Rigot via GitGitGadget 2 siblings, 2 replies; 58+ messages in thread From: Taylor Blau @ 2022-11-04 21:40 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget; +Cc: git, Jeff Hostetler, Rudy Rigot 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3] status: long status advice adapted to recent capabilities 2022-11-04 21:40 ` Taylor Blau @ 2022-11-07 20:02 ` Derrick Stolee 2022-11-07 23:19 ` Taylor Blau 2022-11-15 16:38 ` Jeff Hostetler 1 sibling, 1 reply; 58+ messages in thread From: Derrick Stolee @ 2022-11-07 20:02 UTC (permalink / raw) To: Taylor Blau, Rudy Rigot via GitGitGadget; +Cc: git, Jeff Hostetler, Rudy Rigot 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3] status: long status advice adapted to recent capabilities 2022-11-07 20:02 ` Derrick Stolee @ 2022-11-07 23:19 ` Taylor Blau 0 siblings, 0 replies; 58+ messages in thread From: Taylor Blau @ 2022-11-07 23:19 UTC (permalink / raw) To: Derrick Stolee Cc: Taylor Blau, Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Rudy Rigot 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3] status: long status advice adapted to recent capabilities 2022-11-04 21:40 ` Taylor Blau 2022-11-07 20:02 ` Derrick Stolee @ 2022-11-15 16:38 ` Jeff Hostetler 1 sibling, 0 replies; 58+ messages in thread From: Jeff Hostetler @ 2022-11-15 16:38 UTC (permalink / raw) To: Taylor Blau, Rudy Rigot via GitGitGadget; +Cc: git, Rudy Rigot 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3] status: long status advice adapted to recent capabilities 2022-11-02 21:27 ` [PATCH v3] " Rudy Rigot via GitGitGadget 2022-11-04 21:40 ` Taylor Blau @ 2022-11-07 20:01 ` Derrick Stolee 2022-11-07 20:20 ` Eric Sunshine 2022-11-10 4:46 ` [PATCH v4] " Rudy Rigot via GitGitGadget 2 siblings, 1 reply; 58+ messages in thread From: Derrick Stolee @ 2022-11-07 20:01 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget, git; +Cc: Jeff Hostetler, Rudy Rigot 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 ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3] status: long status advice adapted to recent capabilities 2022-11-07 20:01 ` Derrick Stolee @ 2022-11-07 20:20 ` Eric Sunshine 2022-11-07 20:31 ` Rudy Rigot 0 siblings, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-07 20:20 UTC (permalink / raw) To: Derrick Stolee Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Rudy Rigot 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. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3] status: long status advice adapted to recent capabilities 2022-11-07 20:20 ` Eric Sunshine @ 2022-11-07 20:31 ` Rudy Rigot 0 siblings, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-11-07 20:31 UTC (permalink / raw) To: Eric Sunshine Cc: Derrick Stolee, Rudy Rigot via GitGitGadget, git, Jeff Hostetler 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! ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v4] status: long status advice adapted to recent capabilities 2022-11-02 21:27 ` [PATCH v3] " Rudy Rigot via GitGitGadget 2022-11-04 21:40 ` Taylor Blau 2022-11-07 20:01 ` Derrick Stolee @ 2022-11-10 4:46 ` Rudy Rigot via GitGitGadget 2022-11-10 5:42 ` Eric Sunshine 2022-11-10 20:04 ` [PATCH v5] " Rudy Rigot via GitGitGadget 2 siblings, 2 replies; 58+ messages in thread From: Rudy Rigot via GitGitGadget @ 2022-11-10 4:46 UTC (permalink / raw) To: git Cc: Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine, Rudy Rigot, Rudy Rigot 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. `git status` can be slow when there are a large number of untracked files and directories, because Git must search the entire worktree to enumerate them. Previously, Git would print an advice message with the elapsed search time and a suggestion to disable the search using the `-uno` option. This suggestion also carried a warning that might scare off some users. Git can reduce the size and time of the untracked file search when the `core.untrackedCache` and `core.fsmonitor` features are enabled by caching results from previous `git status` invocations. Update the advice to explain the various combinations of additional configuration options and refer to (new) documentation in the man page that explains it in more detail than what can be printed in an advice message. Finally, add new tests to verify the new functionality. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> --- status: long status advice adapted to recent capabilities Here is version 4 for this patch. Changes since v3: * Integrated all feedback on the doc content itself, as is. * Moved the small test files into here docs in the test code. * Fix some awkward indentations. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1384%2Frudyrigot%2Fadvice_statusFsmonitor-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1384/rudyrigot/advice_statusFsmonitor-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/1384 Range-diff vs v3: 1: 3c98492cb82 ! 1: 85b35882c02 status: long status advice adapted to recent capabilities @@ Documentation/git-status.txt: during the write may conflict with other simultane +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. ++* The `-uno` flag or the `status.showUntrackedfiles=false` ++ config : indicate that `git status` should not report untracked ++ files. 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. ++* `advice.statusUoption=false` : this config option disables a ++ warning message when the search for untracked files takes longer ++ than desired. 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 @@ Documentation/git-status.txt: during the write may conflict with other simultane + 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. ++ modified directories. 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 @@ Documentation/git-status.txt: during the write may conflict with other simultane + `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. ++ exact set of directories that have changed recently. 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 @@ t/t7065-wtstatus-slow.sh (new) +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 - - ## t/t7065/no_untrackedcache_no_fsmonitor (new) ## -@@ ++ git status | sed "s/[0-9]\.[0-9][0-9]/X/g" >../actual && ++ cat >../expected <<-EOF && +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. ++See '"'"'git help status'"'"' for information on how to improve this. + +nothing to commit (create/copy files and use "git add" to track) - - ## t/t7065/with_untrackedcache_no_fsmonitor (new) ## -@@ ++ EOF ++ test_cmp ../expected ../actual && ++ rm -fr ../actual ../expected ++' ++ ++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 && ++ cat >../expected <<-EOF && +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. ++See '"'"'git help status'"'"' for information on how to improve this. + +nothing to commit (create/copy files and use "git add" to track) - - ## t/t7065/with_untrackedcache_with_fsmonitor (new) ## -@@ ++ EOF ++ test_cmp ../expected ../actual && ++ rm -fr ../actual ../expected ++' ++ ++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 && ++ cat >../expected <<-EOF && +On branch test + +No commits yet @@ t/t7065/with_untrackedcache_with_fsmonitor (new) + +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. ++See '"'"'git help status'"'"' for information on how to improve this. + +nothing to commit (create/copy files and use "git add" to track) ++ EOF ++ test_cmp ../expected ../actual && ++ rm -fr ../actual ../expected ++' ++ ++test_done ## wt-status.c ## @@ @@ wt-status.c: static void wt_longstatus_print(struct wt_status *s) 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, ++ if (uf_was_slow(s->untracked_in_ms) && 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, - _("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", ""); -+ } ++ _("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"), Documentation/git-status.txt | 55 +++++++++++++++++++++++++++ t/t7065-wtstatus-slow.sh | 74 ++++++++++++++++++++++++++++++++++++ wt-status.c | 32 +++++++++++++--- 3 files changed, 156 insertions(+), 5 deletions(-) create mode 100755 t/t7065-wtstatus-slow.sh diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 54a4b29b473..c51ba5e79e1 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -457,6 +457,61 @@ 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. + +* The `-uno` flag or the `status.showUntrackedfiles=false` + config : indicate that `git status` should not report untracked + files. 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` : this config option disables a + warning message when the search for untracked files takes longer + than desired. 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. 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. 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..468b8934836 --- /dev/null +++ b/t/t7065-wtstatus-slow.sh @@ -0,0 +1,74 @@ +#!/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 && + cat >../expected <<-EOF && +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) + EOF + test_cmp ../expected ../actual && + rm -fr ../actual ../expected +' + +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 && + cat >../expected <<-EOF && +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) + EOF + test_cmp ../expected ../actual && + rm -fr ../actual ../expected +' + +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 && + cat >../expected <<-EOF && +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) + EOF + test_cmp ../expected ../actual && + rm -fr ../actual ../expected +' + +test_done diff --git a/wt-status.c b/wt-status.c index 5813174896c..336f41e6d9f 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,21 @@ 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) { + if (uf_was_slow(s->untracked_in_ms) && 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, - _("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); + _("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"), base-commit: bbe21b64a08f89475d8a3818e20c111378daa621 -- gitgitgadget ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v4] status: long status advice adapted to recent capabilities 2022-11-10 4:46 ` [PATCH v4] " Rudy Rigot via GitGitGadget @ 2022-11-10 5:42 ` Eric Sunshine 2022-11-10 17:01 ` Rudy Rigot 2022-11-10 20:04 ` [PATCH v5] " Rudy Rigot via GitGitGadget 1 sibling, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-10 5:42 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget Cc: git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Rudy Rigot On Wed, Nov 9, 2022 at 11:46 PM Rudy Rigot via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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. > > `git status` can be slow when there are a large number of untracked > files and directories, because Git must search the entire worktree > to enumerate them. Previously, Git would print an advice message > with the elapsed search time and a suggestion to disable the search > using the `-uno` option. This suggestion also carried a warning > that might scare off some users. > > Git can reduce the size and time of the untracked file search when > the `core.untrackedCache` and `core.fsmonitor` features are enabled > by caching results from previous `git status` invocations. > > Update the advice to explain the various combinations of additional > configuration options and refer to (new) documentation in the man > page that explains it in more detail than what can be printed in an > advice message. > > Finally, add new tests to verify the new functionality. > > Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> > --- > diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt > @@ -457,6 +457,61 @@ during the write may conflict with other simultaneous processes, causing > +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 Not necessarily worth a reroll, but the "since we all work..." fragment could be dropped without any loss of clarity: There is no single optimum configuration suitable to every situation. would probably be sufficient. > +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. This leaves the reader hanging somewhat by not saying where "elsewhere" actually is. You can use gitlink: to insert links to the appropriate documentation. > +* The `-uno` flag or the `status.showUntrackedfiles=false` > + config : indicate that `git status` should not report untracked > + files. 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. This and all the other bullet points use an odd mix of spaces and TAB for indentation. They should consistently use one or the other. Earlier in this thread, Ævar suggested that it might be worthwhile to mention an additional possibility: that simply rerunning `git status` might be sufficient to achieve reasonable speed since the second run of `git status` may benefit from the filesystem cache having been primed by the first invocation of `git status`. As such, it may be overkill for a user to dive into the various options described here. Hence, to mitigate the possibility of a user doing a lot of research and extra unnecessary configuration, it might make sense for the very first bullet point (before this one) to say merely: * Do nothing. Subsequent invocations of `git status` may be faster simply because the first `git status` primed the filesystem cache. or something like that (probably more formal, though). > diff --git a/t/t7065-wtstatus-slow.sh b/t/t7065-wtstatus-slow.sh > @@ -0,0 +1,74 @@ > +#!/bin/sh > + > +test_description='test status when slow untracked files' > + > +. ./test-lib.sh > + > +DATA="$TEST_DIRECTORY/t7065" This variable is unused, isn't it, now that you're inlining everything with here-docs? > +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 && Rather than asserting that some preceding code has left the configuration in a state this test wants, it would be more robust for this test to forcibly set up the state it wants. Something like this should work: test_might_fail git config ---unset-all core.untrackedCache && test_might_fail git config ---unset-all core.fsmonitor && > + git status | sed "s/[0-9]\.[0-9][0-9]/X/g" >../actual && At all costs, avoid escaping the "trash" directory in which this test is running. All files created by this test should be within the "trash" directory hierarchy. Therefore, drop "../" from all the filenames, and instead create these files directly in the "trash" directory or in some subdirectory of the "trash" directory. I presume the reason you're escaping the "trash" directory is because you don't want these untracked "actual" and "expected" files to pollute the `git status` output you're testing? If so, then you should probably instead set up a .gitignore file in the "trash" directory to make `git status` ignore them. We usually want to avoid having a Git command upstream of a pipe since the exit code of the Git command will be lost. So, we'd typically do this instead: git status >out && sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && > + cat >../expected <<-EOF && > +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) > + EOF Two comments. First, use <<-\EOF rather than <<-EOF to make it clear to readers that you're not interpolating any variables in the here-doc body. Second, the <<- operator allows you to indent the here-doc body (with TABs, not spaces), so you can align the body with the rest of the code: cat >expected <<-\EOF && On branch test ... EOF > + test_cmp ../expected ../actual && > + rm -fr ../actual ../expected > +' We usually don't bother cleaning up these files if they don't impact subsequent tests negatively. However, note that if any code above the `rm -fr` command fails, then the `rm -fr` command itself won't be run, hence the files won't get cleaned up upon test failure. To ensure cleanup (if that's what you desire), use test_when_finished() _before_ code which may fail. So, something like this is typical: test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' test_when_finished "rm -fr actual expected" && test_might_fail git config ---unset-all core.untrackedCache && test_might_fail git config ---unset-all core.fsmonitor && ... ' Same comments apply to the other new tests added by this patch. > diff --git a/wt-status.c b/wt-status.c > @@ -1205,6 +1207,17 @@ static void wt_longstatus_print_tracking(struct wt_status *s) > +static inline int uf_was_slow(uint32_t untracked_in_ms) > +{ Does this need to be inline? Is this a hot piece of code, or is this merely a premature optimization? > + const char *x; > + x = getenv("GIT_TEST_UF_DELAY_WARNING"); > + if (x) { > + untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; > + } Style is to avoid { } braces for these one-liner bodies. I think we don't even need the variable "x" in this case, so: if (getenv("GIT_TEST_UF_DELAY_WARNING")) untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; would be cleaner. > + return UF_DELAY_WARNING_IN_MS < untracked_in_ms; > +} ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v4] status: long status advice adapted to recent capabilities 2022-11-10 5:42 ` Eric Sunshine @ 2022-11-10 17:01 ` Rudy Rigot 2022-11-10 17:30 ` Eric Sunshine 0 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot @ 2022-11-10 17:01 UTC (permalink / raw) To: Eric Sunshine Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee > the <<- operator allows you to indent the here-doc body > (with TABs, not spaces), so you can align the body with the rest of > the code Unfortunately, that's how I had done it first, but since some of those lines are blank, the test code had lines just made of "<tab><tab>" and nothing else, which made the check-whitespace check fail. I considered replacing empty line with something on the fly with sed (like just an "x" character for instance), but this felt hacky and brittle (in the unlikely case where an actual "x" would find itself genuinely lost in the middle of that output, the test would mistakenly pass). I went with the solution I'm presenting here because the readability downsides of missing that indentation felt less bad. Definitely willing to be convinced though. I have no issues with anything else from your review, and I'm planning to integrate it all. More specifically: > I presume the reason you're escaping the "trash" directory is because > you don't want these untracked "actual" and "expected" files to > pollute the `git status` output you're testing? You are presuming right! The test was being flappy in CI runs before I changed this, which I found used as a solution in other git-status-related tests currently in the codebase. I'm not familiar with the trash directory approach, but I'll figure it out. > Does this need to be inline? Is this a hot piece of code, or is this > merely a premature optimization? I'll admit my limits, I'm not familiar enough to know. If you feel the inline is unnecessary here, I'm glad to trust you on it, I'll remove it. Thanks a lot for your in-depth review, I am planning to integrate all the other feedback, and bring another iteration forward, possibly today. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v4] status: long status advice adapted to recent capabilities 2022-11-10 17:01 ` Rudy Rigot @ 2022-11-10 17:30 ` Eric Sunshine 2022-11-10 17:47 ` Rudy Rigot 0 siblings, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-10 17:30 UTC (permalink / raw) To: Rudy Rigot Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee On Thu, Nov 10, 2022 at 12:02 PM Rudy Rigot <rudy.rigot@gmail.com> wrote: > > the <<- operator allows you to indent the here-doc body > > (with TABs, not spaces), so you can align the body with the rest of > > the code > > Unfortunately, that's how I had done it first, but since some of those > lines are blank, the test code had lines just made of "<tab><tab>" and > nothing else, which made the check-whitespace check fail. I considered > replacing empty line with something on the fly with sed (like just an > "x" character for instance), but this felt hacky and brittle (in the > unlikely case where an actual "x" would find itself genuinely lost in > the middle of that output, the test would mistakenly pass). I went > with the solution I'm presenting here because the readability > downsides of missing that indentation felt less bad. Definitely > willing to be convinced though. Okay, I see what you're getting at. Fortunately, there is a simple solution as long as those lines are truly blank as emitted by `git status`: just leave the blank lines completely blank in the here-doc body (don't bother inserting a TAB on the blank line). This should product the exact output you want: cat >../expected <<-\EOF && On branch test No commits yet ... EOF Although it should not be needed here, the `sed` approach is generally fine, and we use it often enough in tests, though usually with a more uncommon letter such as "Q". See, for instance, the q_to_nul(), q_to_tab(), etc. functions in t/test-lib-functions.sh. > > I presume the reason you're escaping the "trash" directory is because > > you don't want these untracked "actual" and "expected" files to > > pollute the `git status` output you're testing? > > You are presuming right! The test was being flappy in CI runs before I > changed this, which I found used as a solution in other > git-status-related tests currently in the codebase. I'm not familiar > with the trash directory approach, but I'll figure it out. Each test script is run in a temporary "trash" directory which gets thrown away when the script finishes. We want tests to constrain themselves to the trash directory so they don't inadvertently destroy a user's files outside the directory. I see what you mean about some existing status-related tests using files such as "../actual" and "../expect". It's not at all obvious in a lot of those cases but they are safe[*] because those tests have already cd'd into a subdirectory of the "trash" directory, thus "../actual" is referring to the "trash" directory itself, hence the tests do constrain themselves to "trash". Anyhow, I suspect that crafting a custom .gitignore file in the test setup should satisfy this particular case and allow "actual" and "expected" to reside in the "trash" directory itself without mucking up `git status` output. [*] Unfortunately, some of those scripts are poorly structured because they `cd` around between tests, which can leave CWD in an unexpected state if some test fails and subsequent tests expect CWD to be somewhere other than where it was left by the failed test. These days, we only allow tests to `cd` within a subshell so that CWD is restored automatically whether the test itself succeeds or fails. So, this is safe: test_expect_success 'title' ' do something && mkdir foo && ( cd foo && do something else >../actual && ) grep foo actual ' ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v4] status: long status advice adapted to recent capabilities 2022-11-10 17:30 ` Eric Sunshine @ 2022-11-10 17:47 ` Rudy Rigot 0 siblings, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-11-10 17:47 UTC (permalink / raw) To: Eric Sunshine Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee Oh, thanks, all of this helps a ton. I know exactly what to do about those two things now. On Thu, Nov 10, 2022 at 11:30 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Thu, Nov 10, 2022 at 12:02 PM Rudy Rigot <rudy.rigot@gmail.com> wrote: > > > the <<- operator allows you to indent the here-doc body > > > (with TABs, not spaces), so you can align the body with the rest of > > > the code > > > > Unfortunately, that's how I had done it first, but since some of those > > lines are blank, the test code had lines just made of "<tab><tab>" and > > nothing else, which made the check-whitespace check fail. I considered > > replacing empty line with something on the fly with sed (like just an > > "x" character for instance), but this felt hacky and brittle (in the > > unlikely case where an actual "x" would find itself genuinely lost in > > the middle of that output, the test would mistakenly pass). I went > > with the solution I'm presenting here because the readability > > downsides of missing that indentation felt less bad. Definitely > > willing to be convinced though. > > Okay, I see what you're getting at. Fortunately, there is a simple > solution as long as those lines are truly blank as emitted by `git > status`: just leave the blank lines completely blank in the here-doc > body (don't bother inserting a TAB on the blank line). This should > product the exact output you want: > > cat >../expected <<-\EOF && > On branch test > > No commits yet > ... > EOF > > Although it should not be needed here, the `sed` approach is generally > fine, and we use it often enough in tests, though usually with a more > uncommon letter such as "Q". See, for instance, the q_to_nul(), > q_to_tab(), etc. functions in t/test-lib-functions.sh. > > > > I presume the reason you're escaping the "trash" directory is because > > > you don't want these untracked "actual" and "expected" files to > > > pollute the `git status` output you're testing? > > > > You are presuming right! The test was being flappy in CI runs before I > > changed this, which I found used as a solution in other > > git-status-related tests currently in the codebase. I'm not familiar > > with the trash directory approach, but I'll figure it out. > > Each test script is run in a temporary "trash" directory which gets > thrown away when the script finishes. We want tests to constrain > themselves to the trash directory so they don't inadvertently destroy > a user's files outside the directory. > > I see what you mean about some existing status-related tests using > files such as "../actual" and "../expect". It's not at all obvious in > a lot of those cases but they are safe[*] because those tests have > already cd'd into a subdirectory of the "trash" directory, thus > "../actual" is referring to the "trash" directory itself, hence the > tests do constrain themselves to "trash". > > Anyhow, I suspect that crafting a custom .gitignore file in the test > setup should satisfy this particular case and allow "actual" and > "expected" to reside in the "trash" directory itself without mucking > up `git status` output. > > [*] Unfortunately, some of those scripts are poorly structured because > they `cd` around between tests, which can leave CWD in an unexpected > state if some test fails and subsequent tests expect CWD to be > somewhere other than where it was left by the failed test. These days, > we only allow tests to `cd` within a subshell so that CWD is restored > automatically whether the test itself succeeds or fails. So, this is > safe: > > test_expect_success 'title' ' > do something && > mkdir foo && > ( > cd foo && > do something else >../actual && > ) > grep foo actual > ' ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v5] status: long status advice adapted to recent capabilities 2022-11-10 4:46 ` [PATCH v4] " Rudy Rigot via GitGitGadget 2022-11-10 5:42 ` Eric Sunshine @ 2022-11-10 20:04 ` Rudy Rigot via GitGitGadget 2022-11-15 16:39 ` Jeff Hostetler ` (2 more replies) 1 sibling, 3 replies; 58+ messages in thread From: Rudy Rigot via GitGitGadget @ 2022-11-10 20:04 UTC (permalink / raw) To: git Cc: Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine, Rudy Rigot, Rudy Rigot 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. `git status` can be slow when there are a large number of untracked files and directories, because Git must search the entire worktree to enumerate them. Previously, Git would print an advice message with the elapsed search time and a suggestion to disable the search using the `-uno` option. This suggestion also carried a warning that might scare off some users. Git can reduce the size and time of the untracked file search when the `core.untrackedCache` and `core.fsmonitor` features are enabled by caching results from previous `git status` invocations. Update the advice to explain the various combinations of additional configuration options and refer to (new) documentation in the man page that explains it in more detail than what can be printed in an advice message. Finally, add new tests to verify the new functionality. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> --- status: long status advice adapted to recent capabilities Here is version 5 for this patch. Changes since v4: * Test improvements (readability, isolation. ...) * Doc improvements (referencing other docs, adding advice to try again, ...) * Minor logic simplifications Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1384%2Frudyrigot%2Fadvice_statusFsmonitor-v5 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1384/rudyrigot/advice_statusFsmonitor-v5 Pull-Request: https://github.com/gitgitgadget/git/pull/1384 Range-diff vs v4: 1: 85b35882c02 ! 1: 8b1b9ee094f status: long status advice adapted to recent capabilities @@ Documentation/git-status.txt: during the write may conflict with other simultane +-------------------------------- + +`git status` can be very slow in large worktrees if/when it -+needs to search for untracked files and directories. There are ++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. -+ -+* The `-uno` flag or the `status.showUntrackedfiles=false` -+ config : indicate that `git status` should not report untracked ++Git commands. 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. ++ ++* First, you may want to run `git status` again. Your current ++ configuration may already be caching `git status` results, ++ so it could be faster on subsequent runs. ++ ++* The `--untracked-files=no` flag or the ++ `status.showUntrackedfiles=false` config (see above for both) : ++ indicate that `git status` should not report untracked + files. 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` : this config option disables a -+ warning message when the search for untracked files takes longer -+ than desired. 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. The untracked cache is stored in the ++* `advice.statusUoption=false` (see linkgit:git-config[1]) : ++ this config option disables a warning message when the search ++ for untracked files takes longer than desired. In some large ++ repositories, this message may appear frequently and not be a ++ helpful signal. ++ ++* `core.untrackedCache=true` (see linkgit:git-update-index[1]) : ++ 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. 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. While -+ the FSMonitor feature can be enabled without the untracked -+ cache, the benefits are greatly reduced in that case. ++ `core.fsmonitor=<hook_command_pathname>` (see ++ linkgit:git-update-index[1]) : 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. 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 @@ t/t7065-wtstatus-slow.sh (new) + +. ./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 ++ git checkout -b test && ++ echo "actual" >> .gitignore && ++ echo "expected" >> .gitignore && ++ echo "out" >> .gitignore && ++ git add .gitignore && ++ git commit -m "Add .gitignore" +' + +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 && -+ cat >../expected <<-EOF && -+On branch test -+ -+No commits yet -+ ++ test_might_fail git config --unset-all core.untrackedCache && ++ test_might_fail git config --unset-all core.fsmonitor && ++ git status >out && ++ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && ++ cat >expected <<-\EOF && ++ On branch test + -+It took X seconds to enumerate untracked files. -+See '"'"'git help status'"'"' for information on how to improve this. ++ 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) ++ nothing to commit, working tree clean + EOF -+ test_cmp ../expected ../actual && -+ rm -fr ../actual ../expected ++ test_cmp expected 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 && -+ cat >../expected <<-EOF && -+On branch test ++ test_might_fail git config --unset-all core.fsmonitor && ++ git status >out && ++ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && ++ cat >expected <<-\EOF && ++ 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. + -+ -+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) ++ nothing to commit, working tree clean + EOF -+ test_cmp ../expected ../actual && -+ rm -fr ../actual ../expected ++ test_cmp expected 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 && -+ cat >../expected <<-EOF && -+On branch test -+ -+No commits yet -+ ++ git status >out && ++ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && ++ cat >expected <<-\EOF && ++ On branch test + -+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. ++ 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) ++ nothing to commit, working tree clean + EOF -+ test_cmp ../expected ../actual && -+ rm -fr ../actual ../expected ++ test_cmp expected actual +' + +test_done @@ wt-status.c: static void wt_longstatus_print_tracking(struct wt_status *s) +static inline int uf_was_slow(uint32_t untracked_in_ms) +{ -+ const char *x; -+ x = getenv("GIT_TEST_UF_DELAY_WARNING"); -+ if (x) { ++ if (getenv("GIT_TEST_UF_DELAY_WARNING")) { + untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; + } + Documentation/git-status.txt | 59 +++++++++++++++++++++++++++++++ t/t7065-wtstatus-slow.sh | 68 ++++++++++++++++++++++++++++++++++++ wt-status.c | 30 +++++++++++++--- 3 files changed, 152 insertions(+), 5 deletions(-) create mode 100755 t/t7065-wtstatus-slow.sh diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 5e438a7fdc1..ed1ae3bd35c 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -457,6 +457,65 @@ 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. 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. + +* First, you may want to run `git status` again. Your current + configuration may already be caching `git status` results, + so it could be faster on subsequent runs. + +* The `--untracked-files=no` flag or the + `status.showUntrackedfiles=false` config (see above for both) : + indicate that `git status` should not report untracked + files. 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` (see linkgit:git-config[1]) : + this config option disables a warning message when the search + for untracked files takes longer than desired. In some large + repositories, this message may appear frequently and not be a + helpful signal. + +* `core.untrackedCache=true` (see linkgit:git-update-index[1]) : + 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. 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>` (see + linkgit:git-update-index[1]) : 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. 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..bc624756622 --- /dev/null +++ b/t/t7065-wtstatus-slow.sh @@ -0,0 +1,68 @@ +#!/bin/sh + +test_description='test status when slow untracked files' + +. ./test-lib.sh + +GIT_TEST_UF_DELAY_WARNING=1 +export GIT_TEST_UF_DELAY_WARNING + +test_expect_success setup ' + git checkout -b test && + echo "actual" >> .gitignore && + echo "expected" >> .gitignore && + echo "out" >> .gitignore && + git add .gitignore && + git commit -m "Add .gitignore" +' + +test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' + test_might_fail git config --unset-all core.untrackedCache && + test_might_fail git config --unset-all core.fsmonitor && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch test + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_expect_success 'when core.untrackedCache true, but not fsmonitor' ' + git config core.untrackedCache true && + test_might_fail git config --unset-all core.fsmonitor && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch test + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_expect_success 'when core.untrackedCache true, and fsmonitor' ' + git config core.untrackedCache true && + git config core.fsmonitor true && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch test + + 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, working tree clean + EOF + test_cmp expected actual +' + +test_done diff --git a/wt-status.c b/wt-status.c index 5813174896c..5093bf8c894 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,15 @@ static void wt_longstatus_print_tracking(struct wt_status *s) strbuf_release(&sb); } +static inline int uf_was_slow(uint32_t untracked_in_ms) +{ + if (getenv("GIT_TEST_UF_DELAY_WARNING")) { + 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 +1825,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 +1882,21 @@ 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) { + if (uf_was_slow(s->untracked_in_ms) && 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, - _("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); + _("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"), base-commit: 319605f8f00e402f3ea758a02c63534ff800a711 -- gitgitgadget ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v5] status: long status advice adapted to recent capabilities 2022-11-10 20:04 ` [PATCH v5] " Rudy Rigot via GitGitGadget @ 2022-11-15 16:39 ` Jeff Hostetler 2022-11-15 16:42 ` Rudy Rigot 2022-11-15 17:26 ` Eric Sunshine 2022-11-15 21:19 ` [PATCH v6] " Rudy Rigot via GitGitGadget 2 siblings, 1 reply; 58+ messages in thread From: Jeff Hostetler @ 2022-11-15 16:39 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget, git Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine, Rudy Rigot On 11/10/22 3:04 PM, 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. > > `git status` can be slow when there are a large number of untracked > files and directories, because Git must search the entire worktree > to enumerate them. Previously, Git would print an advice message > with the elapsed search time and a suggestion to disable the search > using the `-uno` option. This suggestion also carried a warning > that might scare off some users. > > Git can reduce the size and time of the untracked file search when > the `core.untrackedCache` and `core.fsmonitor` features are enabled > by caching results from previous `git status` invocations. > > Update the advice to explain the various combinations of additional > configuration options and refer to (new) documentation in the man > page that explains it in more detail than what can be printed in an > advice message. > > Finally, add new tests to verify the new functionality. > > Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> I think V5 looks good. Thanks for your persistence! Jeff ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v5] status: long status advice adapted to recent capabilities 2022-11-15 16:39 ` Jeff Hostetler @ 2022-11-15 16:42 ` Rudy Rigot 0 siblings, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-11-15 16:42 UTC (permalink / raw) To: Jeff Hostetler Cc: Rudy Rigot via GitGitGadget, git, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine > I think V5 has addressed all of the concerns. I confirm I don't think there was any concern expressed so far that I didn't find a way to address in some way. > Thanks for your persistence! My absolute pleasure! ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v5] status: long status advice adapted to recent capabilities 2022-11-10 20:04 ` [PATCH v5] " Rudy Rigot via GitGitGadget 2022-11-15 16:39 ` Jeff Hostetler @ 2022-11-15 17:26 ` Eric Sunshine 2022-11-15 17:45 ` Rudy Rigot 2022-11-15 21:19 ` [PATCH v6] " Rudy Rigot via GitGitGadget 2 siblings, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-15 17:26 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget Cc: git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Rudy Rigot On Thu, Nov 10, 2022 at 3:04 PM Rudy Rigot via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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. > > `git status` can be slow when there are a large number of untracked > files and directories, because Git must search the entire worktree > to enumerate them. Previously, Git would print an advice message > with the elapsed search time and a suggestion to disable the search > using the `-uno` option. This suggestion also carried a warning > that might scare off some users. > > Git can reduce the size and time of the untracked file search when > the `core.untrackedCache` and `core.fsmonitor` features are enabled > by caching results from previous `git status` invocations. > > Update the advice to explain the various combinations of additional > configuration options and refer to (new) documentation in the man > page that explains it in more detail than what can be printed in an > advice message. > > Finally, add new tests to verify the new functionality. > > Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> Mostly just some minor style-related comments below, but also a couple grammos in the new documentation, and a question or two... > diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt > @@ -457,6 +457,65 @@ during the write may conflict with other simultaneous processes, causing > +* `core.untrackedCache=true` (see linkgit:git-update-index[1]) : > + 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 s/file/files/ > + 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. The untracked cache is stored in the > + .git/index file. The reduced cost searching for untracked Might want backticks around the literal filename: `.git/index` Also: s/cost searching/cost of searching/ > + 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. > diff --git a/t/t7065-wtstatus-slow.sh b/t/t7065-wtstatus-slow.sh > @@ -0,0 +1,68 @@ > +test_expect_success setup ' > + git checkout -b test && > + echo "actual" >> .gitignore && > + echo "expected" >> .gitignore && > + echo "out" >> .gitignore && Style: drop space after redirection operator: echo "actual" >>.gitignore && echo "expected" >>.gitignore && echo "out" >>.gitignore && By the way, this is an excellent opportunity to use a here-doc as you're now doing elsewhere in the script: cat >.gitignore <<-\EOF && actual expected out EOF Do we want to anchor these .gitignore patterns to make it clear to future readers the precise expectations of these tests? cat >.gitignore <<-\EOF && /actual /expected /out EOF > + git add .gitignore && > + git commit -m "Add .gitignore" > +' > + > +test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' > + test_might_fail git config --unset-all core.untrackedCache && > + test_might_fail git config --unset-all core.fsmonitor && > + git status >out && > + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && > + cat >expected <<-\EOF && > + On branch test > + > + It took X seconds to enumerate untracked files. > + See '"'"'git help status'"'"' for information on how to improve this. > + > + nothing to commit, working tree clean > + EOF Style: We normally make the here-doc body indentation match the indentation of the command itself: cat >expected <<-\EOF && On branch test ... EOF > + test_cmp expected actual > +' > + > +test_expect_success 'when core.untrackedCache true, but not fsmonitor' ' > + git config core.untrackedCache true && It's perhaps not super important in this case since each subsequent test is setting up its configuration exactly as it wants it, but it is common elsewhere in the test scripts to use test_config() which automatically unsets the configuration when the test finishes: test_config core.untrackedCache true && I'm on the fence as to whether or not to use test_config() in this case, but it shouldn't hurt to do so. > + test_might_fail git config --unset-all core.fsmonitor && > + git status >out && > + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && > + cat >expected <<-\EOF && > + On branch test > + > + It took X seconds to enumerate untracked files. > + See '"'"'git help status'"'"' for information on how to improve this. > + > + nothing to commit, working tree clean > + EOF > + test_cmp expected actual > diff --git a/wt-status.c b/wt-status.c > @@ -1205,6 +1207,15 @@ static void wt_longstatus_print_tracking(struct wt_status *s) > +static inline int uf_was_slow(uint32_t untracked_in_ms) Does this need to be "inline"? > +{ > + if (getenv("GIT_TEST_UF_DELAY_WARNING")) { > + untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; > + } > + > + return UF_DELAY_WARNING_IN_MS < untracked_in_ms; > +} Style: if (getenv("GIT_TEST_UF_DELAY_WARNING")) untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; return UF_DELAY_WARNING_IN_MS < untracked_in_ms; > @@ -1870,13 +1882,21 @@ static void wt_longstatus_print(struct wt_status *s) > - if (advice_enabled(ADVICE_STATUS_U_OPTION) && 2000 < s->untracked_in_ms) { > + if (uf_was_slow(s->untracked_in_ms) && advice_enabled(ADVICE_STATUS_U_OPTION)) { Was there a specific reason you switched around the condition so it checks advice_enabled() _after_ checking for slowness? If so, the reason may not be obvious to future readers and might deserve mention in the commit message. If it was just a whim, then future readers might end up wondering if there was some reason which they are unable to figure out, in which case it would be better to retain the original ordering of conditions. > 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); To what does "this" refer? Is it this repository? Or something else? > + } 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.")); Okay. > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v5] status: long status advice adapted to recent capabilities 2022-11-15 17:26 ` Eric Sunshine @ 2022-11-15 17:45 ` Rudy Rigot 2022-11-15 18:06 ` Eric Sunshine 0 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot @ 2022-11-15 17:45 UTC (permalink / raw) To: Eric Sunshine Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee Thanks for the feedback, and I'll integrate it into a new patch, most likely today. > Does this need to be "inline"? Oops. Just as I said I don't think I left any feedback unaddressed. You did mention it last time, and it fell through the cracks. I'll fix it this time, sorry about that. > Was there a specific reason you switched around the condition Totally a whim. After several iterations, the code had changed enough that the original ordering was lost. I'll switch it back. > To what does "this" refer? Is it this repository? Or something else? Hah, good point. The accurate answer is "the status of currently existing files is being cached, and we'll watch what files changed after now so we only run things on those next time". Obviously that would be too verbose for the inexperienced user hitting this, really this line is here to convey "if you run it again, it's probably going to be faster". Here are some ideas: - "but this result is currently being cached." - "but git status results are currently being cached." (true but not perfectly accurate since index updates don't only happen on git status) - "but untracked files are currently being cached." (not completely accurate, I believe the index is updated for all files; the untracked files are only the interesting ones for this specific performance consideration) - "but the results were cached, and your next runs may be faster." I could use some guidance on what would make most sense here. I strongly feel like the user should know of it since that's been what's been confusing the users of our very large repo specifically when their git status is temporarily slow; but I don't have any opinions at all about the right phrasing. For now, I'm planning to use the latter bullet point in my next patch because it's the most explicit, but I'd be glad to apply someone else's take on this instead. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v5] status: long status advice adapted to recent capabilities 2022-11-15 17:45 ` Rudy Rigot @ 2022-11-15 18:06 ` Eric Sunshine 2022-11-15 18:08 ` Rudy Rigot 0 siblings, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-15 18:06 UTC (permalink / raw) To: Rudy Rigot Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee On Tue, Nov 15, 2022 at 12:46 PM Rudy Rigot <rudy.rigot@gmail.com> wrote: > Thanks for the feedback, and I'll integrate it into a new patch, most > likely today. Thanks. I think this is _almost_ there; much more polished than earlier iterations. Hopefully, one final reroll will make it complete. > > To what does "this" refer? Is it this repository? Or something else? > > Hah, good point. The accurate answer is "the status of currently > existing files is being cached, and we'll watch what files changed > after now so we only run things on those next time". Obviously that > would be too verbose for the inexperienced user hitting this, really > this line is here to convey "if you run it again, it's probably going > to be faster". > > Here are some ideas: > - "but this result is currently being cached." > - "but git status results are currently being cached." (true but not > perfectly accurate since index updates don't only happen on git > status) > - "but untracked files are currently being cached." (not completely > accurate, I believe the index is updated for all files; the untracked > files are only the interesting ones for this specific performance > consideration) > - "but the results were cached, and your next runs may be faster." > > I could use some guidance on what would make most sense here. I > strongly feel like the user should know of it since that's been what's > been confusing the users of our very large repo specifically when > their git status is temporarily slow; but I don't have any opinions at > all about the right phrasing. For now, I'm planning to use the latter > bullet point in my next patch because it's the most explicit, but I'd > be glad to apply someone else's take on this instead. Reading the proposals while wearing the hat of someone who has never had to deal with speeding up untracked-file bookkeeping and who may not even know that remedies are available (as discussed in the documentation), I find that the first three bullet points convey no meaning at all; they leave the reader hanging. The final bullet point, on the other hand, tells the user something conclusive. I _very_ much prefer the final proposal. (In "but the results were cached, and your next runs may be faster.", I might suggest dropping "your" -- i.e. "... and subsequent runs may be faster".) ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v5] status: long status advice adapted to recent capabilities 2022-11-15 18:06 ` Eric Sunshine @ 2022-11-15 18:08 ` Rudy Rigot 0 siblings, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-11-15 18:08 UTC (permalink / raw) To: Eric Sunshine Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee That makes a ton of sense, I agree with your thinking. Thanks for that, I'll do exactly that. ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-10 20:04 ` [PATCH v5] " Rudy Rigot via GitGitGadget 2022-11-15 16:39 ` Jeff Hostetler 2022-11-15 17:26 ` Eric Sunshine @ 2022-11-15 21:19 ` Rudy Rigot via GitGitGadget 2022-11-21 5:06 ` Eric Sunshine 2022-11-22 16:59 ` [PATCH v7] status: modernize git-status "slow untracked files" advice Rudy Rigot via GitGitGadget 2 siblings, 2 replies; 58+ messages in thread From: Rudy Rigot via GitGitGadget @ 2022-11-15 21:19 UTC (permalink / raw) To: git Cc: Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine, Rudy Rigot, Rudy Rigot 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. `git status` can be slow when there are a large number of untracked files and directories, because Git must search the entire worktree to enumerate them. Previously, Git would print an advice message with the elapsed search time and a suggestion to disable the search using the `-uno` option. This suggestion also carried a warning that might scare off some users. Git can reduce the size and time of the untracked file search when the `core.untrackedCache` and `core.fsmonitor` features are enabled by caching results from previous `git status` invocations. Update the advice to explain the various combinations of additional configuration options and refer to (new) documentation in the man page that explains it in more detail than what can be printed in an advice message. Finally, add new tests to verify the new functionality. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> --- status: long status advice adapted to recent capabilities Here is version 6 for this patch. Changes since v5: * End of sentence for fsmonitor case was changed to "but the results were cached, and subsequent runs may be faster." * Except for that, mostly style and doc fixes. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1384%2Frudyrigot%2Fadvice_statusFsmonitor-v6 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1384/rudyrigot/advice_statusFsmonitor-v6 Pull-Request: https://github.com/gitgitgadget/git/pull/1384 Range-diff vs v5: 1: 8b1b9ee094f ! 1: ff3aa0e01c0 status: long status advice adapted to recent capabilities @@ Documentation/git-status.txt: during the write may conflict with other simultane + 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 ++ the set of untracked files 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. The untracked cache is stored in the -+ .git/index file. The reduced cost searching for untracked ++ `.git/index` file. The reduced cost of 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. @@ t/t7065-wtstatus-slow.sh (new) + +test_expect_success setup ' + git checkout -b test && -+ echo "actual" >> .gitignore && -+ echo "expected" >> .gitignore && -+ echo "out" >> .gitignore && ++ cat >.gitignore <<-\EOF && ++ /actual ++ /expected ++ /out ++ EOF + git add .gitignore && + git commit -m "Add .gitignore" +' @@ t/t7065-wtstatus-slow.sh (new) + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && -+ On branch test ++ On branch test + -+ It took X seconds to enumerate untracked files. -+ See '"'"'git help status'"'"' for information on how to improve this. ++ It took X seconds to enumerate untracked files. ++ See '"'"'git help status'"'"' for information on how to improve this. + -+ nothing to commit, working tree clean ++ nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_expect_success 'when core.untrackedCache true, but not fsmonitor' ' -+ git config core.untrackedCache true && ++ test_config core.untrackedCache true && + test_might_fail git config --unset-all core.fsmonitor && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && -+ On branch test ++ On branch test + -+ It took X seconds to enumerate untracked files. -+ See '"'"'git help status'"'"' for information on how to improve this. ++ It took X seconds to enumerate untracked files. ++ See '"'"'git help status'"'"' for information on how to improve this. + -+ nothing to commit, working tree clean ++ nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_expect_success 'when core.untrackedCache true, and fsmonitor' ' -+ git config core.untrackedCache true && -+ git config core.fsmonitor true && ++ test_config core.untrackedCache true && ++ test_config core.fsmonitor true && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && -+ On branch test ++ On branch test + -+ 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. ++ It took X seconds to enumerate untracked files, ++ but the results were cached, and subsequent runs may be faster. ++ See '"'"'git help status'"'"' for information on how to improve this. + -+ nothing to commit, working tree clean ++ nothing to commit, working tree clean + EOF + test_cmp expected actual +' @@ wt-status.c: static void wt_longstatus_print_tracking(struct wt_status *s) strbuf_release(&sb); } -+static inline int uf_was_slow(uint32_t untracked_in_ms) ++static int uf_was_slow(uint32_t untracked_in_ms) +{ -+ if (getenv("GIT_TEST_UF_DELAY_WARNING")) { ++ if (getenv("GIT_TEST_UF_DELAY_WARNING")) + untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; -+ } -+ + return UF_DELAY_WARNING_IN_MS < untracked_in_ms; +} + @@ wt-status.c: static void wt_longstatus_print(struct wt_status *s) 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) { -+ if (uf_was_slow(s->untracked_in_ms) && advice_enabled(ADVICE_STATUS_U_OPTION)) { ++ if (advice_enabled(ADVICE_STATUS_U_OPTION) && uf_was_slow(s->untracked_in_ms)) { 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."), ++ "but the results were cached, and subsequent runs may be faster."), + s->untracked_in_ms / 1000.0); + } else { + status_printf_ln(s, GIT_COLOR_NORMAL, Documentation/git-status.txt | 59 ++++++++++++++++++++++++++++++ t/t7065-wtstatus-slow.sh | 70 ++++++++++++++++++++++++++++++++++++ wt-status.c | 28 ++++++++++++--- 3 files changed, 152 insertions(+), 5 deletions(-) create mode 100755 t/t7065-wtstatus-slow.sh diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 5e438a7fdc1..570c36e07c1 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -457,6 +457,65 @@ 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. 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. + +* First, you may want to run `git status` again. Your current + configuration may already be caching `git status` results, + so it could be faster on subsequent runs. + +* The `--untracked-files=no` flag or the + `status.showUntrackedfiles=false` config (see above for both) : + indicate that `git status` should not report untracked + files. 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` (see linkgit:git-config[1]) : + this config option disables a warning message when the search + for untracked files takes longer than desired. In some large + repositories, this message may appear frequently and not be a + helpful signal. + +* `core.untrackedCache=true` (see linkgit:git-update-index[1]) : + 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 files 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. The untracked cache is stored in the + `.git/index` file. The reduced cost of 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>` (see + linkgit:git-update-index[1]) : 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. 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..8d08a962f88 --- /dev/null +++ b/t/t7065-wtstatus-slow.sh @@ -0,0 +1,70 @@ +#!/bin/sh + +test_description='test status when slow untracked files' + +. ./test-lib.sh + +GIT_TEST_UF_DELAY_WARNING=1 +export GIT_TEST_UF_DELAY_WARNING + +test_expect_success setup ' + git checkout -b test && + cat >.gitignore <<-\EOF && + /actual + /expected + /out + EOF + git add .gitignore && + git commit -m "Add .gitignore" +' + +test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' + test_might_fail git config --unset-all core.untrackedCache && + test_might_fail git config --unset-all core.fsmonitor && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch test + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_expect_success 'when core.untrackedCache true, but not fsmonitor' ' + test_config core.untrackedCache true && + test_might_fail git config --unset-all core.fsmonitor && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch test + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_expect_success 'when core.untrackedCache true, and fsmonitor' ' + test_config core.untrackedCache true && + test_config core.fsmonitor true && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch test + + It took X seconds to enumerate untracked files, + but the results were cached, and subsequent runs may be faster. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_done diff --git a/wt-status.c b/wt-status.c index 5813174896c..1f6d64e759f 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,13 @@ static void wt_longstatus_print_tracking(struct wt_status *s) strbuf_release(&sb); } +static int uf_was_slow(uint32_t untracked_in_ms) +{ + if (getenv("GIT_TEST_UF_DELAY_WARNING")) + 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 +1823,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 +1880,21 @@ 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) { + if (advice_enabled(ADVICE_STATUS_U_OPTION) && uf_was_slow(s->untracked_in_ms)) { 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 the results were cached, and subsequent runs may be faster."), + 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, - _("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); + _("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"), base-commit: 319605f8f00e402f3ea758a02c63534ff800a711 -- gitgitgadget ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-15 21:19 ` [PATCH v6] " Rudy Rigot via GitGitGadget @ 2022-11-21 5:06 ` Eric Sunshine 2022-11-21 15:54 ` Rudy Rigot 2022-11-22 16:59 ` [PATCH v7] status: modernize git-status "slow untracked files" advice Rudy Rigot via GitGitGadget 1 sibling, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-21 5:06 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget Cc: git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Rudy Rigot On Tue, Nov 15, 2022 at 4:19 PM Rudy Rigot via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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. > > Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> > --- > Here is version 6 for this patch. > > Changes since v5: > > * End of sentence for fsmonitor case was changed to "but the results > were cached, and subsequent runs may be faster." > * Except for that, mostly style and doc fixes. Thanks, I think v6 addresses all my earlier review comments. Just one or two notes below... > +test_expect_success setup ' > + git checkout -b test && > + cat >.gitignore <<-\EOF && > + /actual > + /expected > + /out > + EOF > + git add .gitignore && > + git commit -m "Add .gitignore" > +' I suppose I wasn't paying close enough attention earlier, but I see now that this is creating a branch named "test". If I remove the `git checkout -b test` line entirely and change "test" to "master" in the "expected" files, all the tests pass just fine. So, it's not apparent why you need to create a specially-named branch here rather than simply accepting the default branch name. The reason I bring this up is that unnecessary code, whether in tests or elsewhere, can confuse future readers (and reviewers, such as myself) into wondering if something subtle is going on which the reader is overlooking. (This is very much the same sort of concern as when I asked in an earlier review why the conditions in an `if` got switched around from the way they were in the original code; such unnecessary changes can confuse future readers.) If the answer is that you wanted to avoid the term "master", then an alternative would have been to override the default branch name at the top of the script: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME That said, this is minor, and I'm not keen on eating up more of your time or reviewer time, so I doubt this is worth a reroll. > +test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' > + test_might_fail git config --unset-all core.untrackedCache && > + test_might_fail git config --unset-all core.fsmonitor && When I suggested the above code, it had slipped my mind that we have a test_unconfig() function in t/test-lib-functions.sh which does this more concisely: test_unconfig core.untrackedCache && test_unconfig core.fsmonitor && But what you have here in v6 is good enough; it's not worth wasting your time or reviewer time rerolling just to make this change. I mention it merely for future reference. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-21 5:06 ` Eric Sunshine @ 2022-11-21 15:54 ` Rudy Rigot 2022-11-21 16:17 ` Eric Sunshine 0 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot @ 2022-11-21 15:54 UTC (permalink / raw) To: Eric Sunshine Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee > That said, this is minor, and I'm not keen on eating up more of your > time or reviewer time, so I doubt this is worth a reroll. Eh, there's nothing wrong with striving for perfection. Lemme do one more reroll... > So, it's not apparent > why you need to create a specially-named branch here rather than > simply accepting the default branch name. The reason was that it failed some CI pipelines before I did this, with some pipelines printing "main" instead of "master" into the git status output. I fixed it right away, so I don't know if it was a CI glitch that day or if it would still be the same running it now. I could have redacted the branch name away from the output, but it seemed simpler and more readable to just set the branch name in stone for all pipelines. > an alternative would have been to override the default branch name at the > top of the script: Oh, this seems like a better way to do what I was trying to do. I'll change it now. > we have a test_unconfig() function I'll use that. New patch coming! ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-21 15:54 ` Rudy Rigot @ 2022-11-21 16:17 ` Eric Sunshine 2022-11-22 16:52 ` Rudy Rigot 0 siblings, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-21 16:17 UTC (permalink / raw) To: Rudy Rigot Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee On Mon, Nov 21, 2022 at 10:55 AM Rudy Rigot <rudy.rigot@gmail.com> wrote: > > That said, this is minor, and I'm not keen on eating up more of your > > time or reviewer time, so I doubt this is worth a reroll. > > Eh, there's nothing wrong with striving for perfection. Lemme do one > more reroll... Reviewer time is a scarce resource on the mailing list these days, which is why I'm hesitant to see rerolls for minor or subjective changes. However, if you're going to reroll anyhow, I have a couple more things to say... (below) > > So, it's not apparent > > why you need to create a specially-named branch here rather than > > simply accepting the default branch name. > > The reason was that it failed some CI pipelines before I did this, > with some pipelines printing "main" instead of "master" into the git > status output. I fixed it right away, so I don't know if it was a CI > glitch that day or if it would still be the same running it now. I > could have redacted the branch name away from the output, but it > seemed simpler and more readable to just set the branch name in stone > for all pipelines. Most likely it wasn't a glitch, but rather (I'd guess) that Windows CI uses "main" already, whereas Unix CI's still use "master". > > an alternative would have been to override the default branch name at the > > top of the script: > > Oh, this seems like a better way to do what I was trying to do. I'll > change it now. > > > we have a test_unconfig() function > > I'll use that. > > New patch coming! If you're going to reroll, then I'll mention a couple more things which I held back before since I want to use reviewer time wisely. Nevertheless... First, having the commit message explain the problem first and then the solution is more reviewer-friendly, not the solution and then the problem as this patch is doing. Additionally, the commit message should be written in imperative mood. Documentation/SubmittingPatches has a good discussion of these points. It's also typically unnecessary for the commit message to say that the patch is adding new tests; reviewers assume that you will do so when appropriate, and the patch itself shows plainly enough that you did. Taking these points into consideration, you might write the commit message like this: status: modernize git-status "slow untracked files" advice `git status` can be slow when there are a large number of untracked files and directories since Git must search the entire worktree to enumerate them. When it is too slow, Git prints advice with the elapsed search time and a suggestion to disable the search using the `-uno` option. This suggestion also carries a warning that might scare off some users. However, these days, `-uno` isn't the only option. Git can reduce the size and time of the untracked file search when the `core.untrackedCache` and `core.fsmonitor` features are enabled by caching results from previous `git status` invocations. Therefore, update the `git status` man page to explain the various configuration options, and update the advice to provide more detail about the current configuration and to refer to the updated documentation. Second, we usually don't want to waste a test script number (such as "t7065") if we can avoid it, especially for so few tests and such minor functionality. So, if there is an existing test script in which these new tests might fit, it's better to add them to that script instead, usually at the end of the script. (I haven't checked, but maybe you can find an existing script which would be a good fit; if not, then placing them in a new standalone script, as the patch is already doing, may be okay.) ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-21 16:17 ` Eric Sunshine @ 2022-11-22 16:52 ` Rudy Rigot 2022-11-22 17:18 ` Eric Sunshine 0 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot @ 2022-11-22 16:52 UTC (permalink / raw) To: Eric Sunshine Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee > Most likely it wasn't a glitch, but rather (I'd guess) that Windows CI > uses "main" already, whereas Unix CI's still use "master". My intuition was that it was something like that indeed. > you might write the commit message like this The current phrasing was initially copied as is from a past review feedback; I have no issues at all replacing it with yours. > So, if there is an existing test script in which > these new tests might fit, it's better to add them to that script > instead Oh, sorry about that, it hadn't occurred to me that there could be a downside to using new test script numbers. I'm 100% on board with the thinking, but I'm struggling quite a bit to implement it. There are several existing test scripts where these new tests would fit very well semantically (t7060-wtstatus.sh,c, t7512-status-help.sh, t7519-status-fsmonitor.sh, ...), and I spent quite some time yesterday trying to move the 3 news tests to those. For some reason, test_cmp is not giving me a diff anymore when working in those script files, so I feel in the dark about what the tests are failing about, and I'm stumped about what to try next. What I mean: for instance, if I introduce an intentional mistake in the test and run './t7065-wtstatus-slow.sh -v', I get this section that clarifies what the issue is: --- expected 2022-11-21 23:46:00.000000000 +0000 +++ actual 2022-11-21 23:46:00.000000000 +0000 @@ -1,4 +1,4 @@ -On branch maine +On branch main Here is a gist https://gist.github.com/rudyrigot/b31fcb6384e829ca7586818758e48d0b, with: - the patch as I currently have it on t7508-status.sh (it's a bit longer than it was, without the isolation in a separate script I've had to do a few things to mitigate the side effects from other tests in the script) - the end of what I get when running 'sh ./t7508-status.sh -v': https://gist.github.com/rudyrigot/ee80f3d59231f25698c9dd6c48d8ab85. It seems like 2 of my 3 tests are failing, but the output isn't very helpful to figure out why. Would you (or someone else) have pointers to help me get through this one? I'm tempted to throw in the towel, since it sounded like it wasn't too huge a deal if this lived in its separate script file, and that other people's bandwidth (which I'm aware is what I'm requesting here) is an even more scarce resource. So I'll submit a new patch with everything else but this, so there's the option to still proceed with it if that's the most sensible path forward. But I have to admit I'm quite frustrated that I couldn't figure this last one out by myself, so I'm more than happy to dig more into it, if anyone has guidance. Thanks a lot. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-22 16:52 ` Rudy Rigot @ 2022-11-22 17:18 ` Eric Sunshine 2022-11-22 17:24 ` Eric Sunshine 2022-11-22 17:40 ` Eric Sunshine 0 siblings, 2 replies; 58+ messages in thread From: Eric Sunshine @ 2022-11-22 17:18 UTC (permalink / raw) To: Rudy Rigot Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee On Tue, Nov 22, 2022 at 11:52 AM Rudy Rigot <rudy.rigot@gmail.com> wrote: > > you might write the commit message like this > > The current phrasing was initially copied as is from a past review > feedback; I have no issues at all replacing it with yours. Apparently, I overlooked that when scanning backward through the thread. > > So, if there is an existing test script in which > > these new tests might fit, it's better to add them to that script > > instead > > Oh, sorry about that, it hadn't occurred to me that there could be a > downside to using new test script numbers. No need to apologize. Reviewers understand that there is a lot to absorb and figure out. > I'm 100% on board with the thinking, but I'm struggling quite a bit to > implement it. There are several existing test scripts where these new > tests would fit very well semantically (t7060-wtstatus.sh,c, > t7512-status-help.sh, t7519-status-fsmonitor.sh, ...), and I spent > quite some time yesterday trying to move the 3 news tests to those. > For some reason, test_cmp is not giving me a diff anymore when working > in those script files, so I feel in the dark about what the tests are > failing about, and I'm stumped about what to try next. > > Here is a gist https://gist.github.com/rudyrigot/b31fcb6384e829ca7586818758e48d0b, > with: > > - the patch as I currently have it on t7508-status.sh (it's a bit > longer than it was, without the isolation in a separate script I've > had to do a few things to mitigate the side effects from other tests > in the script) I probably should have thought to warn about possible interaction with earlier tests when I made the suggestion to place the tests in an existing script. Nevertheless, we should be able to sidestep all that "isolation goop" you had to add to the tests... (more below). > - the end of what I get when running 'sh ./t7508-status.sh -v': > https://gist.github.com/rudyrigot/ee80f3d59231f25698c9dd6c48d8ab85. It > seems like 2 of my 3 tests are failing, but the output isn't very > helpful to figure out why. > > Would you (or someone else) have pointers to help me get through this one? The second 'gist' URL seems broken, so I can't comment on the exact output. Without having seen the actual problem, I can't really provide direct feedback for fixing the precise issue, however... We actually don't want you to pollute your new tests with goop to clean up after earlier tests in the script since that just introduces a bunch of code in your tests which is not directly relevant to what your tests are checking. So, perhaps the cleanest way to approach this is to have your "setup" test create a new repository in the trash directory and then just run your remaining new tests in that repository. That way, your tests don't have to deal with any gunk from earlier tests. This basically means taking your tests pretty much as you had them in v6, but with a little extra boilerplate. Something like this: test_expect_success setup ' git init slowstatus && ( cd slowstatus && cat >.gitignore <<-\EOF && /actual /expected /out EOF git add .gitignore && git commit -m "Add .gitignore" ) ' test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' ( cd slowstatus && test_unconfig core.untrackedCache && test_unconfig core.fsmonitor && ... ) ' and so on. > I'm tempted to throw in the towel, since it sounded like it wasn't too > huge a deal if this lived in its separate script file, and that other > people's bandwidth (which I'm aware is what I'm requesting here) is an > even more scarce resource. My comment about possibly leaving them in a separate script if you couldn't find a suitable existing script was just general advice. I can't speak for Junio and thus can't say what he will or will not accept in his tree. > So I'll submit a new patch with everything > else but this, so there's the option to still proceed with it if > that's the most sensible path forward. But I have to admit I'm quite > frustrated that I couldn't figure this last one out by myself, so I'm > more than happy to dig more into it, if anyone has guidance. Perhaps the suggestion I outlined above will help. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-22 17:18 ` Eric Sunshine @ 2022-11-22 17:24 ` Eric Sunshine 2022-11-22 17:29 ` Rudy Rigot 2022-11-22 17:40 ` Eric Sunshine 1 sibling, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-22 17:24 UTC (permalink / raw) To: Rudy Rigot Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee On Tue, Nov 22, 2022 at 12:18 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > On Tue, Nov 22, 2022 at 11:52 AM Rudy Rigot <rudy.rigot@gmail.com> wrote: > > > you might write the commit message like this > > - the end of what I get when running 'sh ./t7508-status.sh -v': > > https://gist.github.com/rudyrigot/ee80f3d59231f25698c9dd6c48d8ab85. It > > seems like 2 of my 3 tests are failing, but the output isn't very > > helpful to figure out why. > > The second 'gist' URL seems broken, so I can't comment on the exact > output. Without having seen the actual problem, I can't really provide > direct feedback for fixing the precise issue, however... Despite the URL of the second git being broken, I notice that you have some "-v" output tacked onto the first gist. Indeed, the "-v" output isn't very helpful here. What I normally do when debugging failing tests is run with "-x" and "-i". The "-x" shows all the output the test produces, including any error messages. So "./t7508-status.sh -x -i" would likely make it easier to diagnose such failures. (But first try the suggestion I made in my previous reply for isolating the new tests in their own repository.) ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-22 17:24 ` Eric Sunshine @ 2022-11-22 17:29 ` Rudy Rigot 0 siblings, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-11-22 17:29 UTC (permalink / raw) To: Eric Sunshine Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee Oops, here is the only Gist URL I had meant to share, sorry about the confusion, the other one was a copy-paste mistake: https://gist.github.com/rudyrigot/b31fcb6384e829ca7586818758e48d0b But the suggestions you just gave me are wonderfully helpful! Thanks a lot for that, let me give those a try now... ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-22 17:18 ` Eric Sunshine 2022-11-22 17:24 ` Eric Sunshine @ 2022-11-22 17:40 ` Eric Sunshine 2022-11-22 18:07 ` Eric Sunshine 1 sibling, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-22 17:40 UTC (permalink / raw) To: Rudy Rigot Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee On Tue, Nov 22, 2022 at 12:18 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > This basically means taking your tests pretty much as > you had them in v6, but with a little extra boilerplate. Something > like this: > > test_expect_success setup ' > git init slowstatus && > ( > cd slowstatus && > cat >.gitignore <<-\EOF && > /actual > /expected > /out > EOF > git add .gitignore && > git commit -m "Add .gitignore" > ) > ' A minor additional comment if you do go this route and place the new tests in an existing script... Although "setup" was fine as a title in a standalone script, when adding the new tests to an existing script, you'd probably want to choose a more meaningful name. Perhaps "setup slow untracked-cache status" or something. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-22 17:40 ` Eric Sunshine @ 2022-11-22 18:07 ` Eric Sunshine 2022-11-22 19:19 ` Rudy Rigot 0 siblings, 1 reply; 58+ messages in thread From: Eric Sunshine @ 2022-11-22 18:07 UTC (permalink / raw) To: Rudy Rigot Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee On Tue, Nov 22, 2022 at 12:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > A minor additional comment if you do go this route and place the new > tests in an existing script... And one more comment... By placing: GIT_TEST_UF_DELAY_WARNING=1 export GIT_TEST_UF_DELAY_WARNING at the top of the existing script into which you add the new tests, we have to worry about potential side-effects in other tests in the scripts. Better would be to place these lines just above the new tests, so that the effects are better isolated. However, even better than that would be to isolate the environment variable to exactly the point it is needed. For instance: test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' test_unconfig core.untrackedCache && test_unconfig core.fsmonitor && GIT_TEST_UF_DELAY_WARNING=1 git status >out && ... ' ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-22 18:07 ` Eric Sunshine @ 2022-11-22 19:19 ` Rudy Rigot 2022-11-22 19:48 ` Eric Sunshine 0 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot @ 2022-11-22 19:19 UTC (permalink / raw) To: Eric Sunshine Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee Holy snap it worked! Patch coming as soon as CI confirms it's good everywhere. I believe I was able to integrate all of your advice. Thanks a lot for your guidance! ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v6] status: long status advice adapted to recent capabilities 2022-11-22 19:19 ` Rudy Rigot @ 2022-11-22 19:48 ` Eric Sunshine 0 siblings, 0 replies; 58+ messages in thread From: Eric Sunshine @ 2022-11-22 19:48 UTC (permalink / raw) To: Rudy Rigot Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee On Tue, Nov 22, 2022 at 2:19 PM Rudy Rigot <rudy.rigot@gmail.com> wrote: > Holy snap it worked! Patch coming as soon as CI confirms it's good > everywhere. I believe I was able to integrate all of your advice. > Thanks a lot for your guidance! Glad to hear that it all worked out. ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v7] status: modernize git-status "slow untracked files" advice 2022-11-15 21:19 ` [PATCH v6] " Rudy Rigot via GitGitGadget 2022-11-21 5:06 ` Eric Sunshine @ 2022-11-22 16:59 ` Rudy Rigot via GitGitGadget 2022-11-22 22:07 ` [PATCH v8] " Rudy Rigot via GitGitGadget 1 sibling, 1 reply; 58+ messages in thread From: Rudy Rigot via GitGitGadget @ 2022-11-22 16:59 UTC (permalink / raw) To: git Cc: Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine, Rudy Rigot, Rudy Rigot From: Rudy Rigot <rudy.rigot@gmail.com> `git status` can be slow when there are a large number of untracked files and directories since Git must search the entire worktree to enumerate them. When it is too slow, Git prints advice with the elapsed search time and a suggestion to disable the search using the `-uno` option. This suggestion also carries a warning that might scare off some users. However, these days, `-uno` isn't the only option. Git can reduce the size and time of the untracked file search when the `core.untrackedCache` and `core.fsmonitor` features are enabled by caching results from previous `git status` invocations. Therefore, update the `git status` man page to explain the various configuration options, and update the advice to provide more detail about the current configuration and to refer to the updated documentation. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> --- status: modernize git-status "slow untracked files" advice Here is version 7 for this patch. Changes since v6: * Readability improvements in tests * Rewrite of the commit message Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1384%2Frudyrigot%2Fadvice_statusFsmonitor-v7 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1384/rudyrigot/advice_statusFsmonitor-v7 Pull-Request: https://github.com/gitgitgadget/git/pull/1384 Range-diff vs v6: 1: ff3aa0e01c0 ! 1: 871a9becbdf status: long status advice adapted to recent capabilities @@ Metadata Author: Rudy Rigot <rudy.rigot@gmail.com> ## Commit message ## - status: long status advice adapted to recent capabilities + status: modernize git-status "slow untracked files" advice - 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. + `git status` can be slow when there are a large number of + untracked files and directories since Git must search the entire + worktree to enumerate them. When it is too slow, Git prints + advice with the elapsed search time and a suggestion to disable + the search using the `-uno` option. This suggestion also carries + a warning that might scare off some users. - `git status` can be slow when there are a large number of untracked - files and directories, because Git must search the entire worktree - to enumerate them. Previously, Git would print an advice message - with the elapsed search time and a suggestion to disable the search - using the `-uno` option. This suggestion also carried a warning - that might scare off some users. + However, these days, `-uno` isn't the only option. Git can reduce + the size and time of the untracked file search when the + `core.untrackedCache` and `core.fsmonitor` features are enabled by + caching results from previous `git status` invocations. - Git can reduce the size and time of the untracked file search when - the `core.untrackedCache` and `core.fsmonitor` features are enabled - by caching results from previous `git status` invocations. - - Update the advice to explain the various combinations of additional - configuration options and refer to (new) documentation in the man - page that explains it in more detail than what can be printed in an - advice message. - - Finally, add new tests to verify the new functionality. + Therefore, update the `git status` man page to explain the various + configuration options, and update the advice to provide more + detail about the current configuration and to refer to the updated + documentation. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> @@ t/t7065-wtstatus-slow.sh (new) + +test_description='test status when slow untracked files' + -+. ./test-lib.sh ++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main ++export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +GIT_TEST_UF_DELAY_WARNING=1 +export GIT_TEST_UF_DELAY_WARNING + ++. ./test-lib.sh ++ +test_expect_success setup ' -+ git checkout -b test && + cat >.gitignore <<-\EOF && + /actual + /expected @@ t/t7065-wtstatus-slow.sh (new) +' + +test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' -+ test_might_fail git config --unset-all core.untrackedCache && -+ test_might_fail git config --unset-all core.fsmonitor && ++ test_unconfig core.untrackedCache && ++ test_unconfig core.fsmonitor && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && -+ On branch test ++ On branch main + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. @@ t/t7065-wtstatus-slow.sh (new) + +test_expect_success 'when core.untrackedCache true, but not fsmonitor' ' + test_config core.untrackedCache true && -+ test_might_fail git config --unset-all core.fsmonitor && ++ test_unconfig core.fsmonitor && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && -+ On branch test ++ On branch main + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. @@ t/t7065-wtstatus-slow.sh (new) + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && -+ On branch test ++ On branch main + + It took X seconds to enumerate untracked files, + but the results were cached, and subsequent runs may be faster. Documentation/git-status.txt | 59 +++++++++++++++++++++++++++++ t/t7065-wtstatus-slow.sh | 72 ++++++++++++++++++++++++++++++++++++ wt-status.c | 28 +++++++++++--- 3 files changed, 154 insertions(+), 5 deletions(-) create mode 100755 t/t7065-wtstatus-slow.sh diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 5e438a7fdc1..570c36e07c1 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -457,6 +457,65 @@ 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. 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. + +* First, you may want to run `git status` again. Your current + configuration may already be caching `git status` results, + so it could be faster on subsequent runs. + +* The `--untracked-files=no` flag or the + `status.showUntrackedfiles=false` config (see above for both) : + indicate that `git status` should not report untracked + files. 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` (see linkgit:git-config[1]) : + this config option disables a warning message when the search + for untracked files takes longer than desired. In some large + repositories, this message may appear frequently and not be a + helpful signal. + +* `core.untrackedCache=true` (see linkgit:git-update-index[1]) : + 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 files 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. The untracked cache is stored in the + `.git/index` file. The reduced cost of 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>` (see + linkgit:git-update-index[1]) : 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. 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..6733e8ba36b --- /dev/null +++ b/t/t7065-wtstatus-slow.sh @@ -0,0 +1,72 @@ +#!/bin/sh + +test_description='test status when slow untracked files' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +GIT_TEST_UF_DELAY_WARNING=1 +export GIT_TEST_UF_DELAY_WARNING + +. ./test-lib.sh + +test_expect_success setup ' + cat >.gitignore <<-\EOF && + /actual + /expected + /out + EOF + git add .gitignore && + git commit -m "Add .gitignore" +' + +test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' + test_unconfig core.untrackedCache && + test_unconfig core.fsmonitor && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch main + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_expect_success 'when core.untrackedCache true, but not fsmonitor' ' + test_config core.untrackedCache true && + test_unconfig core.fsmonitor && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch main + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_expect_success 'when core.untrackedCache true, and fsmonitor' ' + test_config core.untrackedCache true && + test_config core.fsmonitor true && + git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch main + + It took X seconds to enumerate untracked files, + but the results were cached, and subsequent runs may be faster. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual +' + +test_done diff --git a/wt-status.c b/wt-status.c index 5813174896c..1f6d64e759f 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,13 @@ static void wt_longstatus_print_tracking(struct wt_status *s) strbuf_release(&sb); } +static int uf_was_slow(uint32_t untracked_in_ms) +{ + if (getenv("GIT_TEST_UF_DELAY_WARNING")) + 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 +1823,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 +1880,21 @@ 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) { + if (advice_enabled(ADVICE_STATUS_U_OPTION) && uf_was_slow(s->untracked_in_ms)) { 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 the results were cached, and subsequent runs may be faster."), + 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, - _("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); + _("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"), base-commit: 319605f8f00e402f3ea758a02c63534ff800a711 -- gitgitgadget ^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v8] status: modernize git-status "slow untracked files" advice 2022-11-22 16:59 ` [PATCH v7] status: modernize git-status "slow untracked files" advice Rudy Rigot via GitGitGadget @ 2022-11-22 22:07 ` Rudy Rigot via GitGitGadget 2022-11-25 4:58 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 58+ messages in thread From: Rudy Rigot via GitGitGadget @ 2022-11-22 22:07 UTC (permalink / raw) To: git Cc: Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine, Rudy Rigot, Rudy Rigot From: Rudy Rigot <rudy.rigot@gmail.com> `git status` can be slow when there are a large number of untracked files and directories since Git must search the entire worktree to enumerate them. When it is too slow, Git prints advice with the elapsed search time and a suggestion to disable the search using the `-uno` option. This suggestion also carries a warning that might scare off some users. However, these days, `-uno` isn't the only option. Git can reduce the size and time of the untracked file search when the `core.untrackedCache` and `core.fsmonitor` features are enabled by caching results from previous `git status` invocations. Therefore, update the `git status` man page to explain the various configuration options, and update the advice to provide more detail about the current configuration and to refer to the updated documentation. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> --- status: modernize git-status "slow untracked files" advice Here is version 8 for this patch. Changes since v7: * Moved tests from new test script to existing one, in order not to needlessly waste a test script number for such a small feature. Two caveats: * The use of test_config in a subshell result in: 'error: bug in the test script: test_when_finished does nothing in a subshell', so I've had to resort to using plain old git config instead. * A test higher in the script is doing git config --global advice.statusuoption false, so I now have to set it locally in my isolated test repo. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1384%2Frudyrigot%2Fadvice_statusFsmonitor-v8 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1384/rudyrigot/advice_statusFsmonitor-v8 Pull-Request: https://github.com/gitgitgadget/git/pull/1384 Range-diff vs v7: 1: 871a9becbdf ! 1: 16e3721515b status: modernize git-status "slow untracked files" advice @@ Documentation/git-status.txt: during the write may conflict with other simultane -------- linkgit:gitignore[5] - ## t/t7065-wtstatus-slow.sh (new) ## -@@ -+#!/bin/sh -+ -+test_description='test status when slow untracked files' -+ -+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main -+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME -+ -+GIT_TEST_UF_DELAY_WARNING=1 -+export GIT_TEST_UF_DELAY_WARNING -+ -+. ./test-lib.sh -+ -+test_expect_success setup ' -+ cat >.gitignore <<-\EOF && -+ /actual -+ /expected -+ /out -+ EOF -+ git add .gitignore && -+ git commit -m "Add .gitignore" + ## t/t7508-status.sh ## +@@ t/t7508-status.sh: test_expect_success 'racy timestamps will be fixed for dirty worktree' ' + ! test_is_magic_mtime .git/index + ' + ++test_expect_success 'setup slow status advice' ' ++ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main git init slowstatus && ++ ( ++ cd slowstatus && ++ cat >.gitignore <<-\EOF && ++ /actual ++ /expected ++ /out ++ EOF ++ git add .gitignore && ++ git commit -m "Add .gitignore" && ++ git config advice.statusuoption true ++ ) +' + -+test_expect_success 'when core.untrackedCache and fsmonitor are unset' ' -+ test_unconfig core.untrackedCache && -+ test_unconfig core.fsmonitor && -+ git status >out && -+ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && -+ cat >expected <<-\EOF && -+ On branch main -+ -+ It took X seconds to enumerate untracked files. -+ See '"'"'git help status'"'"' for information on how to improve this. -+ -+ nothing to commit, working tree clean -+ EOF -+ test_cmp expected actual ++test_expect_success 'slow status advice when core.untrackedCache and fsmonitor are unset' ' ++ ( ++ cd slowstatus && ++ git config core.untrackedCache false && ++ git config core.fsmonitor false && ++ GIT_TEST_UF_DELAY_WARNING=1 git status >out && ++ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && ++ cat >expected <<-\EOF && ++ On branch main ++ ++ It took X seconds to enumerate untracked files. ++ See '"'"'git help status'"'"' for information on how to improve this. ++ ++ nothing to commit, working tree clean ++ EOF ++ test_cmp expected actual ++ ) +' + -+test_expect_success 'when core.untrackedCache true, but not fsmonitor' ' -+ test_config core.untrackedCache true && -+ test_unconfig core.fsmonitor && -+ git status >out && -+ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && -+ cat >expected <<-\EOF && -+ On branch main -+ -+ It took X seconds to enumerate untracked files. -+ See '"'"'git help status'"'"' for information on how to improve this. -+ -+ nothing to commit, working tree clean -+ EOF -+ test_cmp expected actual ++test_expect_success 'slow status advice when core.untrackedCache true, but not fsmonitor' ' ++ ( ++ cd slowstatus && ++ git config core.untrackedCache true && ++ git config core.fsmonitor false && ++ GIT_TEST_UF_DELAY_WARNING=1 git status >out && ++ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && ++ cat >expected <<-\EOF && ++ On branch main ++ ++ It took X seconds to enumerate untracked files. ++ See '"'"'git help status'"'"' for information on how to improve this. ++ ++ nothing to commit, working tree clean ++ EOF ++ test_cmp expected actual ++ ) +' + -+test_expect_success 'when core.untrackedCache true, and fsmonitor' ' -+ test_config core.untrackedCache true && -+ test_config core.fsmonitor true && -+ git status >out && -+ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && -+ cat >expected <<-\EOF && -+ On branch main -+ -+ It took X seconds to enumerate untracked files, -+ but the results were cached, and subsequent runs may be faster. -+ See '"'"'git help status'"'"' for information on how to improve this. -+ -+ nothing to commit, working tree clean -+ EOF -+ test_cmp expected actual ++test_expect_success 'slow status advice when core.untrackedCache true, and fsmonitor' ' ++ ( ++ cd slowstatus && ++ git config core.untrackedCache true && ++ git config core.fsmonitor true && ++ GIT_TEST_UF_DELAY_WARNING=1 git status >out && ++ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && ++ cat >expected <<-\EOF && ++ On branch main ++ ++ It took X seconds to enumerate untracked files, ++ but the results were cached, and subsequent runs may be faster. ++ See '"'"'git help status'"'"' for information on how to improve this. ++ ++ nothing to commit, working tree clean ++ EOF ++ test_cmp expected actual ++ ) +' + -+test_done + test_done ## wt-status.c ## @@ Documentation/git-status.txt | 59 +++++++++++++++++++++++++++++ t/t7508-status.sh | 73 ++++++++++++++++++++++++++++++++++++ wt-status.c | 28 +++++++++++--- 3 files changed, 155 insertions(+), 5 deletions(-) diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 5e438a7fdc1..570c36e07c1 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -457,6 +457,65 @@ 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. 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. + +* First, you may want to run `git status` again. Your current + configuration may already be caching `git status` results, + so it could be faster on subsequent runs. + +* The `--untracked-files=no` flag or the + `status.showUntrackedfiles=false` config (see above for both) : + indicate that `git status` should not report untracked + files. 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` (see linkgit:git-config[1]) : + this config option disables a warning message when the search + for untracked files takes longer than desired. In some large + repositories, this message may appear frequently and not be a + helpful signal. + +* `core.untrackedCache=true` (see linkgit:git-update-index[1]) : + 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 files 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. The untracked cache is stored in the + `.git/index` file. The reduced cost of 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>` (see + linkgit:git-update-index[1]) : 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. 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/t7508-status.sh b/t/t7508-status.sh index 2b7ef6c41a4..02d641f0413 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -1676,4 +1676,77 @@ test_expect_success 'racy timestamps will be fixed for dirty worktree' ' ! test_is_magic_mtime .git/index ' +test_expect_success 'setup slow status advice' ' + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main git init slowstatus && + ( + cd slowstatus && + cat >.gitignore <<-\EOF && + /actual + /expected + /out + EOF + git add .gitignore && + git commit -m "Add .gitignore" && + git config advice.statusuoption true + ) +' + +test_expect_success 'slow status advice when core.untrackedCache and fsmonitor are unset' ' + ( + cd slowstatus && + git config core.untrackedCache false && + git config core.fsmonitor false && + GIT_TEST_UF_DELAY_WARNING=1 git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch main + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual + ) +' + +test_expect_success 'slow status advice when core.untrackedCache true, but not fsmonitor' ' + ( + cd slowstatus && + git config core.untrackedCache true && + git config core.fsmonitor false && + GIT_TEST_UF_DELAY_WARNING=1 git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch main + + It took X seconds to enumerate untracked files. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual + ) +' + +test_expect_success 'slow status advice when core.untrackedCache true, and fsmonitor' ' + ( + cd slowstatus && + git config core.untrackedCache true && + git config core.fsmonitor true && + GIT_TEST_UF_DELAY_WARNING=1 git status >out && + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && + cat >expected <<-\EOF && + On branch main + + It took X seconds to enumerate untracked files, + but the results were cached, and subsequent runs may be faster. + See '"'"'git help status'"'"' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual + ) +' + test_done diff --git a/wt-status.c b/wt-status.c index 5813174896c..1f6d64e759f 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,13 @@ static void wt_longstatus_print_tracking(struct wt_status *s) strbuf_release(&sb); } +static int uf_was_slow(uint32_t untracked_in_ms) +{ + if (getenv("GIT_TEST_UF_DELAY_WARNING")) + 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 +1823,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 +1880,21 @@ 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) { + if (advice_enabled(ADVICE_STATUS_U_OPTION) && uf_was_slow(s->untracked_in_ms)) { 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 the results were cached, and subsequent runs may be faster."), + 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, - _("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); + _("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"), base-commit: 319605f8f00e402f3ea758a02c63534ff800a711 -- gitgitgadget ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v8] status: modernize git-status "slow untracked files" advice 2022-11-22 22:07 ` [PATCH v8] " Rudy Rigot via GitGitGadget @ 2022-11-25 4:58 ` Junio C Hamano 2022-11-29 15:21 ` Rudy Rigot 2022-11-30 0:52 ` [PATCH v9] " Rudy Rigot via GitGitGadget 2023-05-11 5:17 ` [PATCH v8] " Eric Sunshine 2 siblings, 1 reply; 58+ messages in thread From: Junio C Hamano @ 2022-11-25 4:58 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget Cc: git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine, Rudy Rigot "Rudy Rigot via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Rudy Rigot <rudy.rigot@gmail.com> > > `git status` can be slow when there are a large number of > untracked files and directories since Git must search the entire > worktree to enumerate them. When it is too slow, Git prints > advice with the elapsed search time and a suggestion to disable > the search using the `-uno` option. This suggestion also carries > a warning that might scare off some users. > > However, these days, `-uno` isn't the only option. Git can reduce > the size and time of the untracked file search when the "time" I can sort of understand ("can reduce the time taken to enumerate untracked files" is how I may phrase it, though), but what did you want to say with "size"? > `core.untrackedCache` and `core.fsmonitor` features are enabled by > caching results from previous `git status` invocations. > > Therefore, update the `git status` man page to explain the various > configuration options, and update the advice to provide more ... Lose "Therefore, "; the resulting text would be much easier to follow. > +UNTRACKED FILES AND STATUS SPEED "STATUS SPEED" somehow does not sound quite grammatical. Perhaps "untracked files and performance" or something instead? > +-------------------------------- > + > +`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. 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. Good. > +* First, you may want to run `git status` again. Your current > + configuration may already be caching `git status` results, > + so it could be faster on subsequent runs. The above may be a good advice, but it is misleading to make it as if it is another alternative of equal footing with everything else listed. It may likely make the resulting text much easier to follow if you fold it into "Here is a summary", perhaps like... ... right for everyone. We'll list a summary of the relevant options to help you, but before going into the list, you may want to run `git status` again, because your configuration may already be ... > +* The `--untracked-files=no` flag or the > + `status.showUntrackedfiles=false` config (see above for both) : Lose the SP before the ":" (applies to all other entries, too). > + indicate that `git status` should not report untracked > + files. 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. OK. > +* `advice.statusUoption=false` (see linkgit:git-config[1]) : > + this config option disables a warning message when the search > + for untracked files takes longer than desired. In some large > + repositories, this message may appear frequently and not be a > + helpful signal. This is not technically wrong per-se, except that "desired" in "takes longer than desired" may simply be wrong. The reason why the message may not be a "helpful signal" is in such a repository and project the user may have already accepted the current trade-off as _desirable_, iow, the user is WILLING to wait for 2 seconds. And in such a case, it indeed is the most sensible option to disable the advice. We should also stress the fact that this has nothing to do with speeding up, unlike other pieces of advice you are giving here. It's not like disabling the advice will allow us to omit something we need to do to compute the advice (in other words, if the overhead to measure the time taken to list untracked files is large, this may matter, but that is hardly the case). Perhaps Setting this variable to `false` disables the warning message given when enumerating untracked files takes more than 2 seconds. In a large project, it may take longer and the user may have already accepted the trade off (e.g. using "-uno" may not be an acceptable option for the user), in which case, there is no point issuing the warning message, and in such a case, disabling the warning may be the best. or something like that. > +test_expect_success 'setup slow status advice' ' > + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main git init slowstatus && > + ( > + cd slowstatus && > + cat >.gitignore <<-\EOF && > + /actual > + /expected > + /out > + EOF > + git add .gitignore && > + git commit -m "Add .gitignore" && > + git config advice.statusuoption true > + ) > +' > + > +test_expect_success 'slow status advice when core.untrackedCache and fsmonitor are unset' ' > + ( > + cd slowstatus && > + git config core.untrackedCache false && > + git config core.fsmonitor false && > + GIT_TEST_UF_DELAY_WARNING=1 git status >out && > + sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && What if it takes more than 10 seconds, e.g. "It took 92.34 seconds to enumerate..." Wouldn't it be redacted into "It took 9X seconds to enumerate"? It probably does not happen, only because you are forcing the code to pretend that it took 2.001 seconds or something, I suspect. But if you are forcing with GIT_TEST_UF_DELAY_WARNING to pretend that it took some unacceptably long time, it may be more robust to * pass "struct wt_status *s" to uf_was_slow(), instead of passing s->untracked_in_ms * when GIT_TEST_UF_DETAIL_WARNING tells us we are pretending a long delay for the purpose of running tests, ASSIGN a known value to s->untracked_in_ms * get rid of "out" and use of "sed" in these test, and instead check for exact output. e.g. static int uf_was_slow(struct wt_status *s) { if (getenv("GIT_TEST_UF_DETAIL_WARNING")) s->untracked_in_ms = 3.25; return UF_DELAY_WARNING_IN_MS < s->untracked_in_ms; } plus GIT_TEST_UF_DETAIL_WARNING=1 git status >actual && cat >expect <<-\EOF && ... It took 3.25 seconds to enumerate ... EOF test_cmp expect actual Also, what do you need /g modifier in "sed" script for? I do not think we give more than one such number in the message we are testing. > + cat >expected <<-\EOF && > + On branch main > + > + It took X seconds to enumerate untracked files. > + See '"'"'git help status'"'"' for information on how to improve this. This is not wrong per-se, but it is more customary to do say: See '\''git help status'\'' for information on ... All of the comments for this test apply to other two new tests. > + nothing to commit, working tree clean > + EOF > + test_cmp expected actual > + ) > +' Additionally (read: you do not _have_ to do this to make this topic acceptable, but it probably is worth thinking about), if we need to introduce a new helper function uf_was_slow() anyway, a much better change may be to make the 2 seconds cut-off configurable, than inventing GIT_TEST_UF_DETAIL_WARNING used only for tests. You can introduce, say, "status.enumerateUntrackedDelayMS", a configuration variable that can be set to override the hardcoded 2000 milliseconds (i.e. UF_DELAY_WARNING_IN_MS) to control what delay is acceptable for the repository. Then you can run the tests with the configuration set to a negative value (i.e. no time is acceptably short, even 0 milliseconds). If you go that route, then you do need to redirect to "out" and redact with "sed" (make sure you are prepared to see a delay more than 10 seconds in such a case). Thanks. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v8] status: modernize git-status "slow untracked files" advice 2022-11-25 4:58 ` Junio C Hamano @ 2022-11-29 15:21 ` Rudy Rigot 2022-11-30 0:51 ` Rudy Rigot 0 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot @ 2022-11-29 15:21 UTC (permalink / raw) To: Junio C Hamano Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine Thanks for the amazing feedback, and sorry for the delay. > "time" I can sort of understand ("can reduce the time taken to > enumerate untracked files" is how I may phrase it, though), but > what did you want to say with "size"? This bit was provided by a past reviewer so I hope I don't misrepresent it, but I think the idea was to convey that the reason it's faster is because the part of the codebase it will do active work on is smaller. I don't think it provides compellingly more information for end users, compared to just mentioning time, so I'll simplify with your proposal here. > Additionally (read: you do not _have_ to do this to make this topic > acceptable, but it probably is worth thinking about), if we need to > introduce a new helper function uf_was_slow() anyway, a much better > change may be to make the 2 seconds cut-off configurable, than > inventing GIT_TEST_UF_DETAIL_WARNING used only for tests. I agree it would be an improvement, I'm going to try to do this. This doesn't feel like a much more involved change compared to your alternative suggestion of setting a hardcoded test value for `s->untracked_in_ms`, so it feels like there's not much to lose from doing it this way, while users would gain some nice configurability. For transparency, my intuition is that I'm not sure there will be use cases where the config will be meaningfully leveraged by users. My gut is that the current cut-off time is an arbitrary UX perception cut-off, so I'm not sure it would need to be different depending on given repo situations. But I'm also very aware that I could be wrong, and this could open use cases that I'm not thinking about. And to your point, it would be sensible to use it as a test input anyway, so we might as well make it a user-facing tweak; so, I'm on board. > Wouldn't it be redacted into "It took 9X seconds to enumerate"? > It probably does not happen, only because you are forcing the code > to pretend that it took 2.001 seconds or something, I suspect. Yup, you guessed right. I'll be sure to change the regular expression to be resilient to double-digit times. > Also, what do you need /g modifier in "sed" script for? I do not > think we give more than one such number in the message we are > testing. Indeed, it's not useful to anything, and shouldn't be there, I'll fix this too. All of the other comments are crystal clear, and I intend to implement them exactly as advised. You can expect a new patch this week, maybe even today. Thanks a lot again! ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v8] status: modernize git-status "slow untracked files" advice 2022-11-29 15:21 ` Rudy Rigot @ 2022-11-30 0:51 ` Rudy Rigot 0 siblings, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-11-30 0:51 UTC (permalink / raw) To: Junio C Hamano Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine Alright, I tried the "status.enumerateUntrackedDelayMS" approach, but I couldn't pull it off and now I am stumped. This is somewhat frustrating, so I'd welcome guidance if anyone has time and is interested. Since this doesn't actually work, I don't think I should create an actual patch for it on the mailing list, so here are two other ways to show what I've got, I hope they're acceptable: - in Gist form: https://gist.github.com/rudyrigot/aa3e8e5ddb4f71fdc7fc0e92d9b7a4b8 - in GitHub compare form: https://github.com/git/git/compare/master...rudyrigot:git:status_enumerateUntrackedDelayMS The issues I'm seeing: - No matter how I set the config from the test, it doesn't seem to have any effect. I'm thinking I might be doing something wrong in how I set the value, which I've done in git_status_config in builtin/commit.c, which very well may be the wrong place. - Therefore, I've been testing things by changing the default value in wt_status_prepare in wt-status.c. Setting it at 0 and making the operator <= instead of < makes the advice display, which tells me that the logic is sound. Setting at its intended value of 2000 doesn't display the advice message, as expected. But setting it at -1 also doesn't display it. I'm a bit puzzled about why that would be, and I'm wondering: maybe the int is unsigned? It doesn't look like it based on how the structure field is declared in wt-status.h, but I know my own limits in C so I could be wrong. Now, I'm also well aware that Junio raised that advice leaving the door wide open to not actually solve this as part of this patch; and I did express in my previous reply that I am not intuitively convinced there is much value to it for users, although I could be wrong of course. So with that, if it's better to let it go, that is fine by me too. With that in mind, I implemented the alternative that Junio was proposing instead (assigning the value of `s->untracked_in_ms`), and it seems to work all good. It just passed CI, so I'm about to submit that as a patch, with every other piece of feedback also addressed. Unrelated note: I noticed that the first 2 bits of feedback applied to docs that were part of past patches, but were removed in the last patch. The rest of the doc feedback was current, so I was able to implement them, but obviously I couldn't implement the first 2 ones, since the issues they're about are gone. ^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v9] status: modernize git-status "slow untracked files" advice 2022-11-22 22:07 ` [PATCH v8] " Rudy Rigot via GitGitGadget 2022-11-25 4:58 ` Junio C Hamano @ 2022-11-30 0:52 ` Rudy Rigot via GitGitGadget 2022-12-01 6:48 ` Junio C Hamano 2023-05-11 5:17 ` [PATCH v8] " Eric Sunshine 2 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot via GitGitGadget @ 2022-11-30 0:52 UTC (permalink / raw) To: git Cc: Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine, Rudy Rigot, Rudy Rigot From: Rudy Rigot <rudy.rigot@gmail.com> `git status` can be slow when there are a large number of untracked files and directories since Git must search the entire worktree to enumerate them. When it is too slow, Git prints advice with the elapsed search time and a suggestion to disable the search using the `-uno` option. This suggestion also carries a warning that might scare off some users. However, these days, `-uno` isn't the only option. Git can reduce the size and time of the untracked file search when the `core.untrackedCache` and `core.fsmonitor` features are enabled by caching results from previous `git status` invocations. Therefore, update the `git status` man page to explain the various configuration options, and update the advice to provide more detail about the current configuration and to refer to the updated documentation. Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> --- status: modernize git-status "slow untracked files" advice Here is version 9 for this patch. Changes since v8: * Improved tests. * The untracked files delay measured is now set to always the same value in test cases. That has allowed to remove all sed calls from tests. * Improved documentation. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1384%2Frudyrigot%2Fadvice_statusFsmonitor-v9 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1384/rudyrigot/advice_statusFsmonitor-v9 Pull-Request: https://github.com/gitgitgadget/git/pull/1384 Range-diff vs v8: 1: 16e3721515b ! 1: fcb298e6e5a status: modernize git-status "slow untracked files" advice @@ Documentation/git-status.txt: during the write may conflict with other simultane 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 -+-------------------------------- ++UNTRACKED FILES AND PERFORMANCE ++------------------------------- + +`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. 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. -+ -+* First, you may want to run `git status` again. Your current -+ configuration may already be caching `git status` results, -+ so it could be faster on subsequent runs. ++for everyone. We'll list a summary of the relevant options to help ++you, but before going into the list, you may want to run `git status` ++again, because your configuration may already be caching `git status` ++results, so it could be faster on subsequent runs. + +* The `--untracked-files=no` flag or the -+ `status.showUntrackedfiles=false` config (see above for both) : ++ `status.showUntrackedfiles=false` config (see above for both): + indicate that `git status` should not report untracked + files. 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` (see linkgit:git-config[1]) : -+ this config option disables a warning message when the search -+ for untracked files takes longer than desired. In some large -+ repositories, this message may appear frequently and not be a -+ helpful signal. ++* `advice.statusUoption=false` (see linkgit:git-config[1]): ++ setting this variable to `false` disables the warning message ++ given when enumerating untracked files takes more than 2 ++ seconds. In a large project, it may take longer and the user ++ may have already accepted the trade off (e.g. using "-uno" may ++ not be an acceptable option for the user), in which case, there ++ is no point issuing the warning message, and in such a case, ++ disabling the warning may be the best. + -+* `core.untrackedCache=true` (see linkgit:git-update-index[1]) : ++* `core.untrackedCache=true` (see linkgit:git-update-index[1]): + 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 @@ Documentation/git-status.txt: during the write may conflict with other simultane + +* `core.untrackedCache=true` and `core.fsmonitor=true` or + `core.fsmonitor=<hook_command_pathname>` (see -+ linkgit:git-update-index[1]) : enable both the untracked cache ++ linkgit:git-update-index[1]): 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 @@ t/t7508-status.sh: test_expect_success 'racy timestamps will be fixed for dirty + cd slowstatus && + git config core.untrackedCache false && + git config core.fsmonitor false && -+ GIT_TEST_UF_DELAY_WARNING=1 git status >out && -+ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && ++ GIT_TEST_UF_DELAY_WARNING=1 git status >actual && + cat >expected <<-\EOF && + On branch main + -+ It took X seconds to enumerate untracked files. -+ See '"'"'git help status'"'"' for information on how to improve this. ++ It took 3.25 seconds to enumerate untracked files. ++ See '\''git help status'\'' for information on how to improve this. + + nothing to commit, working tree clean + EOF @@ t/t7508-status.sh: test_expect_success 'racy timestamps will be fixed for dirty + cd slowstatus && + git config core.untrackedCache true && + git config core.fsmonitor false && -+ GIT_TEST_UF_DELAY_WARNING=1 git status >out && -+ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && ++ GIT_TEST_UF_DELAY_WARNING=1 git status >actual && + cat >expected <<-\EOF && + On branch main + -+ It took X seconds to enumerate untracked files. -+ See '"'"'git help status'"'"' for information on how to improve this. ++ It took 3.25 seconds to enumerate untracked files. ++ See '\''git help status'\'' for information on how to improve this. + + nothing to commit, working tree clean + EOF @@ t/t7508-status.sh: test_expect_success 'racy timestamps will be fixed for dirty + cd slowstatus && + git config core.untrackedCache true && + git config core.fsmonitor true && -+ GIT_TEST_UF_DELAY_WARNING=1 git status >out && -+ sed "s/[0-9]\.[0-9][0-9]/X/g" out >actual && ++ GIT_TEST_UF_DELAY_WARNING=1 git status >actual && + cat >expected <<-\EOF && + On branch main + -+ It took X seconds to enumerate untracked files, ++ It took 3.25 seconds to enumerate untracked files, + but the results were cached, and subsequent runs may be faster. -+ See '"'"'git help status'"'"' for information on how to improve this. ++ See '\''git help status'\'' for information on how to improve this. + + nothing to commit, working tree clean + EOF @@ wt-status.c: static void wt_longstatus_print_tracking(struct wt_status *s) strbuf_release(&sb); } -+static int uf_was_slow(uint32_t untracked_in_ms) ++static int uf_was_slow(struct wt_status *s) +{ + if (getenv("GIT_TEST_UF_DELAY_WARNING")) -+ untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1; -+ return UF_DELAY_WARNING_IN_MS < untracked_in_ms; ++ s->untracked_in_ms = 3250; ++ return UF_DELAY_WARNING_IN_MS < s->untracked_in_ms; +} + static void show_merge_in_progress(struct wt_status *s, @@ wt-status.c: static void wt_longstatus_print(struct wt_status *s) 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) { -+ if (advice_enabled(ADVICE_STATUS_U_OPTION) && uf_was_slow(s->untracked_in_ms)) { ++ if (advice_enabled(ADVICE_STATUS_U_OPTION) && uf_was_slow(s)) { status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); + if (fsm_mode > FSMONITOR_MODE_DISABLED) { + status_printf_ln(s, GIT_COLOR_NORMAL, Documentation/git-status.txt | 60 +++++++++++++++++++++++++++++++ t/t7508-status.sh | 70 ++++++++++++++++++++++++++++++++++++ wt-status.c | 28 ++++++++++++--- 3 files changed, 153 insertions(+), 5 deletions(-) diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt index 5e438a7fdc1..a051b1e8f38 100644 --- a/Documentation/git-status.txt +++ b/Documentation/git-status.txt @@ -457,6 +457,66 @@ 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 PERFORMANCE +------------------------------- + +`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. There is no single optimum set of settings right +for everyone. We'll list a summary of the relevant options to help +you, but before going into the list, you may want to run `git status` +again, because your configuration may already be caching `git status` +results, so it could be faster on subsequent runs. + +* The `--untracked-files=no` flag or the + `status.showUntrackedfiles=false` config (see above for both): + indicate that `git status` should not report untracked + files. 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` (see linkgit:git-config[1]): + setting this variable to `false` disables the warning message + given when enumerating untracked files takes more than 2 + seconds. In a large project, it may take longer and the user + may have already accepted the trade off (e.g. using "-uno" may + not be an acceptable option for the user), in which case, there + is no point issuing the warning message, and in such a case, + disabling the warning may be the best. + +* `core.untrackedCache=true` (see linkgit:git-update-index[1]): + 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 files 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. The untracked cache is stored in the + `.git/index` file. The reduced cost of 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>` (see + linkgit:git-update-index[1]): 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. 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/t7508-status.sh b/t/t7508-status.sh index 2b7ef6c41a4..aed07c5b622 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -1676,4 +1676,74 @@ test_expect_success 'racy timestamps will be fixed for dirty worktree' ' ! test_is_magic_mtime .git/index ' +test_expect_success 'setup slow status advice' ' + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main git init slowstatus && + ( + cd slowstatus && + cat >.gitignore <<-\EOF && + /actual + /expected + /out + EOF + git add .gitignore && + git commit -m "Add .gitignore" && + git config advice.statusuoption true + ) +' + +test_expect_success 'slow status advice when core.untrackedCache and fsmonitor are unset' ' + ( + cd slowstatus && + git config core.untrackedCache false && + git config core.fsmonitor false && + GIT_TEST_UF_DELAY_WARNING=1 git status >actual && + cat >expected <<-\EOF && + On branch main + + It took 3.25 seconds to enumerate untracked files. + See '\''git help status'\'' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual + ) +' + +test_expect_success 'slow status advice when core.untrackedCache true, but not fsmonitor' ' + ( + cd slowstatus && + git config core.untrackedCache true && + git config core.fsmonitor false && + GIT_TEST_UF_DELAY_WARNING=1 git status >actual && + cat >expected <<-\EOF && + On branch main + + It took 3.25 seconds to enumerate untracked files. + See '\''git help status'\'' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual + ) +' + +test_expect_success 'slow status advice when core.untrackedCache true, and fsmonitor' ' + ( + cd slowstatus && + git config core.untrackedCache true && + git config core.fsmonitor true && + GIT_TEST_UF_DELAY_WARNING=1 git status >actual && + cat >expected <<-\EOF && + On branch main + + It took 3.25 seconds to enumerate untracked files, + but the results were cached, and subsequent runs may be faster. + See '\''git help status'\'' for information on how to improve this. + + nothing to commit, working tree clean + EOF + test_cmp expected actual + ) +' + test_done diff --git a/wt-status.c b/wt-status.c index 5813174896c..b430d25da43 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,13 @@ static void wt_longstatus_print_tracking(struct wt_status *s) strbuf_release(&sb); } +static int uf_was_slow(struct wt_status *s) +{ + if (getenv("GIT_TEST_UF_DELAY_WARNING")) + s->untracked_in_ms = 3250; + return UF_DELAY_WARNING_IN_MS < s->untracked_in_ms; +} + static void show_merge_in_progress(struct wt_status *s, const char *color) { @@ -1814,6 +1823,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 +1880,21 @@ 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) { + if (advice_enabled(ADVICE_STATUS_U_OPTION) && uf_was_slow(s)) { 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 the results were cached, and subsequent runs may be faster."), + 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, - _("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); + _("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"), base-commit: 319605f8f00e402f3ea758a02c63534ff800a711 -- gitgitgadget ^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v9] status: modernize git-status "slow untracked files" advice 2022-11-30 0:52 ` [PATCH v9] " Rudy Rigot via GitGitGadget @ 2022-12-01 6:48 ` Junio C Hamano 2022-12-01 15:16 ` Rudy Rigot 0 siblings, 1 reply; 58+ messages in thread From: Junio C Hamano @ 2022-12-01 6:48 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget Cc: git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine, Rudy Rigot "Rudy Rigot via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Rudy Rigot <rudy.rigot@gmail.com> > > `git status` can be slow when there are a large number of > untracked files and directories since Git must search the entire > worktree to enumerate them. When it is too slow, Git prints > advice with the elapsed search time and a suggestion to disable > the search using the `-uno` option. This suggestion also carries > a warning that might scare off some users. > > However, these days, `-uno` isn't the only option. Git can reduce > the size and time of the untracked file search when the > `core.untrackedCache` and `core.fsmonitor` features are enabled by > caching results from previous `git status` invocations. > > Therefore, update the `git status` man page to explain the various > configuration options, and update the advice to provide more > detail about the current configuration and to refer to the updated > documentation. > > Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> > --- > status: modernize git-status "slow untracked files" advice > > Here is version 9 for this patch. > > Changes since v8: > > * Improved tests. > * The untracked files delay measured is now set to always the same > value in test cases. That has allowed to remove all sed calls from > tests. > * Improved documentation. Looking pretty good (I thought you were going to update the proposed log message, too, though). Let's replace and merge it down to 'next' to cook during the pre-release freeze period. Thanks. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v9] status: modernize git-status "slow untracked files" advice 2022-12-01 6:48 ` Junio C Hamano @ 2022-12-01 15:16 ` Rudy Rigot 2022-12-01 22:45 ` Junio C Hamano 0 siblings, 1 reply; 58+ messages in thread From: Rudy Rigot @ 2022-12-01 15:16 UTC (permalink / raw) To: Junio C Hamano Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine > (I thought you were going to update the proposed > log message, too, though) Oh, I'm sorry, what do you mean by updating the proposed log message? I'm looking back at past feedback, and I can't seem to match it to an unaddressed one. It was not intentional, so I would be very glad to fix it if you'd like, I'll gladly leave it up to you. Thanks a lot! ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v9] status: modernize git-status "slow untracked files" advice 2022-12-01 15:16 ` Rudy Rigot @ 2022-12-01 22:45 ` Junio C Hamano 2022-12-01 22:57 ` Rudy Rigot 0 siblings, 1 reply; 58+ messages in thread From: Junio C Hamano @ 2022-12-01 22:45 UTC (permalink / raw) To: Rudy Rigot Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine Rudy Rigot <rudy.rigot@gmail.com> writes: >> (I thought you were going to update the proposed >> log message, too, though) > > Oh, I'm sorry, what do you mean by updating the proposed log message? > I'm looking back at past feedback, and I can't seem to match it to an > unaddressed one. It was not intentional, so I would be very glad to > fix it if you'd like, I'll gladly leave it up to you. > > Thanks a lot! I was referring to this part: >> ... I don't think it provides compellingly more information >> for end users, compared to just mentioning time, so I'll simplify with >> your proposal here. in https://lore.kernel.org/git/CANaDLW+ukK2GU7NzkCvXVNc9DX3_93Pp+PHq-WcLpRJizPidVA@mail.gmail.com/ you sent earlier. No big deal, and thanks for working on this. ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v9] status: modernize git-status "slow untracked files" advice 2022-12-01 22:45 ` Junio C Hamano @ 2022-12-01 22:57 ` Rudy Rigot 0 siblings, 0 replies; 58+ messages in thread From: Rudy Rigot @ 2022-12-01 22:57 UTC (permalink / raw) To: Junio C Hamano Cc: Rudy Rigot via GitGitGadget, git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Eric Sunshine Oh, now I understand! But I had skipped it because I couldn't find it in my patch, and even now, I'm afraid I can't. It looks to me like part of an old patch, a paragraph that was rewritten 2 patches back. Is it possible that you found it in the lines that were being removed on the patch that you commented on, for instance? If I'm correct, then yeay! No problem in the end, this won't reach users. If I'm looking wrong though, then I'm very sorry about the miss. I also do get how this may not be worth a new round just for this fix. Thanks a lot for your (and everybody else's) guidance through this. This was my first patch, and I'm delighted about the solid shape it's landing with; I'm hoping it is the first of many. Thanks a lot! ^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v8] status: modernize git-status "slow untracked files" advice 2022-11-22 22:07 ` [PATCH v8] " Rudy Rigot via GitGitGadget 2022-11-25 4:58 ` Junio C Hamano 2022-11-30 0:52 ` [PATCH v9] " Rudy Rigot via GitGitGadget @ 2023-05-11 5:17 ` Eric Sunshine 2 siblings, 0 replies; 58+ messages in thread From: Eric Sunshine @ 2023-05-11 5:17 UTC (permalink / raw) To: Rudy Rigot via GitGitGadget Cc: git, Jeff Hostetler, Taylor Blau, Ævar Arnfjörð Bjarmason, Derrick Stolee, Rudy Rigot On Tue, Nov 22, 2022 at 5:07 PM Rudy Rigot via GitGitGadget <gitgitgadget@gmail.com> wrote: > `git status` can be slow when there are a large number of > untracked files and directories since Git must search the entire > worktree to enumerate them. When it is too slow, Git prints > advice with the elapsed search time and a suggestion to disable > the search using the `-uno` option. This suggestion also carries > a warning that might scare off some users. > > However, these days, `-uno` isn't the only option. Git can reduce > the size and time of the untracked file search when the > `core.untrackedCache` and `core.fsmonitor` features are enabled by > caching results from previous `git status` invocations. > > Therefore, update the `git status` man page to explain the various > configuration options, and update the advice to provide more > detail about the current configuration and to refer to the updated > documentation. > > Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com> > --- > Changes since v7: > > * Moved tests from new test script to existing one, in order not to > needlessly waste a test script number for such a small feature. Two > caveats: > * The use of test_config in a subshell result in: 'error: bug in the > test script: test_when_finished does nothing in a subshell', so > I've had to resort to using plain old git config instead. Sorry, I should have thought of this when making suggestions in my earlier reviews. For completeness, in case you run across this sort of issue again if submitting patches in the future, it is possible to get this working by using the -C option (change to directory) with test_config() outside of the subshell. For instance: test_expect_sucess 'some title' ' test_config -C slowstatus core.untrackedCache false && test_config -C slowstatus core.fsmonitor false && ( cd slowstatus && GIT_TEST_UF_DELAY_WARNING=1 git status >out && ... ) ' ^ permalink raw reply [flat|nested] 58+ messages in thread
end of thread, other threads:[~2023-05-11 5:18 UTC | newest] Thread overview: 58+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-15 13:04 [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case Rudy Rigot via GitGitGadget 2022-10-15 13:08 ` Rudy Rigot 2022-10-17 15:39 ` Jeff Hostetler 2022-10-17 16:59 ` Rudy Rigot 2022-10-20 12:56 ` Jeff Hostetler 2022-10-20 20:17 ` Rudy Rigot 2022-10-24 14:55 ` Jeff Hostetler 2022-10-29 0:06 ` [PATCH v2] status: long status advice adapted to recent capabilities Rudy Rigot via GitGitGadget 2022-11-02 19:45 ` Jeff Hostetler 2022-11-02 20:34 ` Rudy Rigot 2022-11-02 23:59 ` Taylor Blau 2022-11-03 14:28 ` Rudy Rigot 2022-11-04 8:52 ` Ævar Arnfjörð Bjarmason 2022-11-04 15:33 ` Rudy Rigot 2022-11-04 21:38 ` Taylor Blau 2022-11-02 21:27 ` [PATCH v3] " Rudy Rigot via GitGitGadget 2022-11-04 21:40 ` Taylor Blau 2022-11-07 20:02 ` Derrick Stolee 2022-11-07 23:19 ` Taylor Blau 2022-11-15 16:38 ` Jeff Hostetler 2022-11-07 20:01 ` Derrick Stolee 2022-11-07 20:20 ` Eric Sunshine 2022-11-07 20:31 ` Rudy Rigot 2022-11-10 4:46 ` [PATCH v4] " Rudy Rigot via GitGitGadget 2022-11-10 5:42 ` Eric Sunshine 2022-11-10 17:01 ` Rudy Rigot 2022-11-10 17:30 ` Eric Sunshine 2022-11-10 17:47 ` Rudy Rigot 2022-11-10 20:04 ` [PATCH v5] " Rudy Rigot via GitGitGadget 2022-11-15 16:39 ` Jeff Hostetler 2022-11-15 16:42 ` Rudy Rigot 2022-11-15 17:26 ` Eric Sunshine 2022-11-15 17:45 ` Rudy Rigot 2022-11-15 18:06 ` Eric Sunshine 2022-11-15 18:08 ` Rudy Rigot 2022-11-15 21:19 ` [PATCH v6] " Rudy Rigot via GitGitGadget 2022-11-21 5:06 ` Eric Sunshine 2022-11-21 15:54 ` Rudy Rigot 2022-11-21 16:17 ` Eric Sunshine 2022-11-22 16:52 ` Rudy Rigot 2022-11-22 17:18 ` Eric Sunshine 2022-11-22 17:24 ` Eric Sunshine 2022-11-22 17:29 ` Rudy Rigot 2022-11-22 17:40 ` Eric Sunshine 2022-11-22 18:07 ` Eric Sunshine 2022-11-22 19:19 ` Rudy Rigot 2022-11-22 19:48 ` Eric Sunshine 2022-11-22 16:59 ` [PATCH v7] status: modernize git-status "slow untracked files" advice Rudy Rigot via GitGitGadget 2022-11-22 22:07 ` [PATCH v8] " Rudy Rigot via GitGitGadget 2022-11-25 4:58 ` Junio C Hamano 2022-11-29 15:21 ` Rudy Rigot 2022-11-30 0:51 ` Rudy Rigot 2022-11-30 0:52 ` [PATCH v9] " Rudy Rigot via GitGitGadget 2022-12-01 6:48 ` Junio C Hamano 2022-12-01 15:16 ` Rudy Rigot 2022-12-01 22:45 ` Junio C Hamano 2022-12-01 22:57 ` Rudy Rigot 2023-05-11 5:17 ` [PATCH v8] " Eric Sunshine
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.