| Message ID | pull.1384.git.1665839050813.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsmonitor: long status advice adapted to the fsmonitor use case | expand |
> 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.
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
> 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.
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
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!
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
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"),