Re: [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Oct 17, 2022, 15:39 UTC
Message-ID: <087d3fca-d01e-f36a-85f5-7e861e4d11ca@jeffhostetler.com>
In-Reply-To: <pull.1384.git.1665839050813.gitgitgadget@gmail.com>

On 10/15/22 9:04 AM, Rudy Rigot via GitGitGadget wrote:

Show 8 quoted lines

> From: Rudy Rigot <rudy.rigot@gmail.com>
> 
> Currently, if git-status takes more than 2 seconds for enumerating
> untracked files, a piece of advice is given to the user to consider
> ignoring untracked files. This is somewhat at odds with the UX
> upsides from having fsmonitor enabled, since fsmonitor will be
> here to take care of mitigating the performance downsides from
> those untracked files.

Yes, the original advice needs some updating.  Thanks!

We should be careful here, FSMonitor only helps with untracked files if the untracked cache (UC) is also turned on. They do work well together and they greatly speed up things, but if either is turned off, `git status` will still need to scan. (Small nerd detail: the UC by itself does speed things up -- it only needs to lstat known directories most of the time, rather than actually readdir them. When both are on, we don't even need to do that.)

The original message suggested turning off the untracked file (UF) scan completely. Perhaps, we could have advice say to turn off UF scan -or- turn on FSM & UC ?

Show 14 quoted lines

> 
> I considered just suppressing that piece of advice entirely for
> repositories with fsmonitor disabled, but I decided to replace
> it with another piece of advice instead, letting the user know
> that this run may have been slow, but the next ones should be faster.
> Of course, please let me know if the phrasing can be improved. To
> keep consistent with other pieces of advice, this new one can be
> hidden with a new advice.statusFsmonitor config.
> 
> If the repository does not have fsmonitor enabled, or if the new
> piece of advice is hidden by config, the behavior falls back to
> today's behavior: show the message advising to ignore untracked
> files, as long as it wasn't disabled with the existing advice.statusUoption
> config.

We also need to be careful here. With FSMonitor the "first one is slow, the second one is fast" happens because `git status` is secretly updating the index (despite looking like a read-only command). That causes the index to have an updated FSM token (so that subsequent FSM responses are relative to a more recent checkpoint). See [1]. So it isn't always correct that the second status will be faster. It usually is, but it just depends on the threshold -- and more importantly, that the UC is turned on. (So I guess what I'm saying is that again, we should advise to turn UF or turn on both FSM & UC.)

[1] 26b9f34ab3 (fsmonitor: force update index after large responses, 2022-03-25)

Show 7 quoted lines

> 
> Test-wise, I tried to figure out ways to mock the behavior of a
> slow git-status, but I couldn't figure it out, so I could use some
> advice. I tracked down Commit 6a38ef2ced (status: advise to consider
> use of -u when read_directory takes too long, 2013-03-13), and it
> also didn't have tests, so I'm questioning whether it can in fact
> be reasonably done. Thanks in advance for any guidance.

[...]

WRT testing, you might do something like:

	#define UF_DELAY_WARNING_IN_MS (2 * 1000)

	static inline int uf_was_slow(uint32_t untracked_in_ms)
	{
	#ifdef GIT_TEST_UF_DELAY_WARNING // or maybe use getenv() here
		untracked_in_ms += UF_DELAY_WARNING_IN_MS + 1;
	#endif

		return UF_DELAY_WARNING_IN_MS < untracked_in_ms;
	}

And then you can set the GIT_TEST_ symbol (or env var) during the test scripts and confirm that we get the messages that you expect.

Hope this helps, Jeff