Re: [PATCH] t3700: avoid suppressing git's exit code

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2026, 18:19 UTC
Message-ID: <xmqqv7fiqcaj.fsf@gitster.g>
In-Reply-To: <20260227165143.70188-1-r.siddharth.shrimali@gmail.com>

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

Show 11 quoted lines

> When piping the output of git ls-files into grep, the exit code of
> git ls-files is suppressed.
>
> Avoid this by redirecting the output of git ls-files to a file and
> then running grep on that file. This ensures that any crash in
> git ls-files will be caught by the test suite.
>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
>  t/t3700-add.sh | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)

A few things that I noticed.

 * If "git foo | grep bar" was expecting to hide exit code from
   "git" and check the output (e.g., "git diff --exit-code | grep foo"),
   a mechanical conversion "git diff --exit-code >out && grep foo out"
   would change the meaning of the test and break it.  I did not check
   if this patch has such an unintended breakage, though.

 * Many of them do this:

        > -	git ls-files foo | grep foo
        > +	git ls-files foo >actual &&
        > +	grep foo <actual

   or this

        > -	! ( git ls-files foo1 | grep foo1 )
        > +	git ls-files foo1 >actual &&
        > +	! grep foo1 actual

    in which we might consider using "test_grep" (and "test_grep !")
    to help the developer who wants to debug a breakage in ls-files
    by highlighting what is unexpected in the output in their broken
    version.

 * A rewrite like this may want to be further broken down.

        > -	test $(git ls-files --stage | grep ^100644 | wc -l) -eq 0 &&
        > +	test $(git ls-files --stage >actual && grep ^100644 actual | wc -l) -eq 0 &&

   If "ls-files --stage" segfaults, "grep | wc" would not run, $()
   may exit with non-zero and turn into an empty string, but the
   final error diagnosis would be something unfathonable like

	test: -eq unary operator expected
	test: missing argument after '0'
   
   which would not help the person debugging the test very much.