Re: [PATCH v3 2/8] auto-crlf tests: don't lose exit code in loops and outside tests

From: "René Scharfe" <l.s.r@web.de>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Torsten Bögershausen" <tboegi@web.de>
Subject: Re: [PATCH v3 2/8] auto-crlf tests: don't lose exit code in loops and outside tests
Date: Fri, 2 Dec 2022 16:59:52 +0100	[thread overview]
Message-ID: <8cfb5bc5-6e17-4cac-cc2f-2f900f14fa60@web.de> (raw)
In-Reply-To: <patch-v3-2.8-394d5e46494-20221202T114733Z-avarab@gmail.com>

Am 02.12.22 um 12:52 schrieb Ævar Arnfjörð Bjarmason:
> Change the functions which are called from within
> "test_expect_success" to add the "|| return 1" idiom to their
> for-loops, so we won't lose the exit code of "cp", "git" etc.
>
> Then for those setup functions that aren't called from a
> "test_expect_success" we need to put the setup code in a
> "test_expect_success" as well. It would not be enough to properly
> &&-chain these, as the calling code is the top-level script itself. As
> we don't run the tests with "set -e" we won't report failing commands
> at the top-level.
>
> The "checkout" part of this would miss 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".
>
> Helped-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t0027-auto-crlf.sh | 60 +++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index a94ac1eae37..23f2e613401 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -70,7 +70,8 @@ create_NNO_MIX_files () {
>  				cp CRLF        ${pfx}_CRLF.txt &&
>  				cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
>  				cp LF_mix_CR   ${pfx}_LF_mix_CR.txt &&
> -				cp CRLF_nul    ${pfx}_CRLF_nul.txt
> +				cp CRLF_nul    ${pfx}_CRLF_nul.txt ||
> +				return 1
>  			done
>  		done
>  	done
> @@ -101,7 +102,8 @@ commit_check_warn () {
>  	do
>  		fname=${pfx}_$f.txt &&
>  		cp $f $fname &&
> -		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
> +		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" ||
> +		return 1
>  	done &&
>  	git commit -m "core.autocrlf $crlf" &&
>  	check_warning "$lfname" ${pfx}_LF.err &&
> @@ -121,15 +123,19 @@ commit_chk_wrnNNO () {
>  	lfmixcr=$1 ; shift
>  	crlfnul=$1 ; shift
>  	pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
> -	#Commit files on top of existing file
> -	create_gitattributes "$attr" $aeol &&
> -	for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
> -	do
> -		fname=${pfx}_$f.txt &&
> -		cp $f $fname &&
> -		printf Z >>"$fname" &&
> -		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
> -	done
> +
> +	test_expect_success 'setup commit NNO files' '
> +		#Commit files on top of existing file
> +		create_gitattributes "$attr" $aeol &&
> +		for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
> +		do
> +			fname=${pfx}_$f.txt &&
> +			cp $f $fname &&
> +			printf Z >>"$fname" &&
> +			git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" ||
> +			return 1
> +		done
> +	'
>
>  	test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
>  		check_warning "$lfwarn" ${pfx}_LF.err
> @@ -163,15 +169,19 @@ commit_MIX_chkwrn () {
>  	lfmixcr=$1 ; shift
>  	crlfnul=$1 ; shift
>  	pfx=MIX_attr_${attr}_aeol_${aeol}_${crlf}
> -	#Commit file with CLRF_mix_LF on top of existing file
> -	create_gitattributes "$attr" $aeol &&
> -	for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
> -	do
> -		fname=${pfx}_$f.txt &&
> -		cp CRLF_mix_LF $fname &&
> -		printf Z >>"$fname" &&
> -		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
> -	done
> +
> +	test_expect_success 'setup commit file with mixed EOL' '
> +		#Commit file with CLRF_mix_LF on top of existing file
> +		create_gitattributes "$attr" $aeol &&
> +		for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
> +		do
> +			fname=${pfx}_$f.txt &&
> +			cp CRLF_mix_LF $fname &&
> +			printf Z >>"$fname" &&
> +			git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" ||
> +			return 1
> +		done
> +	'
>
>  	test_expect_success "commit file with mixed EOL onto LF crlf=$crlf attr=$attr" '
>  		check_warning "$lfwarn" ${pfx}_LF.err
> @@ -294,12 +304,10 @@ checkout_files () {

The context lines right here are:

	create_gitattributes "$attr" $ident $aeol &&
	git config core.autocrlf $crlf &&

Those better be covered by test_expect_success as well, right?  Wrap them and
the whole loop in a single test_expect_success, like you did above?

>  	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
> -		else
> -			git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt
> -		fi
> +		test_expect_success "setup $f checkout ${ceol:+ with -c core.eol=$ceol}"  '
> +			rm -f crlf_false_attr__$f.txt &&
> +			git ${ceol:+-c core.eol=$ceol} checkout -- crlf_false_attr__$f.txt
> +		'
>  	done
>
>  	test_expect_success "ls-files --eol attr=$attr $ident aeol=$aeol core.autocrlf=$crlf core.eol=$ceol" '