Re: [PATCH v2 2/8] auto-crlf tests: check "git checkout" exit code

From: Torsten Bögershausen <tboegi@web.de>
Date: Dec 2, 2022, 05:59 UTC
Message-ID: <20221202055905.2rqp26xu4nuknlxw@tb-raspi4>
In-Reply-To: <02ead668-5b5d-877c-1e40-3656fd85945f@web.de>

On Fri, Dec 02, 2022 at 02:02:55AM +0100, René Scharfe wrote:

Show 54 quoted lines

> Am 02.12.2022 um 01:06 schrieb Ævar Arnfjörð Bjarmason:
> > Don't hide the exit code from the "git checkout" we run to checkout
> > our attributes file.
> >
> > This fixes cases where we'd have e.g. missed memory leaks under
> > SANITIZE=leak, this code doesn't leak (the relevant "git checkout"
> > leak has been fixed), but in a past version of git we'd continue past
> > this failure under SANITIZE=leak when these invocations had errored
> > out, even under "--immediate".
> >
> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> > ---
> >  t/t0027-auto-crlf.sh | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> > index a94ac1eae37..574344a99db 100755
> > --- a/t/t0027-auto-crlf.sh
> > +++ b/t/t0027-auto-crlf.sh
> > @@ -294,11 +294,17 @@ checkout_files () {
> >  	pfx=eol_${ceol}_crlf_${crlf}_attr_${attr}_ &&
> >  	for f in LF CRLF LF_mix_CR CRLF_mix_LF LF_nul
> >  	do
> > -		rm crlf_false_attr__$f.txt &&
> > -		if test -z "$ceol"; then
> > -			git checkout -- crlf_false_attr__$f.txt
> > +		if test -z "$ceol"
> > +		then
> > +			test_expect_success "setup $f checkout" '
> > +				rm crlf_false_attr__$f.txt &&
> > +				git checkout -- crlf_false_attr__$f.txt
> > +			'
> >  		else
> > -			git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt
> > +			test_expect_success "setup $f checkout with core.eol=$ceol" '
> > +				rm crlf_false_attr__$f.txt &&
> > +				git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt
> > +			'
>
> That adds five test_expect_success calls.  Wouldn't one suffice, for the
> whole for loop, and a "|| return 1"?
>
> One line above the context there's a "git config" call that should also
> be covered, right?
>
> Side note: The checkout commands only differ in their -c parameter.
> They could be unified like this, which might simplify handling their
> return code:
>
> 	git ${ceol:+-c core.eol=$ceol} checkout -- crlf_false_attr__$f.txt
>
> >  		fi
> >  	done
> >

That is a nice one. (I learned about the ${:+} yesterday). Is it supported by all the shells/os combinations ? If not, we can still extract the if:

        if test -z "$ceol"; then
          CEOL="-c core.eol=$ceol"
        else
          CEOL=""
        fi
        git $CEOL checkout -- crlf_false_attr__$f.txt &&

The original reasoning (for not looking at the return code) was that if the checkout fails, the test suite will fail further down anyway. But checking for failures early is a good thing. Thanks for cleaning up my mess.