Avoid using pipes
v4 cover Avoid using pipes
I added 2 additional patches that modernize style, based on discussion in [1].
Also added description in commit message as suggested in [2].
[v1]: https://public-inbox.org/git/[email protected]/
[v2]: https://public-inbox.org/git/[email protected]/
[v3]: https://public-inbox.org/git/[email protected]/
[1]: https://public-inbox.org/git/[email protected]/
[2]: https://public-inbox.org/git/[email protected]/
Jonathan Chang (5):
t0000: fix indentation
t0000: avoid using pipes
t0000: use test_line_count instead of wc -l
t0000: use test_cmp instead of "test" builtin
t0000: make use of the test_must_be_empty function
t/t0000-basic.sh | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)
Range-diff against v3:
1: 768bf373d0 = 1: 86146182b7 t0000: fix indentation 2: 8ee5e073a2 ! 2: 1ca1168f91 t0000: avoid using pipes
@@ -6,6 +6,10 @@
using it. By writing out the output of the git command to a file, we can test the exit codes of both the commands.
+ This commit doesn't make any additional simplifications, such as using
+ the test_line_count function for counting the lines in the output. These
+ simplifications will be made in subsequent commits.
+
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
3: 68590a40d7 = 3: 28986ed000 t0000: use test_line_count instead of wc -l -: ---------- > 4: 731463ed98 t0000: use test_cmp instead of "test" builtin -: ---------- > 5: 53f58a4f89 t0000: make use of the test_must_be_empty function
Signature · 1 line
2.21.0
PATCH v4 1/5 t0000: fix indentation
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.
--- t/t0000-basic.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/t0000-basic.sh +1 -1
@@ -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" &&
Signature · 1 line
2.21.0
PATCH v4 2/5 t0000: avoid using pipes
In reply to Jonathan ChangThe 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.
This commit doesn't make any additional simplifications, such as using the test_line_count function for counting the lines in the output. These simplifications will be made in subsequent commits.
--- t/t0000-basic.sh | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
t/t0000-basic.sh +14 -14
@@ -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 '
Signature · 1 line
2.21.0
PATCH v4 3/5 t0000: use test_line_count instead of wc -l
In reply to Jonathan Chang--- t/t0000-basic.sh | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
t/t0000-basic.sh +3 -4
@@ -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 >parents && + test_line_count = 1 parents ' test_expect_success 'update-index D/F conflict' '
@@ -1146,8 +1146,7 @@ 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 + test_line_count = 1 actual ' test_expect_success 'very long name in the index handled sanely' '
Signature · 1 line
2.21.0
PATCH v4 4/5 t0000: use test_cmp instead of "test" builtin
In reply to Jonathan Chang--- t/t0000-basic.sh | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
t/t0000-basic.sh +8 -6
@@ -1118,16 +1118,18 @@ P=$(test_oid root) test_expect_success 'git commit-tree records the correct tree in a commit' ' commit0=$(echo NO | git commit-tree $P) && - git show --pretty=raw $commit0 >actual && - tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) && - test "z$tree" = "z$P" + git show --pretty=raw $commit0 >output && + echo "$P" >expect && + sed -n -e "s/^tree //p" -e "/^author /q" output >actual && + test_cmp expect actual ' test_expect_success 'git commit-tree records the correct parent in a commit' ' commit1=$(echo NO | git commit-tree $P -p $commit0) && - git show --pretty=raw $commit1 >actual && - parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual) && - test "z$commit0" = "z$parent" + git show --pretty=raw $commit1 >output && + echo "$commit0" >expect && + sed -n -e "s/^parent //p" -e "/^author /q" output >actual && + test_cmp expect actual ' test_expect_success 'git commit-tree omits duplicated parent in a commit' '
Signature · 1 line
2.21.0
Re: [GSoC][PATCH v4 4/5] t0000: use test_cmp instead of "test" builtin
Quoted 10 lines · Jonathan Chang
Signed-off-by Jonathan Chang <[email protected]>--- t/t0000-basic.sh | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)t/t0000-basic.sh +0
-0
t/t0000-basic.sh P=$(test_oid root) · line 1121
Quoted 4 lines
t/t0000-basic.sh +0
-1@@ -1118,16 +1118,18 @@ P=$(test_oid root) test_expect_success 'git commit-tree records the correct tree in a commit' ' commit0=$(echo NO | git commit-tree $P) && - git show --pretty=raw $commit0 >actual &&
This line has been introduced in a previous commit. If the file was called 'output' there already, I think that patch would be just as understandable, but this diff would be a little less noisy.
t/t0000-basic.sh P=$(test_oid root) · line 1123
Quoted 5 lines
t/t0000-basic.sh +3
-2- tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) && - test "z$tree" = "z$P" + git show --pretty=raw $commit0 >output && + echo "$P" >expect && + sed -n -e "s/^tree //p" -e "/^author /q" output >actual &&
I'd find it a bit more natural to either first create the expect file, and then do the 'git show' and 'sed' invocations in two subsequent lines, or do them first, and then create the expect files, rather than interleaving them.
I'm not sure either of these by itself is worth a new iteration, unless there is also something else to fix up. But it's something that you might want to keep in mind for future patches.
Quoted 18 lines
t/t0000-basic.sh +5
-3+ test_cmp expect actual ' test_expect_success 'git commit-tree records the correct parent in a commit' ' commit1=$(echo NO | git commit-tree $P -p $commit0) && - git show --pretty=raw $commit1 >actual && - parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual) && - test "z$commit0" = "z$parent" + git show --pretty=raw $commit1 >output && + echo "$commit0" >expect && + sed -n -e "s/^parent //p" -e "/^author /q" output >actual && + test_cmp expect actual ' test_expect_success 'git commit-tree omits duplicated parent in a commit' 'Signature · 1 line
2.21.0
Re: [GSoC][PATCH v4 4/5] t0000: use test_cmp instead of "test" builtin
Quoted 13 lines · Thomas Gummerer
Quoted 11 lines · Jonathan Chang
Signed-off-by Jonathan Chang <[email protected]>--- t/t0000-basic.sh | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)t/t0000-basic.sh +0
-0@@ -1118,16 +1118,18 @@ P=$(test_oid root)
Quoted 7 lines
t/t0000-basic.sh P=$(test_oid root) · line 1121
Quoted 3 lines
t/t0000-basic.sh +0
-1test_expect_success 'git commit-tree records the correct tree in a commit' ' commit0=$(echo NO | git commit-tree $P) && - git show --pretty=raw $commit0 >actual &&This line has been introduced in a previous commit. If the file was called 'output' there already, I think that patch would be just as understandable, but this diff would be a little less noisy.
Make sense. I tried to make patches from last iteration untouched, so I don't break anything. And I was wondering since I'm only appending patches, if I should also append the PATCH number as [PATCH v3 4/3], to reduce the number of emails. Now I realize that by making it a new iteration, we can also make some improvement to reduce the patch noise.
Quoted 7 lines
t/t0000-basic.sh P=$(test_oid root) · line 1123
Quoted 6 lines
t/t0000-basic.sh +3
-2- tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) && - test "z$tree" = "z$P" + git show --pretty=raw $commit0 >output && + echo "$P" >expect && + sed -n -e "s/^tree //p" -e "/^author /q" output >actual &&
Quoted 8 lines
I'd find it a bit more natural to either first create the expect file, and then do the 'git show' and 'sed' invocations in two subsequent lines, or do them first, and then create the expect files, rather than interleaving them.
I'm not sure either of these by itself is worth a new iteration, unless there is also something else to fix up. But it's something that you might want to keep in mind for future patches.
I thought about all three options, starting with expect files being the last, but found that most test_cmp seem to have expect file created first, then decided to make the creation of actual and expect be on consecutive lines.
Now I would probably move the creation expect to the last line. Thanks for the review and suggestions.
Quoted 19 lines
Quoted 19 lines
t/t0000-basic.sh +5
-4+ test_cmp expect actual ' test_expect_success 'git commit-tree records the correct parent in a commit' ' commit1=$(echo NO | git commit-tree $P -p $commit0) && - git show --pretty=raw $commit1 >actual && - parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual) && - test "z$commit0" = "z$parent" + git show --pretty=raw $commit1 >output && + echo "$commit0" >expect && + sed -n -e "s/^parent //p" -e "/^author /q" output >actual && + test_cmp expect actual ' test_expect_success 'git commit-tree omits duplicated parent in a commit' ' --2.21.0
Re: [GSoC][PATCH v4 4/5] t0000: use test_cmp instead of "test" builtin
Quoted 22 lines · jonathan chang
Quoted 13 lines · Thomas Gummerer
Quoted 11 lines · Jonathan Chang
Signed-off-by Jonathan Chang <[email protected]>--- t/t0000-basic.sh | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)t/t0000-basic.sh +0
-0@@ -1118,16 +1118,18 @@ P=$(test_oid root)Quoted 7 lines
t/t0000-basic.sh P=$(test_oid root) · line 1121
Quoted 3 lines
t/t0000-basic.sh +0
-1test_expect_success 'git commit-tree records the correct tree in a commit' ' commit0=$(echo NO | git commit-tree $P) && - git show --pretty=raw $commit0 >actual &&This line has been introduced in a previous commit. If the file was called 'output' there already, I think that patch would be just as understandable, but this diff would be a little less noisy.
Quoted 2 lines
Make sense. I tried to make patches from last iteration untouched, so I don't break anything.
This is what running tests, and reviewing your own code are good for :)
Quoted 5 lines
And I was wondering since I'm only
appending patches, if I should also append the PATCH number as [PATCH v3 4/3], to reduce the number of emails. Now I realize that by making it a new iteration, we can also make some improvement to reduce the patch noise.
Right, appending patches using 4/3 for example can be done in some rare cases, but since you already modified patch 2/2, a new new iteration is required anyway.
So yeah improving the overall series is always a good thing. Especially since I see you already included the range-diff, it's not very hard for reviewers to see what changed in the last iteration.
PATCH v4 5/5 t0000: make use of the test_must_be_empty function
In reply to Jonathan Chang--- t/t0000-basic.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/t0000-basic.sh +2 -2
@@ -51,7 +51,7 @@ test_expect_success 'verify that the running shell supports "local"' ' test_expect_success '.git/objects should be empty after git init in an empty repo' ' find .git/objects -type f -print >should-be-empty && - test_line_count = 0 should-be-empty + test_must_be_empty should-be-empty ' # also it should have 2 subdirectories; no fan-out anymore, pack, and info.
@@ -1110,7 +1110,7 @@ test_expect_success 'git update-index --refresh should succeed' ' test_expect_success 'no diff after checkout and git update-index --refresh' ' git diff-files >current && - cmp -s current /dev/null + test_must_be_empty current ' ################################################################
Signature · 1 line
2.21.0