patchfsmonitor: long status advice adapted to the fsmonitor use case

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(-)

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:

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 ?

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)

[...]

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

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.

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:

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.

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:

(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:

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:

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:

 > [...]

yeah, i'm happy to help.  just let me know when you have a first draft.

Jeff

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

On 10/28/22 8:06 PM, Rudy Rigot via GitGitGadget 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:

     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. :-)

[...]

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.)

[...]

[...]

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.

[...]

[...]

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

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.

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.

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!

Sounds good, will fix.

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.

Oops, I didn't realize people were getting the same content from either. I will fix.

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.

On Wed, Nov 02, 2022 at 03:45:18PM -0400, Jeff Hostetler wrote:

Excellent suggestions, thank you.

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

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

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?

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,

On Thu, Nov 03 2022, Rudy Rigot wrote:

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.

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 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:

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:

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.

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.

On Thu, Nov 03, 2022 at 09:28:56AM -0500, Rudy Rigot wrote:

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

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

On Wed, Nov 02, 2022 at 09:27:47PM +0000, Rudy Rigot via GitGitGadget wrote:

This one is looking good to me. Jeff: do you agree? If so, I'm ready to start merging this one down.

Thanks, Taylor

On 11/4/22 5:40 PM, Taylor Blau wrote:

Sorry, I came in with some late review. I think one more round would be helpful.

Thanks, -Stolee

On Mon, Nov 07, 2022 at 03:02:09PM -0500, Derrick Stolee wrote:

Thanks, all. Will wait for one more round before we start merging this down.

Thanks, Taylor

On 11/4/22 5:40 PM, Taylor Blau wrote:

This series is looking good to me too. I think V5 has addressed all of the concerns.

Jeff

On 11/2/22 5:27 PM, Rudy Rigot via GitGitGadget wrote:> +UNTRACKED FILES AND STATUS SPEED

Sorry I'm late to this series. This new section is a great idea.

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.

  "This is the fastest option." ?

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.

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.

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.

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.

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.

other than that nit, the code looks good to me.

Thanks for working on this! -Stolee

On Mon, Nov 7, 2022 at 3:07 PM Derrick Stolee <derrickstolee@github.com> wrote:

I've been meaning to respond to say the exact same thing about using here-doc instead.

Also, the new tests seem to have an oddball mixture of indentation using spaces and TAB. They should be using TAB only.

Thanks to both of you Derrick and Eric, this is all great feedback. I've been through all of it, it's all crystal clear, and I'm planning to just integrate all of it as is. I'll have a new iteration later this week (it's small stuff, I know, but I'm volunteering as a poll worker today and tomorrow).

Thanks again!

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

On Wed, Nov 9, 2022 at 11:46 PM Rudy Rigot via GitGitGadget <gitgitgadget@gmail.com> wrote:

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.

This leaves the reader hanging somewhat by not saying where "elsewhere" actually is. You can use gitlink: to insert links to the appropriate documentation.

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).

This variable is unused, isn't it, now that you're inlining everything with here-docs?

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 &&

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 &&

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

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.

Does this need to be inline? Is this a hot piece of code, or is this merely a premature optimization?

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.

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:

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.

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.

On Thu, Nov 10, 2022 at 12:02 PM Rudy Rigot <rudy.rigot@gmail.com> wrote:

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.

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
    '

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:

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

On 11/10/22 3:04 PM, Rudy Rigot via GitGitGadget wrote:

I think V5 looks good. Thanks for your persistence!

Jeff

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.

My absolute pleasure!

On Thu, Nov 10, 2022 at 3:04 PM Rudy Rigot via GitGitGadget <gitgitgadget@gmail.com> wrote:

Mostly just some minor style-related comments below, but also a couple grammos in the new documentation, and a question or two...

s/file/files/

Might want backticks around the literal filename: `.git/index`

Also: s/cost searching/cost of searching/

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

Style: We normally make the here-doc body indentation match the
indentation of the command itself:

    cat >expected <<-\EOF &&
    On branch test
    ...
    EOF

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.

Does this need to be "inline"?

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;

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.

To what does "this" refer? Is it this repository? Or something else?

Okay.

Thanks for the feedback, and I'll integrate it into a new patch, most likely today.

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.

Totally a whim. After several iterations, the code had changed enough that the original ordering was lost. I'll switch it back.

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.

On Tue, Nov 15, 2022 at 12:46 PM Rudy Rigot <rudy.rigot@gmail.com> wrote:

Thanks. I think this is _almost_ there; much more polished than earlier iterations. Hopefully, one final reroll will make it complete.

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".)

That makes a ton of sense, I agree with your thinking. Thanks for that, I'll do exactly that.

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

On Tue, Nov 15, 2022 at 4:19 PM Rudy Rigot via GitGitGadget <gitgitgadget@gmail.com> wrote:

Thanks, I think v6 addresses all my earlier review comments. Just one or two notes below...

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.

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.

Eh, there's nothing wrong with striving for perfection. Lemme do one more reroll...

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.

Oh, this seems like a better way to do what I was trying to do. I'll change it now.

I'll use that.

New patch coming!

On Mon, Nov 21, 2022 at 10:55 AM Rudy Rigot <rudy.rigot@gmail.com> wrote:

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)

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".

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.)

My intuition was that it was something like that indeed.

The current phrasing was initially copied as is from a past review feedback; I have no issues at all replacing it with yours.

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

On Tue, Nov 22, 2022 at 11:52 AM Rudy Rigot <rudy.rigot@gmail.com> wrote:

Apparently, I overlooked that when scanning backward through the thread.

No need to apologize. Reviewers understand that there is a lot to absorb and figure out.

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 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.

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.

Perhaps the suggestion I outlined above will help.

On Tue, Nov 22, 2022 at 12:18 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

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.)

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...

On Tue, Nov 22, 2022 at 12:18 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...

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.

On Tue, Nov 22, 2022 at 12:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

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 &&
       ...
    '

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!

On Tue, Nov 22, 2022 at 2:19 PM Rudy Rigot <rudy.rigot@gmail.com> wrote:

Glad to hear that it all worked out.

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

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(-)

"Rudy Rigot via GitGitGadget" <gitgitgadget@gmail.com> writes:

"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"?

Lose "Therefore, "; the resulting text would be much easier to follow.

"STATUS SPEED" somehow does not sound quite grammatical. Perhaps "untracked files and performance" or something instead?

Good.

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 ...

Lose the SP before the ":" (applies to all other entries, too).

OK.

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.

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.

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.

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.

Thanks for the amazing feedback, and sorry for the delay.

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.

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.

Yup, you guessed right. I'll be sure to change the regular expression to be resilient to double-digit times.

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!

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.

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(-)

"Rudy Rigot via GitGitGadget" <gitgitgadget@gmail.com> writes:

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.

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!

Rudy Rigot <rudy.rigot@gmail.com> writes:

I was referring to this part:

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.

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!

On Tue, Nov 22, 2022 at 5:07 PM Rudy Rigot via GitGitGadget <gitgitgadget@gmail.com> wrote:

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 &&
            ...
        )
    '