[GSoC][PATCH] tests: avoid using pipes

[GSoC][PATCH] tests: avoid using pipes

[Jonathan Chang]

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 |
-		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) &&
 	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 &&
 	echo "RNUM	sample	elpmas" >expect &&
-	test_cmp expect actual
+	test_cmp expect actual2
 
 '
 
-- 
2.21.0

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

[Thomas Gummerer]

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
> 

[GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error

[Jonathan Chang]

Hi,

Thanks for the reviews.

Here are the changes in the second version:
	- bug fixes
	- add preparatory patch
	- seperate different files to different patch
	- change to use test_line_count in a seperate patch

Also I found that there is no such function as test_char_count, 
is it worthwile to add such function? Here are some stat:

`git grep 'test_line_count' | wc -l` = 626
`git grep 'wc -l' | wc -l` = 294
`git grep 'wc -c' | wc -l` = 68

-- >8 --

This is a preparatory step prior to removing the pipes after git
commands, which discards git's exit code and may mask a crash.

Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index b6566003dd..53821f5817 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1132,7 +1132,7 @@ test_expect_success 'git commit-tree records the correct parent in a commit' '
 
 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 |
+	parent=$(git show --pretty=raw $commit2 |
 		sed -n -e "s/^parent //p" -e "/^author /q" |
 		sort -u) &&
 	test "z$commit0" = "z$parent" &&
-- 
2.21.0

[GSoC][PATCH v2 2/5] t0000-basic: avoid using pipes

[Jonathan Chang]

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>

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 53821f5817..47666b013e 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 |
-		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) &&
 	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
 '
 
-- 
2.21.0

[GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes

[Jonathan Chang]

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>

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
 	)
 '
-- 
2.21.0

[GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes

[Jonathan Chang]

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>

diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh
index 7af3fbcc7b..05f443dcce 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 >actual &&
+	sed -e "s/R[0-9]*/RNUM/" actual >output &&
 	echo "RNUM	sample	elpmas" >expect &&
-	test_cmp expect actual
+	test_cmp expect output
 
 '
 
-- 
2.21.0

[GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l

[Jonathan Chang]

Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 47666b013e..aa25694b45 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '
 	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) &&
 	test "z$commit0" = "z$parent" &&
 	git show --pretty=raw $commit2 >actual &&
-	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
-	test $numparent = 1
+	sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l >numparent &&
+	test_line_count = 1 numparent
 '
 
 test_expect_success 'update-index D/F conflict' '
@@ -1146,8 +1146,8 @@ test_expect_success 'update-index D/F conflict' '
 	mv tmp path2 &&
 	git update-index --add --replace path2 path0/file2 &&
 	git ls-files path0 >actual &&
-	numpath0=$(wc -l <actual) &&
-	test $numpath0 = 1
+	wc -l <actual >numpath0 &&
+	test_line_count = 1 numpath0
 '
 
 test_expect_success 'very long name in the index handled sanely' '
-- 
2.21.0

Re: [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l

[Eric Sunshine]

On Sun, Mar 10, 2019 at 4:11 AM Jonathan Chang <ttjtftx@gmail.com> wrote:
> Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '
> -       numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
> -       test $numparent = 1
> +       sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l >numparent &&
> +       test_line_count = 1 numparent

This transformation makes no sense. The output of 'sed' is fed to 'wc
-l' whose output is redirected to file "numparent", which means that
the output file will end up containing a single line no matter how
many "parent" lines are matched in the input. Since test_line_count()
checks the number of lines in the named file, the test will succeed
unconditionally (which makes for a pretty poor test).

Also, the filename "numparent" doesn't do a good job of representing
what the file is expected to contain. A better name might be
"parents". So, a more correct transformation would be:

    sed -n -e "s/^parent //p" -e "/^author /q" actual >parents &&
    test_line_count = 1 parents

> @@ -1146,8 +1146,8 @@ test_expect_success 'update-index D/F conflict' '
> -       numpath0=$(wc -l <actual) &&
> -       test $numpath0 = 1
> +       wc -l <actual >numpath0 &&
> +       test_line_count = 1 numpath0

Same comment about bogus transformation. The entire sequence should
collapse to a single line:

    test_line_count = 1 actual

Re: [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l

[ttjtftx]

On Sun, Mar 10, 2019 at 5:51 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Mar 10, 2019 at 4:11 AM Jonathan Chang <ttjtftx@gmail.com> wrote:
> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
> >
> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> > @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '
> > -       numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
> > -       test $numparent = 1
> > +       sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l >numparent &&
> > +       test_line_count = 1 numparent
>
> This transformation makes no sense. The output of 'sed' is fed to 'wc
> -l' whose output is redirected to file "numparent", which means that
> the output file will end up containing a single line no matter how
> many "parent" lines are matched in the input. Since test_line_count()
> checks the number of lines in the named file, the test will succeed
> unconditionally (which makes for a pretty poor test).

You are right. I will make sure I have thoroughly reviewed my patch before
submitting next time.

> Also, the filename "numparent" doesn't do a good job of representing
> what the file is expected to contain. A better name might be
> "parents". So, a more correct transformation would be:
>
>     sed -n -e "s/^parent //p" -e "/^author /q" actual >parents &&
>     test_line_count = 1 parents
>
> > @@ -1146,8 +1146,8 @@ test_expect_success 'update-index D/F conflict' '
> > -       numpath0=$(wc -l <actual) &&
> > -       test $numpath0 = 1
> > +       wc -l <actual >numpath0 &&
> > +       test_line_count = 1 numpath0
>
> Same comment about bogus transformation. The entire sequence should
> collapse to a single line:
>
>     test_line_count = 1 actual

Thanks for the help.

Re: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes

[Eric Sunshine]

On Sun, Mar 10, 2019 at 4:10 AM Jonathan Chang <ttjtftx@gmail.com> wrote:
> 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>

All of the patches in this series are malformed. There should be a
"---" line right here below your sign-off. The "---" line is
recognized by git-am/git-apply as separating the commit message from
the actual diff(s).

> diff --git 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 >actual &&
> +       sed -e "s/R[0-9]*/RNUM/" actual >output &&
>         echo "RNUM      sample  elpmas" >expect &&
> -       test_cmp expect actual
> +       test_cmp expect output

It is a very well-established custom in Git tests for the files handed
to test_cmp() to be named "expect" and "actual", so this change is not
the most desirable. What you can do instead is:

    git diff-tree -M -r --name-status HEAD^ HEAD >output &&
    sed -e "s/R[0-9]*/RNUM/" output >actual &&

which allows you to leave the test_cmp() line alone, thus (as a bonus)
makes the patch less noisy.

Fwd: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes

[ttjtftx]

Sorry, I forgot to Cc.

---------- Forwarded message ---------
From: ttjtftx <ttjtftx@gmail.com>
Date: Mon, Mar 11, 2019 at 11:43 PM
Subject: Re: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes
To: Eric Sunshine <sunshine@sunshineco.com>


On Sun, Mar 10, 2019 at 6:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

> All of the patches in this series are malformed. There should be a
> "---" line right here below your sign-off. The "---" line is
> recognized by git-am/git-apply as separating the commit message from
> the actual diff(s).

I will check it next time.

> It is a very well-established custom in Git tests for the files handed
> to test_cmp() to be named "expect" and "actual", so this change is not
> the most desirable. What you can do instead is:
>
>     git diff-tree -M -r --name-status HEAD^ HEAD >output &&
>     sed -e "s/R[0-9]*/RNUM/" output >actual &&
>
> which allows you to leave the test_cmp() line alone, thus (as a bonus)
> makes the patch less noisy.

Thanks for the tips! Now I see why "actual" is the most used name for
output files.

Re: [GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes

[Eric Sunshine]

On Sun, Mar 10, 2019 at 4:09 AM Jonathan Chang <ttjtftx@gmail.com> wrote:
> 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>
>
> diff --git 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
>  '

There is no existing use of "sort -o" anywhere in the Git test suite
(or, for that matter, anywhere else in Git), which should give one
pause before using it here. Although -o is allowed by POSIX, and POSIX
even says it's safe for the output file to have the same name as one
of the input files, there is no guarantee that "sort -o" will be
supported on all platforms, or that all platforms promise that the
output filename can match an input filename (in fact, neither the
MacOS nor FreeBSD man pages for 'sort' make this promise).
Consequently, it would be better to err on the side of safety and
avoid "sort -o", which is easily enough done by using another
temporary file:

    git check-attr --stdin --all <stdin-all >output &&
    sort output >actual &&

The same comment applies to the remaining changes.

Re: [GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes

[jonathan chang]

On Sun, Mar 10, 2019 at 6:13 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Mar 10, 2019 at 4:09 AM Jonathan Chang <ttjtftx@gmail.com> wrote:
> > 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>
> >
> > diff --git 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
> >  '
>
> There is no existing use of "sort -o" anywhere in the Git test suite
> (or, for that matter, anywhere else in Git), which should give one
> pause before using it here. Although -o is allowed by POSIX, and POSIX
> even says it's safe for the output file to have the same name as one
> of the input files, there is no guarantee that "sort -o" will be
> supported on all platforms, or that all platforms promise that the
> output filename can match an input filename (in fact, neither the
> MacOS nor FreeBSD man pages for 'sort' make this promise).
> Consequently, it would be better to err on the side of safety and
> avoid "sort -o", which is easily enough done by using another
> temporary file:
>
>     git check-attr --stdin --all <stdin-all >output &&
>     sort output >actual &&
>
> The same comment applies to the remaining changes.

Noted. I did check POSIX, I should have also checked the use in Git.

Thanks for the review.

Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error

[Thomas Gummerer]

On 03/10, Jonathan Chang wrote:
> Hi,
> 
> Thanks for the reviews.
> 
> Here are the changes in the second version:
> 	- bug fixes
> 	- add preparatory patch
> 	- seperate different files to different patch
> 	- change to use test_line_count in a seperate patch
> 
> Also I found that there is no such function as test_char_count, 
> is it worthwile to add such function? Here are some stat:
> 
> `git grep 'test_line_count' | wc -l` = 626
> `git grep 'wc -l' | wc -l` = 294
> `git grep 'wc -c' | wc -l` = 68

I do think it would be helpful to introduce that helper, especially if
it is useful in this patch series.  There seem to be enough other
places where it can be useful to make it worth adding the helper.

> -- >8 --
> 
> This is a preparatory step prior to removing the pipes after git
> commands, which discards git's exit code and may mask a crash.

The commit message should also describe why we need this preparatory
step. Maybe something like:

      To reduce the noise in when refactoring this pipeline in a
      subsequent commit fix the indentation.  This has been wrong
      since the refactoring done in 1b5b2b641a ("t0000: modernise
      style", 2012-03-02), but carries no meaning.

> Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
>

Out of curiosity, how did you create the patch?  'git format-patch'
would add a '---' line followed by the diffstat, where you would
usually put the commentary that you put before the scissors line.  It
seems like 'git am' still handles this fine, which I didn't know, just
something I noticed because I'm used to the other format.

Since this patch series is now 5 patches, that commentary should go
into a cover letter (see the --cover-letter option in format-patch),
so the reviewers can read that first, and read the patches with that
in mind, focusing on the patch only, and not additional commentary
that applies to the whole series when reading the patch.

> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index b6566003dd..53821f5817 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -1132,7 +1132,7 @@ test_expect_success 'git commit-tree records the correct parent in a commit' '
>  
>  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 |
> +	parent=$(git show --pretty=raw $commit2 |
>  		sed -n -e "s/^parent //p" -e "/^author /q" |
>  		sort -u) &&
>  	test "z$commit0" = "z$parent" &&
> -- 
> 2.21.0
> 

Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error

[jonathan chang]

On Mon, Mar 11, 2019 at 1:59 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> On 03/10, Jonathan Chang wrote:
> > Hi,
> >
> > Thanks for the reviews.
> >
> > Here are the changes in the second version:
> >       - bug fixes
> >       - add preparatory patch
> >       - seperate different files to different patch
> >       - change to use test_line_count in a seperate patch
> >
> > Also I found that there is no such function as test_char_count,
> > is it worthwile to add such function? Here are some stat:
> >
> > `git grep 'test_line_count' | wc -l` = 626
> > `git grep 'wc -l' | wc -l` = 294
> > `git grep 'wc -c' | wc -l` = 68
>
> I do think it would be helpful to introduce that helper, especially if
> it is useful in this patch series.  There seem to be enough other
> places where it can be useful to make it worth adding the helper.

Thanks for the feedback.

> > -- >8 --
> >
> > This is a preparatory step prior to removing the pipes after git
> > commands, which discards git's exit code and may mask a crash.
>
> The commit message should also describe why we need this preparatory
> step. Maybe something like:
>
>       To reduce the noise in when refactoring this pipeline in a
>       subsequent commit fix the indentation.  This has been wrong
>       since the refactoring done in 1b5b2b641a ("t0000: modernise
>       style", 2012-03-02), but carries no meaning.
>
> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
> >
>
> Out of curiosity, how did you create the patch?  'git format-patch'
> would add a '---' line followed by the diffstat, where you would
> usually put the commentary that you put before the scissors line.  It
> seems like 'git am' still handles this fine, which I didn't know, just
> something I noticed because I'm used to the other format.

I believe I used git for that. But I cannot think of why it happened nor
reproduce it.

> Since this patch series is now 5 patches, that commentary should go
> into a cover letter (see the --cover-letter option in format-patch),
> so the reviewers can read that first, and read the patches with that
> in mind, focusing on the patch only, and not additional commentary
> that applies to the whole series when reading the patch.

I wasn't aware of this option. I tried to produce the format in others
cover letter using 'git diff' and options like '--stat', '--summary', with no
success. I consulted Documentation/SubmittingPatches, where I got the
idea of cover letter, but it doesn't mention the option '--cover-letter' and
the idea of cover letter is even confused with '--notes'[1].

I just reread some of the GSoC related mails in the mailing list and
found one[2] that introduced the usage of 'cover-letter', '--range-diff' and
'--interdiff'. As a newbie, I personally think it would be helpful to include
theses options along with others mentioned in SubmittingPatches.

[1]: Documentation/SubmittingPatches:

> You often want to add additional explanation about the patch,
> other than the commit message itself. Place such "cover letter"
> material between the three-dash line and the diffstat. For
> patches requiring multiple iterations of review and discussion,
> an explanation of changes between each iteration can be kept in
> Git-notes and inserted automatically following the three-dash
> line via `git format-patch --notes`.

[2]: https://public-inbox.org/git/CAPig+cSsAufCnHPJfjQd8A778UNAsXEst1m+ekQ7T83=2mMUnw@mail.gmail.com/

Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error

[Christian Couder]

Hi,

On Fri, Mar 15, 2019 at 2:55 AM jonathan chang <ttjtftx@gmail.com> wrote:
>
> On Mon, Mar 11, 2019 at 1:59 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > On 03/10, Jonathan Chang wrote:

> > > Also I found that there is no such function as test_char_count,
> > > is it worthwile to add such function? Here are some stat:
> > >
> > > `git grep 'test_line_count' | wc -l` = 626
> > > `git grep 'wc -l' | wc -l` = 294
> > > `git grep 'wc -c' | wc -l` = 68
> >
> > I do think it would be helpful to introduce that helper, especially if
> > it is useful in this patch series.  There seem to be enough other
> > places where it can be useful to make it worth adding the helper.

Yeah, it would be useful for Git, but it's not necessary of course for
your microproject, which is already more than big enough.

> > > This is a preparatory step prior to removing the pipes after git
> > > commands, which discards git's exit code and may mask a crash.
> >
> > The commit message should also describe why we need this preparatory
> > step. Maybe something like:
> >
> >       To reduce the noise in when refactoring this pipeline in a
> >       subsequent commit fix the indentation.

Or perhaps:

      Fix indentation of a line containing a pipeline to reduce the
noise when refactoring the pipeline in a subsequent commit.

> This has been wrong
> >       since the refactoring done in 1b5b2b641a ("t0000: modernise
> >       style", 2012-03-02), but carries no meaning.
> >
> > > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>

> > Since this patch series is now 5 patches, that commentary should go
> > into a cover letter (see the --cover-letter option in format-patch),
> > so the reviewers can read that first, and read the patches with that
> > in mind, focusing on the patch only, and not additional commentary
> > that applies to the whole series when reading the patch.
>
> I wasn't aware of this option. I tried to produce the format in others
> cover letter using 'git diff' and options like '--stat', '--summary', with no
> success. I consulted Documentation/SubmittingPatches, where I got the
> idea of cover letter, but it doesn't mention the option '--cover-letter' and
> the idea of cover letter is even confused with '--notes'[1].

The issue is that there are different things that can be considered
"cover letter". I agree that it's not very clear in
Documentation/SubmittingPatches.

There is the following which is related to --cover-letter:

"Multiple related patches should be grouped into their own e-mail
thread to help readers find all parts of the series.  To that end,
send them as replies to either an additional "cover letter" message
(see below), the first patch, or the respective preceding patch."

but it doesn't mention --cover-letter and it contains "(see below)"
though I don't see what that refers to.

> I just reread some of the GSoC related mails in the mailing list and
> found one[2] that introduced the usage of 'cover-letter', '--range-diff' and
> '--interdiff'. As a newbie, I personally think it would be helpful to include
> theses options along with others mentioned in SubmittingPatches.

I agree that there is room for improvement in SubmittingPatches.

Thanks,
Christian.

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

[Christian Couder]

On Sat, Mar 9, 2019 at 4:50 PM Jonathan Chang <ttjtftx@gmail.com> wrote:
>
> This is my attempt for this year's GSoC microproject.

Thanks for being interested in this year's GSoC and doing a 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].

If you take a look at c6f44e1da5 ("t9813: avoid using pipes",
2017-01-04) you can see the following:

    - it changes only one test file: t9813-git-p4-preserve-users.sh
    - its title starts with "t9813: "

It would have been nice if you did the same in your patch (that is
only change one test file and start the patch title with the test file
number).

On the microproject page, we say:

"Students: Please attempt only ONE microproject. We want quality, not quantity!"

And to other students working on a microproject, we also ask them to
focus on only one test file when they make those kinds of changes.

Best,
Christian.

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

[ttjtftx]

Thanks for the reminders.

On Sun, Mar 10, 2019 at 2:06 PM Christian Couder
<christian.couder@gmail.com> wrote:

> If you take a look at c6f44e1da5 ("t9813: avoid using pipes",
> 2017-01-04) you can see the following:
>
>     - it changes only one test file: t9813-git-p4-preserve-users.sh
>     - its title starts with "t9813: "
I adapted this format in my second version[1].

[1]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com

Thanks,
Jonathan

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

[Christian Couder]

On Sun, Mar 10, 2019 at 9:28 AM ttjtftx <ttjtftx@gmail.com> wrote:
>
> On Sun, Mar 10, 2019 at 2:06 PM Christian Couder
> <christian.couder@gmail.com> wrote:
>
> > If you take a look at c6f44e1da5 ("t9813: avoid using pipes",
> > 2017-01-04) you can see the following:
> >
> >     - it changes only one test file: t9813-git-p4-preserve-users.sh
> >     - its title starts with "t9813: "
> I adapted this format in my second version[1].
>
> [1]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com

It's better because each patch changes only one file, but I also
wanted to say that for a microproject you only need to focus on only
one file. So I would you suggest you keep only the patches that are
about "t0000-basic.sh" and drop the patches about
"t0003-attributes.sh" and "t0022-crlf-rename.sh".

(https://git.github.io/SoC-2019-Microprojects/ has now a "Only ONE
quality focused microproject per student" section with more content.)

It's also a small nit but your patches start with "t0000-basic: " not
just "t0000: " though the latter format is more frequent in existing
commits than the former.

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

[jonathan chang]

Sorry, I forgot to Cc, again.

---------- Forwarded message ---------
From: ttjtftx <ttjtftx@gmail.com>
Date: Tue, Mar 12, 2019 at 12:32 AM
Subject: Re: [GSoC][PATCH] tests: avoid using pipes
To: Christian Couder <christian.couder@gmail.com>


On Sun, Mar 10, 2019 at 11:05 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Sun, Mar 10, 2019 at 9:28 AM ttjtftx <ttjtftx@gmail.com> wrote:
> >
> > On Sun, Mar 10, 2019 at 2:06 PM Christian Couder
> > <christian.couder@gmail.com> wrote:
> >
> > > If you take a look at c6f44e1da5 ("t9813: avoid using pipes",
> > > 2017-01-04) you can see the following:
> > >
> > >     - it changes only one test file: t9813-git-p4-preserve-users.sh
> > >     - its title starts with "t9813: "
> > I adapted this format in my second version[1].
> >
> > [1]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com
>
> It's better because each patch changes only one file, but I also
> wanted to say that for a microproject you only need to focus on only
> one file. So I would you suggest you keep only the patches that are
> about "t0000-basic.sh" and drop the patches about
> "t0003-attributes.sh" and "t0022-crlf-rename.sh".

I will do that as I have made too many mistakes. I decided to do more
than one file because
I thought it might be too small even for a microproject and that there
would be more chances
for feedback if I include more files, since the same file tend to have
similar conversions needed.

> It's also a small nit but your patches start with "t0000-basic: " not
> just "t0000: " though the latter format is more frequent in existing
> commits than the former.

Got it.