Re: [PATCH v3 0/6] t: add greplint.pl and convert grep to test_grep

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

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Changes since v2:
>
>  * t3420-rebase-autostash: dropped the change to the '! grep dirty file3'
>    line under 'rebase --quit'. As SZEDER pointed out, file3 never exists in
>    the conflicted state, so that grep was passing only because it could not
>    open the file. SZEDER's fix (sg/t3420-do-not-grep-in-missing-file, now in
>    'next') replaces the line with 'test_path_is_missing file3', which is the
>    right check; this series simply leaves that line to his fix.
>
>  * Audited the remaining '# lint-ok' annotations for the same "grep a file
>    that never exists with correctly running Git" gotcha, as Junio suggested.
>    The rule the audit applies: 'grep' becomes 'test_grep' only where its
>    exit code is the assertion; grep that produces data (a filter) or that
>    reads a file whose presence is conditional stays a plain 'grep', because
>    test_grep BUGs on a missing file.
>    
>    * t5537 (.git/shallow): the file is still present after the repack (the
>      client stays shallow), so the assertion is converted to 'test_grep !'
>      like any other; the "may not exist" note was wrong.
>    
>    * t1400 (.git/packed-refs): the file exists only with the files backend.
>      Guarded the packed-refs check with a REFFILES prerequisite; the
>      backend-agnostic 'git show-ref' check that follows still runs under
>      every backend.
>    
>    * t7450 (squatting-clone/d/a/git~2): kept as '! grep' with an improved '#
>      lint-ok'. 'git~2' is the NTFS 8.3 short name of a planted '..git' decoy
>      and only exists when 8.3 short-name generation is enabled. Verified on
>      a Windows VM: with 8.3 disabled (the modern default) the short name is
>      absent, the '! grep' correctly tolerates it, and a plain test_grep
>      would BUG. So this one deliberately stays a missing-file-tolerant grep.
>    
>    * t5326 and t5702 remain annotated: these are genuine data filters (grep
>      produces data that is redirected/captured, not an assertion).

Great.

>      ++   test_grep requires <file> to exist and will BUG otherwise.
>      ++   When a file's presence is conditional (a backend-specific
>      ++   file, or a path that only exists on some platforms, such as
>      ++   an NTFS 8.3 short name), keep a plain guarded 'grep' instead.

It is not quite clear if I can follow this instruction myself,
without knowing what a "plain guarded 'grep'" is, unfortunately.  Is
it different from bog-standard grep?

>      @@ t/t1400-update-ref.sh: test_expect_success "move $m (by HEAD)" '
>        	test_when_finished "git update-ref -d $m" &&
>        	git update-ref -d HEAD $B &&
>       -	! grep "$m" .git/packed-refs &&
>      -+	! grep "$m" .git/packed-refs && # lint-ok: file may not exist (reftable)
>      ++	if test_have_prereq REFFILES
>      ++	then
>      ++		test_grep ! "$m" .git/packed-refs
>      ++	fi &&

The intent is shown very well in this version (admittedly, the
lint-ok comment is readable but only by humans and LLMs).  Here, we
expect .git/packed-refs only while REFFILES prerequiste is active.

Thanks.