Re: [PATCH v8] status: modernize git-status "slow untracked files" advice

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