Re: [PATCH v2 6/6] t2019: don't create unused files

[Andrei Rybak <rybak.a.v@gmail.com>]

Disclaimer: while trying to write a response to this email, I went a bit
off track, and a big portion of message below is an investigation of
test coverage of what is printed to standard output and standard error
by "git checkout".  It's still mostly relevant to the discussion about
t2019, or at least I hope so.

There are four parts to this investigation, starting with:


1. Standard output of "git checkout"
Other than "git checkout -p" (tested in t3701-add-interactive.sh), it
seems that the only thing that "git checkout" prints to standard output
is in:

   a) function "show_local_changes" in builtin/checkout.c -- a couple
      of tests in t7201-co.sh validate this
   b) function "report_tracking" in builtin/checkout.c -- there are
      tests that validate tracking information in output of "git
      checkout" in t6040-tracking-info.sh.  One test in
      t2020-checkout-detach.sh, titled 'tracking count is accurate after
      orphan check' validates it as well.

While trying to find all tests that validate standard output of
"git checkout", I found out a couple of things.


2. Standard error of "git checkout"
Honestly, I haven't noticed it before, but I found it surprising that
messages about branch switching:

    - "Switched to branch '%s'\n"
    - "Switched to a new branch '%s'\n"
    - "Switched to and reset branch '%s'\n"

are printed to standard error.

Several tests in t2020-checkout-detach.sh validate what is printed into
standard error by "git checkout" via a variation of ">actual 2>&1".
Tests for advice printed by "git checkout" (looking at "advice.c", it
all goes to stderr), also do a variation of ">actual 2>&1".


3. t2024-checkout-dwim.sh
Test 'loosely defined local base branch is reported correctly' in t2024
has an interesting validation of output of "git checkout":

	git checkout strict | sed -e "s/strict/BRANCHNAME/g" >expect &&
	status_uno_is_clean &&
	git checkout loose | sed -e "s/loose/BRANCHNAME/g" >actual &&
	status_uno_is_clean &&

	test_cmp expect actual

which is fine, except that neither file "expect" nor "actual" contain
the string "BRANCHNAME".  And this test was broken when it was
introduced in 05e73682cd (checkout: report upstream correctly even with
loosely defined branch.*.merge, 2014-10-14).  It was probably intended
for this test to redirect standard error of "git checkout".  It should
be cleaned up as a separate patch/topic.

On 06/04/2023 10:44, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Apr 04 2023, Andrei Rybak wrote:
> 
>> Tests in t2019-checkout-ambiguous-ref.sh redirect two invocations of
>> "git checkout" to files "stdout" and "stderr".  Several assertions are
>> made using file "stderr".  File "stdout", however, is unused.
>>
>> Don't redirect standard output of "git checkout" to file "stdout" in
>> t2019-checkout-ambiguous-ref.sh to avoid creating unnecessary files.
>>
>> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
>> ---
>>   t/t2019-checkout-ambiguous-ref.sh | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t2019-checkout-ambiguous-ref.sh b/t/t2019-checkout-ambiguous-ref.sh
>> index 2c8c926b4d..9540588664 100755
>> --- a/t/t2019-checkout-ambiguous-ref.sh
>> +++ b/t/t2019-checkout-ambiguous-ref.sh
>> @@ -16,7 +16,7 @@ test_expect_success 'setup ambiguous refs' '
>>   '
>>   
>>   test_expect_success 'checkout ambiguous ref succeeds' '
>> -	git checkout ambiguity >stdout 2>stderr
>> +	git checkout ambiguity 2>stderr
>>   '
> 
> Ditto earlier comments that we should just fix this, if I make this
> ">out" and "test_must_be_empty out" this succeeds, shouldn't we just use
> that?

4. t2019-checkout-ambiguous-ref.sh
Back on topic: is empty standard output something that this test in
t2019 should worry about?  Let's take a look at other tests.

Aside from what was mentioned in section 1, tests in t7201 don't look
at standard output of "git checkout".  There isn't a lot of
"test_must_be_empty" in t/t2*check*:

     $ git grep 'test_must_be_empty' t/t2*check*
     t/t2004-checkout-cache-temp.sh: test_must_be_empty stderr &&
     t/t2013-checkout-submodule.sh:  test_must_be_empty actual
     t/t2013-checkout-submodule.sh:  test_must_be_empty actual
     t/t2013-checkout-submodule.sh:  test_must_be_empty actual
     t/t2024-checkout-dwim.sh:       test_must_be_empty status.actual

The first one, in t2004, asserts output of "git checkout-index".
All three in t2013 assert output of "git checkout HEAD >actual 2>&1".
The last one, in t2024, asserts output of "git status".

(There's also one "test_line_count = 0" in the same test in t2004,
  but otherwise these tests seem to be pretty up-to-date w.r.t.
  to using test_must_be_empty helper)

> 
>>   test_expect_success 'checkout produces ambiguity warning' '
> 
> As an aside, we should really just combine these two tests.

My dumb script for finding unused files gives false-positives for such
tests.  And there a lot of tests that got split during introduction of
C_LOCALE_OUTPUT prerequisite or were introduced before C_LOCALE_OUTPUT
was phased out.

For t2019, however, the tests were created this way before
C_LOCALE_OUTPUT in 0cb6ad3c3d ("checkout: fix bug with ambiguous refs",
2011-01-11).  Then the prerequisite was added in 6b3d83efac
("t2019-checkout-ambiguous-ref.sh: depend on C_LOCALE_OUTPUT",
2011-04-03) and removed in d3bd0425b2 ("i18n: use test_i18ngrep in
lib-httpd and t2019", 2011-04-12).

> 
>> @@ -37,7 +37,7 @@ test_expect_success 'checkout reports switch to branch' '
>>   '
>>   
>>   test_expect_success 'checkout vague ref succeeds' '
>> -	git checkout vagueness >stdout 2>stderr &&
>> +	git checkout vagueness 2>stderr &&
>>   	test_set_prereq VAGUENESS_SUCCESS
>>   '
>