Re: [PATCH v4 1/6] auto-crlf tests: don't lose exit code in loops and outside tests

["René Scharfe" <l.s.r@web.de>]

Am 19.12.22 um 11:19 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 | 66 +++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 28 deletions(-)
>
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index a94ac1eae37..2f57c8669cb 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
> @@ -289,17 +299,17 @@ checkout_files () {
>  	lfmixcrlf=$1 ; shift
>  	lfmixcr=$1 ; shift
>  	crlfnul=$1 ; shift
> -	create_gitattributes "$attr" $ident $aeol &&
> -	git config core.autocrlf $crlf &&
> +	test_expect_success "setup config for checkout attr=$attr ident=$ident aeol=$aeol core.autocrlf=$crlf" '
> +		create_gitattributes "$attr" $ident $aeol &&
> +		git config core.autocrlf $crlf
> +	'
>  	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
> +		'

OK, but why have test_expect_success inside the loop here, while a simlilar
loop is inside the two test_expect_success bodies above?  Do we really need
five invocations here instead of one?  It's just setup code.

>  	done
>
>  	test_expect_success "ls-files --eol attr=$attr $ident aeol=$aeol core.autocrlf=$crlf core.eol=$ceol" '