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

From: Rudy Rigot <rudy.rigot@gmail.com>
Date: Nov 29, 2022, 15:21 UTC
Message-ID: <CANaDLW+ukK2GU7NzkCvXVNc9DX3_93Pp+PHq-WcLpRJizPidVA@mail.gmail.com>
In-Reply-To: <xmqq5yf3fx4s.fsf@gitster.g>

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

> "time" I can sort of understand ("can reduce the time taken to
> enumerate untracked files" is how I may phrase it, though), but
> what did you want to say with "size"?

This bit was provided by a past reviewer so I hope I don't misrepresent it, but I think the idea was to convey that the reason it's faster is because the part of the codebase it will do active work on is smaller. I don't think it provides compellingly more information for end users, compared to just mentioning time, so I'll simplify with your proposal here.

Show 5 quoted lines

> Additionally (read: you do not _have_ to do this to make this topic
> acceptable, but it probably is worth thinking about), if we need to
> introduce a new helper function uf_was_slow() anyway, a much better
> change may be to make the 2 seconds cut-off configurable, than
> inventing GIT_TEST_UF_DETAIL_WARNING used only for tests.

I agree it would be an improvement, I'm going to try to do this. This doesn't feel like a much more involved change compared to your alternative suggestion of setting a hardcoded test value for `s->untracked_in_ms`, so it feels like there's not much to lose from doing it this way, while users would gain some nice configurability.

For transparency, my intuition is that I'm not sure there will be use cases where the config will be meaningfully leveraged by users. My gut is that the current cut-off time is an arbitrary UX perception cut-off, so I'm not sure it would need to be different depending on given repo situations. But I'm also very aware that I could be wrong, and this could open use cases that I'm not thinking about. And to your point, it would be sensible to use it as a test input anyway, so we might as well make it a user-facing tweak; so, I'm on board.

> Wouldn't it be redacted into "It took 9X seconds to enumerate"?
> It probably does not happen, only because you are forcing the code
> to pretend that it took 2.001 seconds or something, I suspect.

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

> Also, what do you need /g modifier in "sed" script for?  I do not
> think we give more than one such number in the message we are
> testing.

Indeed, it's not useful to anything, and shouldn't be there, I'll fix this too.

All of the other comments are crystal clear, and I intend to implement them exactly as advised. You can expect a new patch this week, maybe even today.

Thanks a lot again!