Re: [GSoC][PATCH] tests: avoid using pipes

[Thomas Gummerer <t.gummerer@gmail.com>]

Hi,

welcome and thanks for the patch!

On 03/09, Jonathan Chang wrote:
> This is my attempt for this year's GSoC microproject.
> 
> I copied the commit message of the commit[1] mentioned in the microproject
> page[2]. Is this OK?
> 
> Here is a summary of what I did:
> 	- simple substitution as in c6f44e1da5[1]. 
> 	- besides simple substitution, I moved some git command out of command 
> 		substitution to improve readability, which was not possible with pipes, 
> 		while in these cases keeping them inside won't discard git's exit code.
> 	- use actual1 and actual2 in cases where actual is already in use.
> 	- use `sort -o actual actual` to avoid using actual1 and actual2.
> 
> [1] c6f44e1da5 ("t9813: avoid using pipes", 2017-01-04)
> [2] https://git.github.io/SoC-2019-Microprojects/
> 
> ---------------------->8-------------------
> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it. By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.
> 
> Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
> ---
>  t/t0000-basic.sh       | 28 ++++++++++++++--------------
>  t/t0003-attributes.sh  | 13 ++++++++-----
>  t/t0022-crlf-rename.sh |  6 +++---
>  3 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index b6566003dd..adc9545973 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -1118,27 +1118,25 @@ P=$(test_oid root)
>  
>  test_expect_success 'git commit-tree records the correct tree in a commit' '
>  	commit0=$(echo NO | git commit-tree $P) &&
> -	tree=$(git show --pretty=raw $commit0 |
> -		 sed -n -e "s/^tree //p" -e "/^author /q") &&
> +	git show --pretty=raw $commit0 >actual &&
> +	tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) &&
>  	test "z$tree" = "z$P"
>  '
>  
>  test_expect_success 'git commit-tree records the correct parent in a commit' '
>  	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
> -	parent=$(git show --pretty=raw $commit1 |
> -		sed -n -e "s/^parent //p" -e "/^author /q") &&
> +	git show --pretty=raw $commit1 >actual &&
> +	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual) &&
>  	test "z$commit0" = "z$parent"
>  '
>  
>  test_expect_success 'git commit-tree omits duplicated parent in a commit' '
>  	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
> -	     parent=$(git show --pretty=raw $commit2 |

This line is a bit oddly indented.  This is of course not something
you did, but it did leave me puzzled why the indentation was here
before and if the conversion is actually correct.  To help reviewers,
it would be nice to fix the indentation of this line in a preparatory
patch, and clarify since when this indentation was there and that it
is not correct.

That is if I'm not missing something, and there is not actually a
good reason for this to be indented, but judging from the original and
the conversion in this commit, I don't think there is.

> -		sed -n -e "s/^parent //p" -e "/^author /q" |
> -		sort -u) &&
> +	git show --pretty=raw $commit2 >actual &&
> +	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) &&
>  	test "z$commit0" = "z$parent" &&
> -	numparent=$(git show --pretty=raw $commit2 |
> -		sed -n -e "s/^parent //p" -e "/^author /q" |
> -		wc -l) &&
> +	git show --pretty=raw $commit2 >actual &&
> +	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
>  	test $numparent = 1
>  '
>  
> @@ -1147,7 +1145,8 @@ test_expect_success 'update-index D/F conflict' '
>  	mv path2 path0 &&
>  	mv tmp path2 &&
>  	git update-index --add --replace path2 path0/file2 &&
> -	numpath0=$(git ls-files path0 | wc -l) &&
> +	git ls-files path0 >actual &&
> +	numpath0=$(wc -l actual) &&

This test is actually failing now (and so is the one just below).  The
reason is that 'wc -l <file>' outputs the number of lines followed by
the filename, so numpath0 includes that.

We have a helper function that avoids exactly that,
'test_line_count'.  See t/README or t/test-lib-functions.sh for more
information on that function.

Before submitting a patch series, please also run the test suite, or
if you are only modifying tests such as in this case, at least the
tests that are modified.

>  	test $numpath0 = 1
>  '
>  
> @@ -1162,12 +1161,13 @@ test_expect_success 'very long name in the index handled sanely' '
>  	>path4 &&
>  	git update-index --add path4 &&
>  	(
> -		git ls-files -s path4 |
> -		sed -e "s/	.*/	/" |
> +		git ls-files -s path4 >actual &&
> +		sed -e "s/	.*/	/" actual |
>  		tr -d "\012" &&
>  		echo "$a"
>  	) | git update-index --index-info &&
> -	len=$(git ls-files "a*" | wc -c) &&
> +	git ls-files "a*" >actual &&
> +	len=$(wc -c actual) &&
>  	test $len = 4098
>  '
>  
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index 71e63d8b50..14274f1ced 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' '
>  test_expect_success 'attribute test: --all option' '
>  	grep -v unspecified <expect-all | sort >specified-all &&
>  	sed -e "s/:.*//" <expect-all | uniq >stdin-all &&
> -	git check-attr --stdin --all <stdin-all | sort >actual &&
> +	git check-attr --stdin --all <stdin-all >actual &&
> +	sort -o actual actual &&
>  	test_cmp specified-all actual
>  '
>  
>  test_expect_success 'attribute test: --cached option' '
> -	git check-attr --cached --stdin --all <stdin-all | sort >actual &&
> +	git check-attr --cached --stdin --all <stdin-all >actual &&
> +	sort -o actual actual &&
>  	test_must_be_empty actual &&
>  	git add .gitattributes a/.gitattributes a/b/.gitattributes &&
> -	git check-attr --cached --stdin --all <stdin-all | sort >actual &&
> +	git check-attr --cached --stdin --all <stdin-all >actual &&
> +	sort -o actual actual &&
>  	test_cmp specified-all actual
>  '
>  
> @@ -301,8 +304,8 @@ test_expect_success 'bare repository: check that --cached honors index' '
>  	(
>  		cd bare.git &&
>  		GIT_INDEX_FILE=../.git/index \
> -		git check-attr --cached --stdin --all <../stdin-all |
> -		sort >actual &&
> +		git check-attr --cached --stdin --all <../stdin-all >actual &&
> +		sort -o actual actual &&
>  		test_cmp ../specified-all actual
>  	)
>  '
> diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh
> index 7af3fbcc7b..b4772b72f7 100755
> --- a/t/t0022-crlf-rename.sh
> +++ b/t/t0022-crlf-rename.sh
> @@ -23,10 +23,10 @@ test_expect_success setup '
>  
>  test_expect_success 'diff -M' '
>  
> -	git diff-tree -M -r --name-status HEAD^ HEAD |
> -	sed -e "s/R[0-9]*/RNUM/" >actual &&
> +	git diff-tree -M -r --name-status HEAD^ HEAD >actual1 &&
> +	sed -e "s/R[0-9]*/RNUM/" actual1 >actual2 &&

Nit: rather than actual1 and actual2, it might be nice to give the
files slightly more meaningful names, maybe "output" and "normalized",
but feel free to try and come up with something better than that.

>  	echo "RNUM	sample	elpmas" >expect &&
> -	test_cmp expect actual
> +	test_cmp expect actual2
>  
>  '
>  
> -- 
> 2.21.0
>