Re: [PATCH] sparse-checkout: disable advice in 'disable'

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <stolee@gmail.com> writes:

>>> +	grep "The sparse index is expanding to a full index" err &&
>>> +
>>> +	git -C sparse-index sparse-checkout disable 2>err &&
>>> +	test_line_count = 0 err
>>
>> I am not a huge fun of insisting that other code in the code path in
>> which I got rid of an unwanted error message must stay silent.
> ...
> I would say that there are generally two reasons why I chose to check
> that 'err' was empty here:
>
>  1. Using "! grep" feels flaky to me. If we changed the error message,
>     then we could accidentally cause the test to pass because we don't
>     see the old message. This is somewhat mitigated by having the same
>     test check for the existence of the message, so careful use of a
>     common variable might avoid this potential future.

Yup.  Duplicating is probably OK in practice as the eyes of the
developer who changed the message will be pulled to this test when
they notice the failure from the positive "grep" to notice the
negated version you use to replace "The err file must be absolutely
empty", but I agree that a common variable would be even safer.

> +  msg="The sparse index is expanding to a full index" &&
> -	grep "The sparse index is expanding to a full index" err
> +	grep "$msg" err &&
> +
> +	git -C sparse-index sparse-checkout disable 2>err &&
> +	! grep "$msg" err

Excellent.  "test_grep" and "test_grep !" are better choices, though.

>  2. If the output is currently empty, then testing that it stays empty
>     will be a more rigid test. It will help us notice a change of
>     behavior here, even if it is an intentional change.

Such a stricter position cuts both ways.

If we assume infinite engineering resource availablility on ongoing
basis, yes, it may lead to a good discipline.  But having millions
of such tests that will _notice_ changes that are irrelevant to the
thing the test is about (e.g., this thing is about sparse index
expansion advice), we'd be setting ourselves to adjust numerous
tests whose primary purpose has nothing to do with what we are
changing.

The choice also largely depends more on preference and taste that do
not have one absolute and universal answer.  I would personally
prefer avoiding overly specific tests, but I also find it attractive
if we can afford to be more specific in tests at times.

> For the progress motivation, I'm not too worried because the progress
> indicators depend on isatty(2)[*], so would not appear here even if the
> command was slow or somehow GIT_PROGRESS_DELAY=0 was set.

I already said that "progress" was a mere example, didn't I?

Even if we expect we will not ever see unwanted progress indicators
in this code, the general point still stands (iow, progress and
"unsparsifying warning" are not the only things that output to the
standard error stream).

> [*] #leftoverbits: 'git sparse-checkout' commands do not have --progress
> options, but could. The 'unpack_trees_options' structs have a member
> called 'show_progress' that could be populated based on a user option.

Nice.

Thanks.