threads / patch / 50703

patchtests: avoid using pipes

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

## tl;dr

20 messages between Mar 9, 2019 and Mar 15, 2019. Diffs are folded; open one to read it.

replies: 19people: 4as markdown or json

Jonathan Chang· Mar 9, 2019, 15:45 UTC · lore
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(-)
Show changes to 3 files +25 −22

t/t0000-basic.sh, t/t0003-attributes.sh, t/t0022-crlf-rename.sh

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
Thomas Gummerer· Mar 9, 2019, 16:45 UTC · re: Jonathan Chang · lore

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

Hi,
welcome and thanks for the patch!
On 03/09, Jonathan Chang wrote:
Show 55 quoted lines
> 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.

Show 20 quoted lines
> -		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.

Show 69 quoted lines
>  	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.
Show 9 quoted lines
>  	echo "RNUM	sample	elpmas" >expect &&
> -	test_cmp expect actual
> +	test_cmp expect actual2
>  
>  '
>  
> -- 
> 2.21.0
>
Jonathan Chang· Mar 10, 2019, 08:07 UTC · re: Thomas Gummerer · lore

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

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>
Show changes to t/t0000-basic.sh +1 −1
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
Jonathan Chang· Mar 10, 2019, 08:08 UTC · re: Jonathan Chang · lore

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

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>
Show changes to t/t0000-basic.sh +14 −14
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
Jonathan Chang· Mar 10, 2019, 08:09 UTC · re: Jonathan Chang · lore

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

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>
Show changes to t/t0003-attributes.sh +8 −5
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
Jonathan Chang· Mar 10, 2019, 08:10 UTC · re: Jonathan Chang · lore

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

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>
Show changes to t/t0022-crlf-rename.sh +3 −3
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
Jonathan Chang· Mar 10, 2019, 08:11 UTC · re: Jonathan Chang · lore

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

Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
Show changes to t/t0000-basic.sh +4 −4
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
Eric Sunshine· Mar 10, 2019, 09:50 UTC · re: Jonathan Chang · lore

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

On Sun, Mar 10, 2019 at 4:11 AM Jonathan Chang <ttjtftx@gmail.com> wrote:
Show 8 quoted lines
> 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
Show 5 quoted lines
> @@ -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
ttjtftx· Mar 11, 2019, 16:10 UTC · re: Eric Sunshine · lore

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

On Sun, Mar 10, 2019 at 5:51 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 17 quoted lines
>
> 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.

Show 17 quoted lines
> 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.
Eric Sunshine· Mar 10, 2019, 10:03 UTC · re: Jonathan Chang · lore

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

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

Show 11 quoted lines
> 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.

Eric Sunshine· Mar 10, 2019, 10:13 UTC · re: Jonathan Chang · lore

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

On Sun, Mar 10, 2019 at 4:09 AM Jonathan Chang <ttjtftx@gmail.com> wrote:
Show 16 quoted lines
> 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.
jonathan chang· Mar 15, 2019, 01:56 UTC · re: Eric Sunshine · lore

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

On Sun, Mar 10, 2019 at 6:13 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 35 quoted lines
>
> 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.
Thomas Gummerer· Mar 10, 2019, 17:59 UTC · re: Jonathan Chang · lore

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

On 03/10, Jonathan Chang wrote:
Show 16 quoted lines
> 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.

Show 16 quoted lines
> 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
>
jonathan chang· Mar 15, 2019, 01:55 UTC · re: Thomas Gummerer · lore

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

On Mon, Mar 11, 2019 at 1:59 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 22 quoted lines
>
> 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.
Show 21 quoted lines
> > -- >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.

Show 5 quoted lines
> 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:
Show 7 quoted lines
> 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/
Christian Couder· Mar 15, 2019, 12:48 UTC · re: jonathan chang · lore

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

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:
Show 10 quoted lines
> > > 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.

Show 8 quoted lines
> > > 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.
Show 5 quoted lines
> 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>
Show 11 quoted lines
> > 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.

Christian Couder· Mar 10, 2019, 06:05 UTC · re: Jonathan Chang · lore

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

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.
Show 5 quoted lines
> 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.

ttjtftx· Mar 10, 2019, 08:27 UTC · re: Christian Couder · lore

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

Thanks for the reminders.

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

Show 5 quoted lines
> 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

Christian Couder· Mar 10, 2019, 15:05 UTC · re: ttjtftx · lore

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

On Sun, Mar 10, 2019 at 9:28 AM ttjtftx <ttjtftx@gmail.com> wrote:
Show 12 quoted lines
>
> 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.

ttjtftx· Mar 11, 2019, 15:54 UTC · lore

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

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.
Show 9 quoted lines
> 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.

jonathan chang· Mar 11, 2019, 16:45 UTC · lore

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

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:

Show 20 quoted lines
>
> 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.

← back to recent threads