git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
@ 2018-03-13 20:19 Pratik Karki
  2018-03-14  7:30 ` Eric Sunshine
  0 siblings, 1 reply; 7+ messages in thread
From: Pratik Karki @ 2018-03-13 20:19 UTC (permalink / raw)
  To: git; +Cc: Pratik Karki

This patch removes the necessity of pipes in git related commands for test suite.

Exit code of the upstream in a pipe is ignored so, it's use should be avoided. The fix for this is to write the output of the git command to a file and test the exit codes of both the commands being linked by pipe.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 t/t7001-mv.sh                        | 24 ++++++++++++------------
 t/t9104-git-svn-follow-parent.sh     |  4 ++--
 t/t9110-git-svn-use-svm-props.sh     | 36 ++++++++++++++++++------------------
 t/t9111-git-svn-use-svnsync-props.sh | 36 ++++++++++++++++++------------------
 t/t9114-git-svn-dcommit-merge.sh     |  8 ++++----
 t/t9130-git-svn-authors-file.sh      | 16 ++++++++--------
 t/t9138-git-svn-authors-prog.sh      | 28 ++++++++++++++--------------
 t/t9153-git-svn-rewrite-uuid.sh      |  8 ++++----
 8 files changed, 80 insertions(+), 80 deletions(-)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 6e5031f56..0dcf1fa3e 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -21,8 +21,8 @@ test_expect_success \
 
 test_expect_success \
     'checking the commit' \
-    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-    grep "^R100..*path0/COPYING..*path1/COPYING"'
+    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+    grep "^R100..*path0/COPYING..*path1/COPYING" actual'
 
 test_expect_success \
     'moving the file back into subdirectory' \
@@ -35,8 +35,8 @@ test_expect_success \
 
 test_expect_success \
     'checking the commit' \
-    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-    grep "^R100..*path1/COPYING..*path0/COPYING"'
+    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+    grep "^R100..*path1/COPYING..*path0/COPYING" actual'
 
 test_expect_success \
     'checking -k on non-existing file' \
@@ -116,10 +116,10 @@ test_expect_success \
 
 test_expect_success \
     'checking the commit' \
-    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-     grep "^R100..*path0/COPYING..*path2/COPYING" &&
-     git diff-tree -r -M --name-status  HEAD^ HEAD | \
-     grep "^R100..*path0/README..*path2/README"'
+    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+     grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
+     git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+     grep "^R100..*path0/README..*path2/README" actual'
 
 test_expect_success \
     'succeed when source is a prefix of destination' \
@@ -135,10 +135,10 @@ test_expect_success \
 
 test_expect_success \
     'checking the commit' \
-    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-     grep "^R100..*path2/COPYING..*path1/path2/COPYING" &&
-     git diff-tree -r -M --name-status  HEAD^ HEAD | \
-     grep "^R100..*path2/README..*path1/path2/README"'
+    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+     grep "^R100..*path2/COPYING..*path1/path2/COPYING" actual &&
+     git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+     grep "^R100..*path2/README..*path1/path2/README" actual'
 
 test_expect_success \
     'do not move directory over existing directory' \
diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index cd480edf1..284d1224e 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -204,8 +204,8 @@ test_expect_success "follow-parent is atomic" '
 test_expect_success "track multi-parent paths" '
 	svn_cmd cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
 	git svn multi-fetch &&
-	test $(git cat-file commit refs/remotes/glob | \
-	       grep "^parent " | wc -l) -eq 2
+	test $(git cat-file commit refs/remotes/glob >actual &&
+	       grep "^parent " actual | wc -l) -eq 2
 	'
 
 test_expect_success "multi-fetch continues to work" "
diff --git a/t/t9110-git-svn-use-svm-props.sh b/t/t9110-git-svn-use-svm-props.sh
index dde0a3c22..a1a00c298 100755
--- a/t/t9110-git-svn-use-svm-props.sh
+++ b/t/t9110-git-svn-use-svm-props.sh
@@ -21,32 +21,32 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
 
 bar_url=http://mayonaise/svnrepo/bar
 test_expect_success 'verify metadata for /bar' "
-	git cat-file commit refs/remotes/bar | \
-	   grep '^git-svn-id: $bar_url@12 $uuid$' &&
-	git cat-file commit refs/remotes/bar~1 | \
-	   grep '^git-svn-id: $bar_url@11 $uuid$' &&
-	git cat-file commit refs/remotes/bar~2 | \
-	   grep '^git-svn-id: $bar_url@10 $uuid$' &&
-	git cat-file commit refs/remotes/bar~3 | \
-	   grep '^git-svn-id: $bar_url@9 $uuid$' &&
-	git cat-file commit refs/remotes/bar~4 | \
-	   grep '^git-svn-id: $bar_url@6 $uuid$' &&
-	git cat-file commit refs/remotes/bar~5 | \
-	   grep '^git-svn-id: $bar_url@1 $uuid$'
+	git cat-file commit refs/remotes/bar >actual &&
+	   grep '^git-svn-id: $bar_url@12 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~1 >actual &&
+	   grep '^git-svn-id: $bar_url@11 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~2 >actual &&
+	   grep '^git-svn-id: $bar_url@10 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~3 >actual &&
+	   grep '^git-svn-id: $bar_url@9 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~4 >actual &&
+	   grep '^git-svn-id: $bar_url@6 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~5 >actual &&
+	   grep '^git-svn-id: $bar_url@1 $uuid$' actual
 	"
 
 e_url=http://mayonaise/svnrepo/dir/a/b/c/d/e
 test_expect_success 'verify metadata for /dir/a/b/c/d/e' "
-	git cat-file commit refs/remotes/e | \
-	   grep '^git-svn-id: $e_url@1 $uuid$'
+	git cat-file commit refs/remotes/e >actual &&
+	   grep '^git-svn-id: $e_url@1 $uuid$' actual
 	"
 
 dir_url=http://mayonaise/svnrepo/dir
 test_expect_success 'verify metadata for /dir' "
-	git cat-file commit refs/remotes/dir | \
-	   grep '^git-svn-id: $dir_url@2 $uuid$' &&
-	git cat-file commit refs/remotes/dir~1 | \
-	   grep '^git-svn-id: $dir_url@1 $uuid$'
+	git cat-file commit refs/remotes/dir >actual &&
+	   grep '^git-svn-id: $dir_url@2 $uuid$' actual &&
+	git cat-file commit refs/remotes/dir~1 >actual &&
+	   grep '^git-svn-id: $dir_url@1 $uuid$' actual
 	"
 
 test_expect_success 'find commit based on SVN revision number' "
diff --git a/t/t9111-git-svn-use-svnsync-props.sh b/t/t9111-git-svn-use-svnsync-props.sh
index 22b6e5ee7..5306a87f3 100755
--- a/t/t9111-git-svn-use-svnsync-props.sh
+++ b/t/t9111-git-svn-use-svnsync-props.sh
@@ -20,32 +20,32 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
 
 bar_url=http://mayonaise/svnrepo/bar
 test_expect_success 'verify metadata for /bar' "
-	git cat-file commit refs/remotes/bar | \
-	   grep '^git-svn-id: $bar_url@12 $uuid$' &&
-	git cat-file commit refs/remotes/bar~1 | \
-	   grep '^git-svn-id: $bar_url@11 $uuid$' &&
-	git cat-file commit refs/remotes/bar~2 | \
-	   grep '^git-svn-id: $bar_url@10 $uuid$' &&
-	git cat-file commit refs/remotes/bar~3 | \
-	   grep '^git-svn-id: $bar_url@9 $uuid$' &&
-	git cat-file commit refs/remotes/bar~4 | \
-	   grep '^git-svn-id: $bar_url@6 $uuid$' &&
-	git cat-file commit refs/remotes/bar~5 | \
-	   grep '^git-svn-id: $bar_url@1 $uuid$'
+	git cat-file commit refs/remotes/bar >actual &&
+	   grep '^git-svn-id: $bar_url@12 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~1 >actual &&
+	   grep '^git-svn-id: $bar_url@11 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~2 >actual &&
+	   grep '^git-svn-id: $bar_url@10 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~3 >actual &&
+	   grep '^git-svn-id: $bar_url@9 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~4 >actual &&
+	   grep '^git-svn-id: $bar_url@6 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~5 >actual &&
+	   grep '^git-svn-id: $bar_url@1 $uuid$' actual
 	"
 
 e_url=http://mayonaise/svnrepo/dir/a/b/c/d/e
 test_expect_success 'verify metadata for /dir/a/b/c/d/e' "
-	git cat-file commit refs/remotes/e | \
-	   grep '^git-svn-id: $e_url@1 $uuid$'
+	git cat-file commit refs/remotes/e >actual &&
+	   grep '^git-svn-id: $e_url@1 $uuid$' actual
 	"
 
 dir_url=http://mayonaise/svnrepo/dir
 test_expect_success 'verify metadata for /dir' "
-	git cat-file commit refs/remotes/dir | \
-	   grep '^git-svn-id: $dir_url@2 $uuid$' &&
-	git cat-file commit refs/remotes/dir~1 | \
-	   grep '^git-svn-id: $dir_url@1 $uuid$'
+	git cat-file commit refs/remotes/dir >actual &&
+	   grep '^git-svn-id: $dir_url@2 $uuid$' actual &&
+	git cat-file commit refs/remotes/dir~1 >actual &&
+	   grep '^git-svn-id: $dir_url@1 $uuid$' actual
 	"
 
 test_done
diff --git a/t/t9114-git-svn-dcommit-merge.sh b/t/t9114-git-svn-dcommit-merge.sh
index 50bca62de..c945c3758 100755
--- a/t/t9114-git-svn-dcommit-merge.sh
+++ b/t/t9114-git-svn-dcommit-merge.sh
@@ -68,7 +68,7 @@ test_debug 'gitk --all & sleep 1'
 test_expect_success 'verify pre-merge ancestry' "
 	test x\$(git rev-parse --verify refs/heads/svn^2) = \
 	     x\$(git rev-parse --verify refs/heads/merge) &&
-	git cat-file commit refs/heads/svn^ | grep '^friend$'
+	git cat-file commit refs/heads/svn^ >actual && grep '^friend$' actual
 	"
 
 test_expect_success 'git svn dcommit merges' "
@@ -82,12 +82,12 @@ test_expect_success 'verify post-merge ancestry' "
 	     x\$(git rev-parse --verify refs/remotes/origin/trunk) &&
 	test x\$(git rev-parse --verify refs/heads/svn^2) = \
 	     x\$(git rev-parse --verify refs/heads/merge) &&
-	git cat-file commit refs/heads/svn^ | grep '^friend$'
+	git cat-file commit refs/heads/svn^ >actual && grep '^friend$' actual
 	"
 
 test_expect_success 'verify merge commit message' "
-	git rev-list --pretty=raw -1 refs/heads/svn | \
-	  grep \"    Merge branch 'merge' into svn\"
+	git rev-list --pretty=raw -1 refs/heads/svn >actual &&
+	  grep \"    Merge branch 'merge' into svn\" actual
 	"
 
 test_done
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index 41264818c..e12f8cf3b 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -27,10 +27,10 @@ test_expect_success 'imported 2 revisions successfully' '
 	(
 		cd x
 		test "$(git rev-list refs/remotes/git-svn | wc -l)" -eq 2 &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn | \
-		  grep "^author BBBBBBB BBBBBBB <bb@example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 | \
-		  grep "^author AAAAAAA AAAAAAA <aa@example\.com> "
+		git rev-list -1 --pretty=raw refs/remotes/git-svn >actual &&
+		  grep "^author BBBBBBB BBBBBBB <bb@example\.com> " actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 >actual &&
+		  grep "^author AAAAAAA AAAAAAA <aa@example\.com> " actual
 	)
 	'
 
@@ -44,10 +44,10 @@ test_expect_success 'continues to import once authors have been added' '
 		cd x
 		git svn fetch --authors-file=../svn-authors &&
 		test "$(git rev-list refs/remotes/git-svn | wc -l)" -eq 4 &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn | \
-		  grep "^author DDDDDDD DDDDDDD <dd@example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 | \
-		  grep "^author CCCCCCC CCCCCCC <cc@example\.com> "
+		git rev-list -1 --pretty=raw refs/remotes/git-svn >actual &&
+		  grep "^author DDDDDDD DDDDDDD <dd@example\.com> " actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 >actual &&
+		  grep "^author CCCCCCC CCCCCCC <cc@example\.com> " actual
 	)
 	'
 
diff --git a/t/t9138-git-svn-authors-prog.sh b/t/t9138-git-svn-authors-prog.sh
index 7d7e9d46b..5b04c2b40 100755
--- a/t/t9138-git-svn-authors-prog.sh
+++ b/t/t9138-git-svn-authors-prog.sh
@@ -44,24 +44,24 @@ test_expect_success 'imported 6 revisions successfully' '
 test_expect_success 'authors-prog ran correctly' '
 	(
 		cd x
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 | \
-		  grep "^author ee-foo <ee-foo@example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~2 | \
-		  grep "^author dd <dd@sub\.example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~3 | \
-		  grep "^author cc <cc@sub\.example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~4 | \
-		  grep "^author bb <bb@example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~5 | \
-		  grep "^author aa <aa@example\.com> "
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 >actual &&
+		  grep "^author ee-foo <ee-foo@example\.com> " actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~2 >actual &&
+		  grep "^author dd <dd@sub\.example\.com> " actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~3 >actual &&
+		  grep "^author cc <cc@sub\.example\.com> " actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~4 >actual &&
+		  grep "^author bb <bb@example\.com> " actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~5 >actual &&
+		  grep "^author aa <aa@example\.com> " actual
 	)
 '
 
 test_expect_success 'authors-file overrode authors-prog' '
 	(
 		cd x
-		git rev-list -1 --pretty=raw refs/remotes/git-svn | \
-		  grep "^author FFFFFFF FFFFFFF <fFf@other\.example\.com> "
+		git rev-list -1 --pretty=raw refs/remotes/git-svn >actual &&
+		  grep "^author FFFFFFF FFFFFFF <fFf@other\.example\.com> " actual
 	)
 '
 
@@ -73,8 +73,8 @@ test_expect_success 'authors-prog handled special characters in username' '
 	(
 		cd x &&
 		git svn --authors-prog=../svn-authors-prog fetch &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn |
-		grep "^author xyz; touch evil <xyz; touch evil@example\.com> " &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn >actual &&
+		grep "^author xyz; touch evil <xyz; touch evil@example\.com> " actual &&
 		! test -f evil
 	)
 '
diff --git a/t/t9153-git-svn-rewrite-uuid.sh b/t/t9153-git-svn-rewrite-uuid.sh
index 372ef1568..3b00a9135 100755
--- a/t/t9153-git-svn-rewrite-uuid.sh
+++ b/t/t9153-git-svn-rewrite-uuid.sh
@@ -16,10 +16,10 @@ test_expect_success 'load svn repo' "
 	"
 
 test_expect_success 'verify uuid' "
-	git cat-file commit refs/remotes/git-svn~0 | \
-	   grep '^git-svn-id: .*@2 $uuid$' &&
-	git cat-file commit refs/remotes/git-svn~1 | \
-	   grep '^git-svn-id: .*@1 $uuid$'
+	git cat-file commit refs/remotes/git-svn~0 >actual &&
+	   grep '^git-svn-id: .*@2 $uuid$' actual &&
+	git cat-file commit refs/remotes/git-svn~1 >actual &&
+	   grep '^git-svn-id: .*@1 $uuid$' actual
 	"
 
 test_done
-- 
2.16.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
  2018-03-13 20:19 [GSoC] [PATCH] test: avoid pipes in git related commands for test suite Pratik Karki
@ 2018-03-14  7:30 ` Eric Sunshine
  2018-03-14  9:57   ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Sunshine @ 2018-03-14  7:30 UTC (permalink / raw)
  To: Pratik Karki; +Cc: Git List

Thanks for the patch. See comments below...

On Tue, Mar 13, 2018 at 4:19 PM, Pratik Karki <predatoramigo@gmail.com> wrote:
> This patch removes the necessity of pipes in git related commands for test suite.
>
> Exit code of the upstream in a pipe is ignored so, it's use should be avoided. The fix for this is to write the output of the git command to a file and test the exit codes of both the commands being linked by pipe.

Please wrap commit messages to fit in about 72 columns; this one is
far too wide.

On the Git project, commit messages are written in imperative mood, as
if telling the codebase to "do something". So, instead of writing
"This patch removes...", you could word it "Remove..." or "Avoid...".

It's misleading to say that the patch "removes the _necessity_ of
pipes" since pipes were not used out of necessity; they were probably
just a convenience and seemed reasonable at the time, but later
experience has shown that they can be problematic for the reason you
give in the second paragraph.

Taking these observations into consideration, perhaps you could
rewrite the commit message something like this:

    Avoid using pipes downstream of Git commands since the exit codes
    of commands upstream of pipes get swallowed, thus potentially
    hiding failure of those commands. Instead, capture Git command
    output to a file apply the downstream command(s) to that file.

More comments below...

> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> @@ -116,10 +116,10 @@ test_expect_success \
>  test_expect_success \
>      'checking the commit' \
> -    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
> -     grep "^R100..*path0/COPYING..*path2/COPYING" &&
> -     git diff-tree -r -M --name-status  HEAD^ HEAD | \
> -     grep "^R100..*path0/README..*path2/README"'
> +    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
> +     grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
> +     git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
> +     grep "^R100..*path0/README..*path2/README" actual'

Although this "mechanical" transformation is technically correct, it
is nevertheless wasteful. The exact same "git diff-tree ..." command
is run twice, and both times output is captured to file 'actual',
which makes the second invocation superfluous. Instead, a better
transformation would be:

    git diff-tree ... >actual &&
    grep ... actual &&
    grep ... actual

The same observation applies to other transformations in this patch.

> diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
> @@ -204,8 +204,8 @@ test_expect_success "follow-parent is atomic" '
>  test_expect_success "track multi-parent paths" '
>         svn_cmd cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
>         git svn multi-fetch &&
> -       test $(git cat-file commit refs/remotes/glob | \
> -              grep "^parent " | wc -l) -eq 2
> +       test $(git cat-file commit refs/remotes/glob >actual &&
> +              grep "^parent " actual | wc -l) -eq 2
>         '

This is not a great transformation. If "git cat-file" fails, then
neither 'grep' nor 'wc' will run, and the result will be as if 'test'
was called without an argument before "-eq". For example:

    % test $(false >actual && grep "^parent " actual | wc -l) -eq 2
    test: -eq: unary operator expected

It would be better to run "git cat-file" outside of "test $(...)". For instance:

    git cat-file ... >actual &&
    test $(grep ... actual | wc -l) -eq 2

Alternately, you could take advantage of the test_line_count() helper function:

    git cat-file ... >actual &&
    grep ... actual >actual2 &&
    test_line_count = 2 actual2

> diff --git a/t/t9110-git-svn-use-svm-props.sh b/t/t9110-git-svn-use-svm-props.sh
> @@ -21,32 +21,32 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
>  test_expect_success 'verify metadata for /bar' "
> -       git cat-file commit refs/remotes/bar | \
> -          grep '^git-svn-id: $bar_url@12 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~1 | \
> -          grep '^git-svn-id: $bar_url@11 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~2 | \
> -          grep '^git-svn-id: $bar_url@10 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~3 | \
> -          grep '^git-svn-id: $bar_url@9 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~4 | \
> -          grep '^git-svn-id: $bar_url@6 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~5 | \
> -          grep '^git-svn-id: $bar_url@1 $uuid$'
> +       git cat-file commit refs/remotes/bar >actual &&
> +          grep '^git-svn-id: $bar_url@12 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~1 >actual &&
> +          grep '^git-svn-id: $bar_url@11 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~2 >actual &&
> +          grep '^git-svn-id: $bar_url@10 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~3 >actual &&
> +          grep '^git-svn-id: $bar_url@9 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~4 >actual &&
> +          grep '^git-svn-id: $bar_url@6 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~5 >actual &&
> +          grep '^git-svn-id: $bar_url@1 $uuid$' actual
>         "

An indented line in the original shows that it is a continuation of
the preceding line. However, in the revised code, that is not so, thus
it probably makes sense to drop the indentation.

The same comment applies to several additional cases snipped from this reply.

> diff --git a/t/t9114-git-svn-dcommit-merge.sh b/t/t9114-git-svn-dcommit-merge.sh
> index 50bca62de..c945c3758 100755
> --- a/t/t9114-git-svn-dcommit-merge.sh
> +++ b/t/t9114-git-svn-dcommit-merge.sh
> @@ -68,7 +68,7 @@ test_debug 'gitk --all & sleep 1'
>  test_expect_success 'verify pre-merge ancestry' "
>         test x\$(git rev-parse --verify refs/heads/svn^2) = \
>              x\$(git rev-parse --verify refs/heads/merge) &&
> -       git cat-file commit refs/heads/svn^ | grep '^friend$'
> +       git cat-file commit refs/heads/svn^ >actual && grep '^friend$' actual
>         "

Style: split the line at the '&&'...

    git cat-file ... >actual &&
    grep ... actual

The same comment applies to another test snipped from this reply.

Aside: The current patch wants to solve the problem of exit code being
swallowed down pipes, however, this and other tests are afflicted by a
similar problem with $(...) also swallowing the exit code. A failure
of "git rev-parse" could potentially go unnoticed inside "test
x$(...)". Fixing that is outside the scope of the current patch,
however, a follow-on patch to fix that problem (if you feel so
inclined) might transform it something like this:

    git rev-parse ... >rev1 &&
    git rev-parse ... >rev2 &&
    test_cmp rev1 rev2 &&

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
  2018-03-14  7:30 ` Eric Sunshine
@ 2018-03-14  9:57   ` Ævar Arnfjörð Bjarmason
  2018-03-14 18:22     ` Eric Sunshine
  0 siblings, 1 reply; 7+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-03-14  9:57 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Pratik Karki, Git List


On Wed, Mar 14 2018, Eric Sunshine jotted:

> Thanks for the patch. See comments below...
>
> On Tue, Mar 13, 2018 at 4:19 PM, Pratik Karki <predatoramigo@gmail.com> wrote:
>> This patch removes the necessity of pipes in git related commands for test suite.
>>
>> Exit code of the upstream in a pipe is ignored so, it's use should be avoided. The fix for this is to write the output of the git command to a file and test the exit codes of both the commands being linked by pipe.
>
> Please wrap commit messages to fit in about 72 columns; this one is
> far too wide.
>
> On the Git project, commit messages are written in imperative mood, as
> if telling the codebase to "do something". So, instead of writing
> "This patch removes...", you could word it "Remove..." or "Avoid...".
>
> It's misleading to say that the patch "removes the _necessity_ of
> pipes" since pipes were not used out of necessity; they were probably
> just a convenience and seemed reasonable at the time, but later
> experience has shown that they can be problematic for the reason you
> give in the second paragraph.
>
> Taking these observations into consideration, perhaps you could
> rewrite the commit message something like this:
>
>     Avoid using pipes downstream of Git commands since the exit codes
>     of commands upstream of pipes get swallowed, thus potentially
>     hiding failure of those commands. Instead, capture Git command
>     output to a file apply the downstream command(s) to that file.
>
> More comments below...

Makes sense.

>> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
>> ---
>> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
>> @@ -116,10 +116,10 @@ test_expect_success \
>>  test_expect_success \
>>      'checking the commit' \
>> -    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
>> -     grep "^R100..*path0/COPYING..*path2/COPYING" &&
>> -     git diff-tree -r -M --name-status  HEAD^ HEAD | \
>> -     grep "^R100..*path0/README..*path2/README"'
>> +    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
>> +     grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
>> +     git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
>> +     grep "^R100..*path0/README..*path2/README" actual'
>
> Although this "mechanical" transformation is technically correct, it
> is nevertheless wasteful. The exact same "git diff-tree ..." command
> is run twice, and both times output is captured to file 'actual',
> which makes the second invocation superfluous. Instead, a better
> transformation would be:
>
>     git diff-tree ... >actual &&
>     grep ... actual &&
>     grep ... actual
>
> The same observation applies to other transformations in this patch.

I think we have to be careful to not be overly picky with rejecting
mechanical transformations that fix bugs on the basis that while we're
at it the test could also be rewritten.

I.e. this bug was there before, maybe we should purely focus on just
replacing the harmful pipe pattern that hides errors in this series and
leave rewriting the actual test logic for a later patch.

>> diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
>> @@ -204,8 +204,8 @@ test_expect_success "follow-parent is atomic" '
>>  test_expect_success "track multi-parent paths" '
>>         svn_cmd cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
>>         git svn multi-fetch &&
>> -       test $(git cat-file commit refs/remotes/glob | \
>> -              grep "^parent " | wc -l) -eq 2
>> +       test $(git cat-file commit refs/remotes/glob >actual &&
>> +              grep "^parent " actual | wc -l) -eq 2
>>         '
>
> This is not a great transformation. If "git cat-file" fails, then
> neither 'grep' nor 'wc' will run, and the result will be as if 'test'
> was called without an argument before "-eq". For example:
>
>     % test $(false >actual && grep "^parent " actual | wc -l) -eq 2
>     test: -eq: unary operator expected
>
> It would be better to run "git cat-file" outside of "test $(...)". For instance:
>
>     git cat-file ... >actual &&
>     test $(grep ... actual | wc -l) -eq 2
>
> Alternately, you could take advantage of the test_line_count() helper function:
>
>     git cat-file ... >actual &&
>     grep ... actual >actual2 &&
>     test_line_count = 2 actual2

In this case though as you rightly point out the rewrite is introducing
a regression, which should definitely be fixed.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
  2018-03-14  9:57   ` Ævar Arnfjörð Bjarmason
@ 2018-03-14 18:22     ` Eric Sunshine
  2018-03-15 17:04       ` Junio C Hamano
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Sunshine @ 2018-03-14 18:22 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Pratik Karki, Git List

On Wed, Mar 14, 2018 at 5:57 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Wed, Mar 14 2018, Eric Sunshine jotted:
>> On Tue, Mar 13, 2018 at 4:19 PM, Pratik Karki <predatoramigo@gmail.com> wrote:
>>> -    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
>>> -     grep "^R100..*path0/COPYING..*path2/COPYING" &&
>>> -     git diff-tree -r -M --name-status  HEAD^ HEAD | \
>>> -     grep "^R100..*path0/README..*path2/README"'
>>> +    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
>>> +     grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
>>> +     git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
>>> +     grep "^R100..*path0/README..*path2/README" actual'
>>
>> Although this "mechanical" transformation is technically correct, it
>> is nevertheless wasteful. The exact same "git diff-tree ..." command
>> is run twice, and both times output is captured to file 'actual',
>> which makes the second invocation superfluous. Instead, a better
>> transformation would be:
>>
>>     git diff-tree ... >actual &&
>>     grep ... actual &&
>>     grep ... actual
>>
> I think we have to be careful to not be overly picky with rejecting
> mechanical transformations that fix bugs on the basis that while we're
> at it the test could also be rewritten.
>
> I.e. this bug was there before, maybe we should purely focus on just
> replacing the harmful pipe pattern that hides errors in this series and
> leave rewriting the actual test logic for a later patch.

Thanks for presenting an opposing opinion. While I understand your
position, the reason for my suggested transformation is that if the
patch already transformed the code in the way suggested, it would
increase my confidence, as a reviewer, that the patch author had
_studied_ and _understood_ the code. Increased confidence is
especially important for mechanical transformations since -- as seen
in the unsnipped review comment below -- blindly-applied mechanical
transformations can be suboptimal or outright incorrect.

It's also the sort of review comment I would make even to very
seasoned project participants[1].

[1]: https://public-inbox.org/git/CAPig+cQLmYQeRhPxvZHmY7gApnbE25H_KoSWs-ZjuBo4BruimQ@mail.gmail.com/

>>> -       test $(git cat-file commit refs/remotes/glob | \
>>> -              grep "^parent " | wc -l) -eq 2
>>> +       test $(git cat-file commit refs/remotes/glob >actual &&
>>> +              grep "^parent " actual | wc -l) -eq 2
>>
>> This is not a great transformation. If "git cat-file" fails, then
>> neither 'grep' nor 'wc' will run, and the result will be as if 'test'
>> was called without an argument before "-eq". For example:
>>
>>     % test $(false >actual && grep "^parent " actual | wc -l) -eq 2
>>     test: -eq: unary operator expected
>>
>> It would be better to run "git cat-file" outside of "test $(...)". For instance:
>>
>>     git cat-file ... >actual &&
>>     test $(grep ... actual | wc -l) -eq 2
>>
>> Alternately, you could take advantage of the test_line_count() helper function:
>>
>>     git cat-file ... >actual &&
>>     grep ... actual >actual2 &&
>>     test_line_count = 2 actual2
>
> In this case though as you rightly point out the rewrite is introducing
> a regression, which should definitely be fixed.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
  2018-03-14 18:22     ` Eric Sunshine
@ 2018-03-15 17:04       ` Junio C Hamano
  2018-03-19 17:32         ` [GSoC][PATCH] " Pratik Karki
  0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2018-03-15 17:04 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Ævar Arnfjörð Bjarmason, Pratik Karki, Git List

Eric Sunshine <sunshine@sunshineco.com> writes:

> Thanks for presenting an opposing opinion. While I understand your
> position, the reason for my suggested transformation is that if the
> patch already transformed the code in the way suggested, it would
> increase my confidence, as a reviewer, that the patch author had
> _studied_ and _understood_ the code. Increased confidence is
> especially important for mechanical transformations since -- as seen
> in the unsnipped review comment below -- blindly-applied mechanical
> transformations can be suboptimal or outright incorrect.
>
> It's also the sort of review comment I would make even to very
> seasoned project participants[1].
>
> [1]: https://public-inbox.org/git/CAPig+cQLmYQeRhPxvZHmY7gApnbE25H_KoSWs-ZjuBo4BruimQ@mail.gmail.com/

Yes, it is a good example that mechanical conversions are often
mind-numbing and make even seasoned participants miss trivially
obvious improvement opportunities ;-)

It however is OK to be more lenient to newer participants and allow
deferring such "while at it, make it right" on top of "minimally
required for correctness", in order to encourage them by getting
something to the tree early ;-)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [GSoC][PATCH] test: avoid pipes in git related commands for test suite
  2018-03-15 17:04       ` Junio C Hamano
@ 2018-03-19 17:32         ` " Pratik Karki
  2018-03-21 11:02           ` Eric Sunshine
  0 siblings, 1 reply; 7+ messages in thread
From: Pratik Karki @ 2018-03-19 17:32 UTC (permalink / raw)
  To: git; +Cc: Pratik Karki

Thank you Eric Sunshine,

I have done as you had instructed me. I look forward to more
understanding of the codebase and would love to fix
"git rev-parse" problems in my follow-on patches.
Thank you for the professional review comment.

Sorry for late follow-on patch, I got tied up with my university stuffs.

Please do review this patch as before. I will correct it if needed.

Cheers,
Pratik Karki

Avoid using pipes downstream of Git commands since the exit codes
of commands upstream of pipes get swallowed, thus potentially
hiding failure of those commands. Instead, capture Git command
output to a file apply the downstream command(s) to that file.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 t/t5300-pack-object.sh                     | 10 +++---
 t/t5510-fetch.sh                           |  8 ++---
 t/t7001-mv.sh                              | 22 ++++++-------
 t/t7003-filter-branch.sh                   |  9 ++++--
 t/t9104-git-svn-follow-parent.sh           | 16 +++++----
 t/t9108-git-svn-glob.sh                    | 14 ++++----
 t/t9109-git-svn-multi-glob.sh              | 28 +++++++++-------
 t/t9110-git-svn-use-svm-props.sh           | 42 ++++++++++++------------
 t/t9111-git-svn-use-svnsync-props.sh       | 36 ++++++++++-----------
 t/t9114-git-svn-dcommit-merge.sh           | 10 +++---
 t/t9130-git-svn-authors-file.sh            | 28 +++++++++-------
 t/t9138-git-svn-authors-prog.sh            | 31 +++++++++---------
 t/t9153-git-svn-rewrite-uuid.sh            |  8 ++---
 t/t9168-git-svn-partially-globbed-names.sh | 34 +++++++++++--------
 t/t9350-fast-export.sh                     | 52 ++++++++++++++++--------------
 15 files changed, 187 insertions(+), 161 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 9c68b9925..91207ae0c 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -311,9 +311,9 @@ test_expect_success 'unpacking with --strict' '
 	rm -f .git/index &&
 	tail -n 10 LIST | git update-index --index-info &&
 	ST=$(git write-tree) &&
-	PACK5=$( git rev-list --objects "$LIST" "$LI" "$ST" | \
-		git pack-objects test-5 ) &&
-	PACK6=$( (
+	git rev-list --objects "$LIST" "$LI" "$ST" >actual &&
+	PACK5=$(git pack-objects test-5 < actual) &&
+	PACK6=$((
 			echo "$LIST"
 			echo "$LI"
 			echo "$ST"
@@ -358,8 +358,8 @@ test_expect_success 'index-pack with --strict' '
 	rm -f .git/index &&
 	tail -n 10 LIST | git update-index --index-info &&
 	ST=$(git write-tree) &&
-	PACK5=$( git rev-list --objects "$LIST" "$LI" "$ST" | \
-		git pack-objects test-5 ) &&
+	git rev-list --objects "$LIST" "$LI" "$ST" >actual &&
+	PACK5=$(git pack-objects test-5 < actual) &&
 	PACK6=$( (
 			echo "$LIST"
 			echo "$LI"
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 668c54be4..c7b284138 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -693,8 +693,8 @@ test_expect_success C_LOCALE_OUTPUT 'fetch aligned output' '
 	test_commit looooooooooooong-tag &&
 	(
 		cd full-output &&
-		git -c fetch.output=full fetch origin 2>&1 | \
-			grep -e "->" | cut -c 22- >../actual
+		git -c fetch.output=full fetch origin >actual2 2>&1 &&
+		grep -e "->" actual2 | cut -c 22- >../actual
 	) &&
 	cat >expect <<-\EOF &&
 	master               -> origin/master
@@ -708,8 +708,8 @@ test_expect_success C_LOCALE_OUTPUT 'fetch compact output' '
 	test_commit extraaa &&
 	(
 		cd compact &&
-		git -c fetch.output=compact fetch origin 2>&1 | \
-			grep -e "->" | cut -c 22- >../actual
+		git -c fetch.output=compact fetch origin >actual2 2>&1 &&
+		grep -e "->" actual2 | cut -c 22- >../actual
 	) &&
 	cat >expect <<-\EOF &&
 	master     -> origin/*
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 6e5031f56..00aa9e45b 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -21,8 +21,8 @@ test_expect_success \
 
 test_expect_success \
     'checking the commit' \
-    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-    grep "^R100..*path0/COPYING..*path1/COPYING"'
+    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+    grep "^R100..*path0/COPYING..*path1/COPYING" actual'
 
 test_expect_success \
     'moving the file back into subdirectory' \
@@ -35,8 +35,8 @@ test_expect_success \
 
 test_expect_success \
     'checking the commit' \
-    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-    grep "^R100..*path1/COPYING..*path0/COPYING"'
+    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+    grep "^R100..*path1/COPYING..*path0/COPYING" actual'
 
 test_expect_success \
     'checking -k on non-existing file' \
@@ -116,10 +116,9 @@ test_expect_success \
 
 test_expect_success \
     'checking the commit' \
-    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-     grep "^R100..*path0/COPYING..*path2/COPYING" &&
-     git diff-tree -r -M --name-status  HEAD^ HEAD | \
-     grep "^R100..*path0/README..*path2/README"'
+    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+     grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
+     grep "^R100..*path0/README..*path2/README" actual'
 
 test_expect_success \
     'succeed when source is a prefix of destination' \
@@ -135,10 +134,9 @@ test_expect_success \
 
 test_expect_success \
     'checking the commit' \
-    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-     grep "^R100..*path2/COPYING..*path1/path2/COPYING" &&
-     git diff-tree -r -M --name-status  HEAD^ HEAD | \
-     grep "^R100..*path2/README..*path1/path2/README"'
+    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+     grep "^R100..*path2/COPYING..*path1/path2/COPYING" actual &&
+     grep "^R100..*path2/README..*path1/path2/README" actual'
 
 test_expect_success \
     'do not move directory over existing directory' \
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 7cb60799b..82c9c2825 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -187,7 +187,8 @@ test_expect_success 'author information is preserved' '
 			test \$GIT_COMMIT != $(git rev-parse master) || \
 			echo Hallo" \
 		preserved-author) &&
-	test 1 = $(git rev-list --author="B V Uips" preserved-author | wc -l)
+	git rev-list --author="B V Uips" preserved-author > actual &&
+	test 1 = $(wc -l < actual)
 '
 
 test_expect_success "remove a certain author's commits" '
@@ -205,7 +206,8 @@ test_expect_success "remove a certain author's commits" '
 	cnt1=$(git rev-list master | wc -l) &&
 	cnt2=$(git rev-list removed-author | wc -l) &&
 	test $cnt1 -eq $(($cnt2 + 1)) &&
-	test 0 = $(git rev-list --author="B V Uips" removed-author | wc -l)
+	git rev-list --author="B V Uips" removed-author >actual &&
+	test 0 = $(wc -l < actual)
 '
 
 test_expect_success 'barf on invalid name' '
@@ -258,7 +260,8 @@ test_expect_success 'Subdirectory filter with disappearing trees' '
 	git commit -m "Re-adding foo" &&
 
 	git filter-branch -f --subdirectory-filter foo &&
-	test $(git rev-list master | wc -l) = 3
+	git rev-list master >actual &&
+	test_line_count = 3 actual
 '
 
 test_expect_success 'Tag name filtering retains tag message' '
diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index cd480edf1..a532c49af 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -33,8 +33,8 @@ test_expect_success 'init and fetch a moved directory' '
 	git svn fetch -i thunk &&
 	test "$(git rev-parse --verify refs/remotes/thunk@2)" \
 	   = "$(git rev-parse --verify refs/remotes/thunk~1)" &&
-	test "$(git cat-file blob refs/remotes/thunk:readme |\
-		 sed -n -e "3p")" = goodbye &&
+	git cat-file blob refs/remotes/thunk:readme >actual &&
+	test "$(sed -n -e "3p" actual)" = goodbye &&
 	test -z "$(git config --get svn-remote.svn.fetch \
 		 "^trunk:refs/remotes/thunk@2$")"
 	'
@@ -48,8 +48,8 @@ test_expect_success 'init and fetch from one svn-remote' '
         git svn fetch -i svn/thunk &&
 	test "$(git rev-parse --verify refs/remotes/svn/trunk)" \
 	   = "$(git rev-parse --verify refs/remotes/svn/thunk~1)" &&
-	test "$(git cat-file blob refs/remotes/svn/thunk:readme |\
-		 sed -n -e "3p")" = goodbye
+	git cat-file blob refs/remotes/svn/thunk:readme >actual &&
+	test "$(sed -n -e "3p" actual)" = goodbye
         '
 
 test_expect_success 'follow deleted parent' '
@@ -107,7 +107,8 @@ test_expect_success 'follow deleted directory' '
 	git svn init --minimize-url -i glob "$svnrepo"/glob &&
 	git svn fetch -i glob &&
 	test "$(git cat-file blob refs/remotes/glob:blob/bye)" = hi &&
-	test "$(git ls-tree refs/remotes/glob | wc -l )" -eq 1
+	git ls-tree refs/remotes/glob >actual &&
+	test_line_count = 1 actual
 	'
 
 # ref: r9270 of the Subversion repository: (http://svn.collab.net/repos/svn)
@@ -204,8 +205,9 @@ test_expect_success "follow-parent is atomic" '
 test_expect_success "track multi-parent paths" '
 	svn_cmd cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
 	git svn multi-fetch &&
-	test $(git cat-file commit refs/remotes/glob | \
-	       grep "^parent " | wc -l) -eq 2
+	git cat-file commit refs/remotes/glob >actual &&
+	grep "^parent " actual > actual2 &&
+	test_line_count = 2 actual2
 	'
 
 test_expect_success "multi-fetch continues to work" "
diff --git a/t/t9108-git-svn-glob.sh b/t/t9108-git-svn-glob.sh
index a94286c8e..e01f3553f 100755
--- a/t/t9108-git-svn-glob.sh
+++ b/t/t9108-git-svn-glob.sh
@@ -47,8 +47,8 @@ test_expect_success 'test refspec globbing' '
 	git config --add svn-remote.svn.tags\
 	                 "tags/*/src/a:refs/remotes/tags/*" &&
 	git svn multi-fetch &&
-	git log --pretty=oneline refs/remotes/tags/end | \
-	    sed -e "s/^.\{41\}//" > output.end &&
+	git log --pretty=oneline refs/remotes/tags/end >actual &&
+	sed -e "s/^.\{41\}//" actual > output.end &&
 	test_cmp expect.end output.end &&
 	test "$(git rev-parse refs/remotes/tags/end~1)" = \
 		"$(git rev-parse refs/remotes/branches/start)" &&
@@ -75,14 +75,16 @@ test_expect_success 'test left-hand-side only globbing' '
 		svn_cmd commit -m "try to try"
 	) &&
 	git svn fetch two &&
-	test $(git rev-list refs/remotes/two/tags/end | wc -l) -eq 6 &&
-	test $(git rev-list refs/remotes/two/branches/start | wc -l) -eq 3 &&
+	git rev-list refs/remotes/two/tags/end >actual &&
+	test_line_count = 6 actual &&
+	git rev-list refs/remotes/two/branches/start >actual2 &&
+	test_line_count = 3 actual2 &&
 	test $(git rev-parse refs/remotes/two/branches/start~2) = \
 	     $(git rev-parse refs/remotes/two/trunk) &&
 	test $(git rev-parse refs/remotes/two/tags/end~3) = \
 	     $(git rev-parse refs/remotes/two/branches/start) &&
-	git log --pretty=oneline refs/remotes/two/tags/end | \
-	    sed -e "s/^.\{41\}//" > output.two &&
+	git log --pretty=oneline refs/remotes/two/tags/end >actual3 &&
+	sed -e "s/^.\{41\}//" actual3 > output.two &&
 	test_cmp expect.two output.two
 	'
 
diff --git a/t/t9109-git-svn-multi-glob.sh b/t/t9109-git-svn-multi-glob.sh
index 8d99e848d..a09fc3e14 100755
--- a/t/t9109-git-svn-multi-glob.sh
+++ b/t/t9109-git-svn-multi-glob.sh
@@ -47,8 +47,8 @@ test_expect_success 'test refspec globbing' '
 	git config --add svn-remote.svn.tags\
 	                 "tags/*/src/a:refs/remotes/tags/*" &&
 	git svn multi-fetch &&
-	git log --pretty=oneline refs/remotes/tags/end | \
-	    sed -e "s/^.\{41\}//" > output.end &&
+	git log --pretty=oneline refs/remotes/tags/end >actual &&
+	sed -e "s/^.\{41\}//" actual > output.end &&
 	test_cmp expect.end output.end &&
 	test "$(git rev-parse refs/remotes/tags/end~1)" = \
 		"$(git rev-parse refs/remotes/branches/v1/start)" &&
@@ -75,14 +75,16 @@ test_expect_success 'test left-hand-side only globbing' '
 		svn_cmd commit -m "try to try"
 	) &&
 	git svn fetch two &&
-	test $(git rev-list refs/remotes/two/tags/end | wc -l) -eq 6 &&
-	test $(git rev-list refs/remotes/two/branches/v1/start | wc -l) -eq 3 &&
+	git rev-list refs/remotes/two/tags/end >actual2 &&
+	test_line_count = 6 actual2 &&
+	git rev-list refs/remotes/two/branches/v1/start >actual3 &&
+	test_line_count = 3 actual3 &&
 	test $(git rev-parse refs/remotes/two/branches/v1/start~2) = \
 	     $(git rev-parse refs/remotes/two/trunk) &&
 	test $(git rev-parse refs/remotes/two/tags/end~3) = \
 	     $(git rev-parse refs/remotes/two/branches/v1/start) &&
-	git log --pretty=oneline refs/remotes/two/tags/end | \
-	    sed -e "s/^.\{41\}//" > output.two &&
+	git log --pretty=oneline refs/remotes/two/tags/end >actual4 &&
+	sed -e "s/^.\{41\}//" actual4 > output.two &&
 	test_cmp expect.two output.two
 	'
 cat > expect.four <<EOF
@@ -120,18 +122,20 @@ test_expect_success 'test another branch' '
 	git config --add svn-remote.four.url "$svnrepo" &&
 	git config --add svn-remote.four.fetch trunk:refs/remotes/four/trunk &&
 	git config --add svn-remote.four.branches \
-	                 "branches/*/*:refs/remotes/four/branches/*/*" &&
+			 "branches/*/*:refs/remotes/four/branches/*/*" &&
 	git config --add svn-remote.four.tags \
-	                 "tags/*:refs/remotes/four/tags/*" &&
+			 "tags/*:refs/remotes/four/tags/*" &&
 	git svn fetch four &&
-	test $(git rev-list refs/remotes/four/tags/next | wc -l) -eq 5 &&
-	test $(git rev-list refs/remotes/four/branches/v2/start | wc -l) -eq 3 &&
+	git rev-list refs/remotes/four/tags/next >actual &&
+	test_line_count = 5 actual &&
+	git rev-list refs/remotes/four/branches/v2/start >actual2 &&
+	test_line_count = 3 actual2 &&
 	test $(git rev-parse refs/remotes/four/branches/v2/start~2) = \
 	     $(git rev-parse refs/remotes/four/trunk) &&
 	test $(git rev-parse refs/remotes/four/tags/next~2) = \
 	     $(git rev-parse refs/remotes/four/branches/v2/start) &&
-	git log --pretty=oneline refs/remotes/four/tags/next | \
-	    sed -e "s/^.\{41\}//" > output.four &&
+	git log --pretty=oneline refs/remotes/four/tags/next >actual3 &&
+	sed -e "s/^.\{41\}//" actual3 > output.four &&
 	test_cmp expect.four output.four
 	'
 
diff --git a/t/t9110-git-svn-use-svm-props.sh b/t/t9110-git-svn-use-svm-props.sh
index dde0a3c22..4200b567f 100755
--- a/t/t9110-git-svn-use-svm-props.sh
+++ b/t/t9110-git-svn-use-svm-props.sh
@@ -21,38 +21,38 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
 
 bar_url=http://mayonaise/svnrepo/bar
 test_expect_success 'verify metadata for /bar' "
-	git cat-file commit refs/remotes/bar | \
-	   grep '^git-svn-id: $bar_url@12 $uuid$' &&
-	git cat-file commit refs/remotes/bar~1 | \
-	   grep '^git-svn-id: $bar_url@11 $uuid$' &&
-	git cat-file commit refs/remotes/bar~2 | \
-	   grep '^git-svn-id: $bar_url@10 $uuid$' &&
-	git cat-file commit refs/remotes/bar~3 | \
-	   grep '^git-svn-id: $bar_url@9 $uuid$' &&
-	git cat-file commit refs/remotes/bar~4 | \
-	   grep '^git-svn-id: $bar_url@6 $uuid$' &&
-	git cat-file commit refs/remotes/bar~5 | \
-	   grep '^git-svn-id: $bar_url@1 $uuid$'
+	git cat-file commit refs/remotes/bar >actual &&
+	grep '^git-svn-id: $bar_url@12 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~1 >actual1 &&
+	grep '^git-svn-id: $bar_url@11 $uuid$' actual1 &&
+	git cat-file commit refs/remotes/bar~2 >actual2 &&
+	grep '^git-svn-id: $bar_url@10 $uuid$' actual2 &&
+	git cat-file commit refs/remotes/bar~3 >actual3 &&
+	grep '^git-svn-id: $bar_url@9 $uuid$' actual3 &&
+	git cat-file commit refs/remotes/bar~4 >actual4 &&
+	grep '^git-svn-id: $bar_url@6 $uuid$' actual4 &&
+	git cat-file commit refs/remotes/bar~5 >actual5 &&
+	grep '^git-svn-id: $bar_url@1 $uuid$' actual5
 	"
 
 e_url=http://mayonaise/svnrepo/dir/a/b/c/d/e
 test_expect_success 'verify metadata for /dir/a/b/c/d/e' "
-	git cat-file commit refs/remotes/e | \
-	   grep '^git-svn-id: $e_url@1 $uuid$'
+	git cat-file commit refs/remotes/e >actual &&
+	grep '^git-svn-id: $e_url@1 $uuid$' actual
 	"
 
 dir_url=http://mayonaise/svnrepo/dir
 test_expect_success 'verify metadata for /dir' "
-	git cat-file commit refs/remotes/dir | \
-	   grep '^git-svn-id: $dir_url@2 $uuid$' &&
-	git cat-file commit refs/remotes/dir~1 | \
-	   grep '^git-svn-id: $dir_url@1 $uuid$'
+	git cat-file commit refs/remotes/dir >actual &&
+	grep '^git-svn-id: $dir_url@2 $uuid$' actual &&
+	git cat-file commit refs/remotes/dir~1 >actual1 &&
+	grep '^git-svn-id: $dir_url@1 $uuid$' actual1
 	"
 
 test_expect_success 'find commit based on SVN revision number' "
-        git svn find-rev r12 |
-	    grep $(git rev-parse HEAD)
-        "
+	git svn find-rev r12 >actual &&
+	grep $(git rev-parse HEAD) actual
+	"
 
 test_expect_success 'empty rebase' "
 	git svn rebase
diff --git a/t/t9111-git-svn-use-svnsync-props.sh b/t/t9111-git-svn-use-svnsync-props.sh
index 22b6e5ee7..a4225c9f6 100755
--- a/t/t9111-git-svn-use-svnsync-props.sh
+++ b/t/t9111-git-svn-use-svnsync-props.sh
@@ -20,32 +20,32 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
 
 bar_url=http://mayonaise/svnrepo/bar
 test_expect_success 'verify metadata for /bar' "
-	git cat-file commit refs/remotes/bar | \
-	   grep '^git-svn-id: $bar_url@12 $uuid$' &&
-	git cat-file commit refs/remotes/bar~1 | \
-	   grep '^git-svn-id: $bar_url@11 $uuid$' &&
-	git cat-file commit refs/remotes/bar~2 | \
-	   grep '^git-svn-id: $bar_url@10 $uuid$' &&
-	git cat-file commit refs/remotes/bar~3 | \
-	   grep '^git-svn-id: $bar_url@9 $uuid$' &&
-	git cat-file commit refs/remotes/bar~4 | \
-	   grep '^git-svn-id: $bar_url@6 $uuid$' &&
-	git cat-file commit refs/remotes/bar~5 | \
-	   grep '^git-svn-id: $bar_url@1 $uuid$'
+	git cat-file commit refs/remotes/bar >actual &&
+	grep '^git-svn-id: $bar_url@12 $uuid$' actual &&
+	git cat-file commit refs/remotes/bar~1 >actual1 &&
+	grep '^git-svn-id: $bar_url@11 $uuid$' actual1 &&
+	git cat-file commit refs/remotes/bar~2 >actual2 &&
+	grep '^git-svn-id: $bar_url@10 $uuid$' actual2 &&
+	git cat-file commit refs/remotes/bar~3 >actual3 &&
+	grep '^git-svn-id: $bar_url@9 $uuid$' actual3 &&
+	git cat-file commit refs/remotes/bar~4 >actual4 &&
+	grep '^git-svn-id: $bar_url@6 $uuid$' actual4 &&
+	git cat-file commit refs/remotes/bar~5 >actual5 &&
+	grep '^git-svn-id: $bar_url@1 $uuid$' actual5
 	"
 
 e_url=http://mayonaise/svnrepo/dir/a/b/c/d/e
 test_expect_success 'verify metadata for /dir/a/b/c/d/e' "
-	git cat-file commit refs/remotes/e | \
-	   grep '^git-svn-id: $e_url@1 $uuid$'
+	git cat-file commit refs/remotes/e >actual &&
+	grep '^git-svn-id: $e_url@1 $uuid$' actual
 	"
 
 dir_url=http://mayonaise/svnrepo/dir
 test_expect_success 'verify metadata for /dir' "
-	git cat-file commit refs/remotes/dir | \
-	   grep '^git-svn-id: $dir_url@2 $uuid$' &&
-	git cat-file commit refs/remotes/dir~1 | \
-	   grep '^git-svn-id: $dir_url@1 $uuid$'
+	git cat-file commit refs/remotes/dir >actual &&
+	grep '^git-svn-id: $dir_url@2 $uuid$' actual &&
+	git cat-file commit refs/remotes/dir~1 >actual1 &&
+	grep '^git-svn-id: $dir_url@1 $uuid$' actual1
 	"
 
 test_done
diff --git a/t/t9114-git-svn-dcommit-merge.sh b/t/t9114-git-svn-dcommit-merge.sh
index 50bca62de..32317d6bc 100755
--- a/t/t9114-git-svn-dcommit-merge.sh
+++ b/t/t9114-git-svn-dcommit-merge.sh
@@ -68,7 +68,8 @@ test_debug 'gitk --all & sleep 1'
 test_expect_success 'verify pre-merge ancestry' "
 	test x\$(git rev-parse --verify refs/heads/svn^2) = \
 	     x\$(git rev-parse --verify refs/heads/merge) &&
-	git cat-file commit refs/heads/svn^ | grep '^friend$'
+	git cat-file commit refs/heads/svn^ >actual &&
+	grep '^friend$' actual
 	"
 
 test_expect_success 'git svn dcommit merges' "
@@ -82,12 +83,13 @@ test_expect_success 'verify post-merge ancestry' "
 	     x\$(git rev-parse --verify refs/remotes/origin/trunk) &&
 	test x\$(git rev-parse --verify refs/heads/svn^2) = \
 	     x\$(git rev-parse --verify refs/heads/merge) &&
-	git cat-file commit refs/heads/svn^ | grep '^friend$'
+	git cat-file commit refs/heads/svn^ >actual &&
+	grep '^friend$' actual
 	"
 
 test_expect_success 'verify merge commit message' "
-	git rev-list --pretty=raw -1 refs/heads/svn | \
-	  grep \"    Merge branch 'merge' into svn\"
+	git rev-list --pretty=raw -1 refs/heads/svn >actual &&
+	grep \"    Merge branch 'merge' into svn\" actual
 	"
 
 test_done
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index 41264818c..a0f10b2d2 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -26,11 +26,12 @@ test_expect_success 'start import with incomplete authors file' '
 test_expect_success 'imported 2 revisions successfully' '
 	(
 		cd x
-		test "$(git rev-list refs/remotes/git-svn | wc -l)" -eq 2 &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn | \
-		  grep "^author BBBBBBB BBBBBBB <bb@example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 | \
-		  grep "^author AAAAAAA AAAAAAA <aa@example\.com> "
+		git rev-list refs/remotes/git-svn >actual &&
+		test_line_count = 2 actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn >actual &&
+		grep "^author BBBBBBB BBBBBBB <bb@example\.com> " actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 >actual1 &&
+		grep "^author AAAAAAA AAAAAAA <aa@example\.com> " actual1
 	)
 	'
 
@@ -43,11 +44,12 @@ test_expect_success 'continues to import once authors have been added' '
 	(
 		cd x
 		git svn fetch --authors-file=../svn-authors &&
-		test "$(git rev-list refs/remotes/git-svn | wc -l)" -eq 4 &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn | \
-		  grep "^author DDDDDDD DDDDDDD <dd@example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 | \
-		  grep "^author CCCCCCC CCCCCCC <cc@example\.com> "
+		git rev-list refs/remotes/git-svn >actual &&
+		test_line_count = 4 actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn >actual &&
+		grep "^author DDDDDDD DDDDDDD <dd@example\.com> " actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 >actual1 &&
+		grep "^author CCCCCCC CCCCCCC <cc@example\.com> " actual1
 	)
 	'
 
@@ -102,8 +104,10 @@ test_expect_success !MINGW 'fresh clone with svn.authors-file in config' '
 		test x"$HOME"/svn-authors = x"$(git config svn.authorsfile)" &&
 		git svn clone "$svnrepo" gitconfig.clone &&
 		cd gitconfig.clone &&
-		nr_ex=$(git log | grep "^Author:.*example.com" | wc -l) &&
-		nr_rev=$(git rev-list HEAD | wc -l) &&
+		nr_ex=$(git log >actual &&
+			    grep "^Author:.*example.com" actual | wc -l) &&
+		nr_rev=$(git rev-list HEAD >actual &&
+			     wc -l < actual) &&
 		test $nr_rev -eq $nr_ex
 	)
 '
diff --git a/t/t9138-git-svn-authors-prog.sh b/t/t9138-git-svn-authors-prog.sh
index 7d7e9d46b..f684f5578 100755
--- a/t/t9138-git-svn-authors-prog.sh
+++ b/t/t9138-git-svn-authors-prog.sh
@@ -37,31 +37,32 @@ test_expect_success 'import authors with prog and file' '
 test_expect_success 'imported 6 revisions successfully' '
 	(
 		cd x
-		test "$(git rev-list refs/remotes/git-svn | wc -l)" -eq 6
+		git rev-list refs/remotes/git-svn >actual &&
+		test_line_count = 6 actual
 	)
 '
 
 test_expect_success 'authors-prog ran correctly' '
 	(
 		cd x
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 | \
-		  grep "^author ee-foo <ee-foo@example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~2 | \
-		  grep "^author dd <dd@sub\.example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~3 | \
-		  grep "^author cc <cc@sub\.example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~4 | \
-		  grep "^author bb <bb@example\.com> " &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn~5 | \
-		  grep "^author aa <aa@example\.com> "
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~1 >actual &&
+		grep "^author ee-foo <ee-foo@example\.com> " actual &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~2 >actual2 &&
+		grep "^author dd <dd@sub\.example\.com> " actual2 &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~3 >actual3 &&
+		grep "^author cc <cc@sub\.example\.com> " actual3 &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~4 >actual4 &&
+		grep "^author bb <bb@example\.com> " actual4 &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn~5 >actual5 &&
+		grep "^author aa <aa@example\.com> " actual5
 	)
 '
 
 test_expect_success 'authors-file overrode authors-prog' '
 	(
 		cd x
-		git rev-list -1 --pretty=raw refs/remotes/git-svn | \
-		  grep "^author FFFFFFF FFFFFFF <fFf@other\.example\.com> "
+		git rev-list -1 --pretty=raw refs/remotes/git-svn >actual &&
+		grep "^author FFFFFFF FFFFFFF <fFf@other\.example\.com> " actual
 	)
 '
 
@@ -73,8 +74,8 @@ test_expect_success 'authors-prog handled special characters in username' '
 	(
 		cd x &&
 		git svn --authors-prog=../svn-authors-prog fetch &&
-		git rev-list -1 --pretty=raw refs/remotes/git-svn |
-		grep "^author xyz; touch evil <xyz; touch evil@example\.com> " &&
+		git rev-list -1 --pretty=raw refs/remotes/git-svn >actual &&
+		grep "^author xyz; touch evil <xyz; touch evil@example\.com> " actual &&
 		! test -f evil
 	)
 '
diff --git a/t/t9153-git-svn-rewrite-uuid.sh b/t/t9153-git-svn-rewrite-uuid.sh
index 372ef1568..6cd28bb9a 100755
--- a/t/t9153-git-svn-rewrite-uuid.sh
+++ b/t/t9153-git-svn-rewrite-uuid.sh
@@ -16,10 +16,10 @@ test_expect_success 'load svn repo' "
 	"
 
 test_expect_success 'verify uuid' "
-	git cat-file commit refs/remotes/git-svn~0 | \
-	   grep '^git-svn-id: .*@2 $uuid$' &&
-	git cat-file commit refs/remotes/git-svn~1 | \
-	   grep '^git-svn-id: .*@1 $uuid$'
+	git cat-file commit refs/remotes/git-svn~0 >actual &&
+	grep '^git-svn-id: .*@2 $uuid$' actual &&
+	git cat-file commit refs/remotes/git-svn~1 >actual1 &&
+	grep '^git-svn-id: .*@1 $uuid$' actual1
 	"
 
 test_done
diff --git a/t/t9168-git-svn-partially-globbed-names.sh b/t/t9168-git-svn-partially-globbed-names.sh
index 8b22f2272..df6f3a974 100755
--- a/t/t9168-git-svn-partially-globbed-names.sh
+++ b/t/t9168-git-svn-partially-globbed-names.sh
@@ -48,8 +48,8 @@ test_expect_success 'test refspec prefixed globbing' '
 	git config --add svn-remote.svn.tags\
 			 "tags/t_*/src/a:refs/remotes/tags/t_*" &&
 	git svn multi-fetch &&
-	git log --pretty=oneline refs/remotes/tags/t_end | \
-	    sed -e "s/^.\{41\}//" >output.end &&
+	git log --pretty=oneline refs/remotes/tags/t_end >actual &&
+	sed -e "s/^.\{41\}//" actual >output.end &&
 	test_cmp expect.end output.end &&
 	test "$(git rev-parse refs/remotes/tags/t_end~1)" = \
 		"$(git rev-parse refs/remotes/branches/b_start)" &&
@@ -78,14 +78,16 @@ test_expect_success 'test left-hand-side only prefixed globbing' '
 		svn_cmd commit -m "try to try"
 	) &&
 	git svn fetch two &&
-	test $(git rev-list refs/remotes/two/tags/t_end | wc -l) -eq 6 &&
-	test $(git rev-list refs/remotes/two/branches/b_start | wc -l) -eq 3 &&
+	git rev-list refs/remotes/two/tags/t_end >actual &&
+	test_line_count = 6 actual &&
+	git rev-list refs/remotes/two/branches/b_start >actual2 &&
+	test_line_count = 3 actual2 &&
 	test $(git rev-parse refs/remotes/two/branches/b_start~2) = \
 	     $(git rev-parse refs/remotes/two/trunk) &&
 	test $(git rev-parse refs/remotes/two/tags/t_end~3) = \
 	     $(git rev-parse refs/remotes/two/branches/b_start) &&
-	git log --pretty=oneline refs/remotes/two/tags/t_end | \
-	    sed -e "s/^.\{41\}//" >output.two &&
+	git log --pretty=oneline refs/remotes/two/tags/t_end >actual3 &&
+	sed -e "s/^.\{41\}//" actual3 >output.two &&
 	test_cmp expect.two output.two
 	'
 
@@ -118,14 +120,16 @@ test_expect_success 'test prefixed globs match just prefix' '
 		svn_cmd up
 	) &&
 	git svn fetch three &&
-	test $(git rev-list refs/remotes/three/branches/b_ | wc -l) -eq 2 &&
-	test $(git rev-list refs/remotes/three/tags/t_ | wc -l) -eq 3 &&
+	git rev-list refs/remotes/three/branches/b_ >actual &&
+	test_line_count = 2 actual &&
+	git rev-list refs/remotes/three/tags/t_ >actual2 &&
+	test_line_count = 3 actual2 &&
 	test $(git rev-parse refs/remotes/three/branches/b_~1) = \
 	     $(git rev-parse refs/remotes/three/trunk) &&
 	test $(git rev-parse refs/remotes/three/tags/t_~1) = \
 	     $(git rev-parse refs/remotes/three/branches/b_) &&
-	git log --pretty=oneline refs/remotes/three/tags/t_ | \
-	    sed -e "s/^.\{41\}//" >output.three &&
+	git log --pretty=oneline refs/remotes/three/tags/t_ >actual3 &&
+	sed -e "s/^.\{41\}//" actual3 >output.three &&
 	test_cmp expect.three output.three
 	'
 
@@ -186,14 +190,16 @@ test_expect_success 'test globbing in the middle of the word' '
 		svn_cmd up
 	) &&
 	git svn fetch five &&
-	test $(git rev-list refs/remotes/five/branches/abcde | wc -l) -eq 2 &&
-	test $(git rev-list refs/remotes/five/tags/fghij | wc -l) -eq 3 &&
+	git rev-list refs/remotes/five/branches/abcde >actual &&
+	test_line_count = 2 actual &&
+	git rev-list refs/remotes/five/tags/fghij >actual2 &&
+	test_line_count = 3 actual2 &&
 	test $(git rev-parse refs/remotes/five/branches/abcde~1) = \
 	     $(git rev-parse refs/remotes/five/trunk) &&
 	test $(git rev-parse refs/remotes/five/tags/fghij~1) = \
 	     $(git rev-parse refs/remotes/five/branches/abcde) &&
-	git log --pretty=oneline refs/remotes/five/tags/fghij | \
-	    sed -e "s/^.\{41\}//" >output.five &&
+	git log --pretty=oneline refs/remotes/five/tags/fghij >actual3 &&
+	sed -e "s/^.\{41\}//" actual3 >output.five &&
 	test_cmp expect.five output.five
 	'
 
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 866ddf605..45176742b 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -43,20 +43,20 @@ test_expect_success 'fast-export | fast-import' '
 	MUSS=$(git rev-parse --verify muss) &&
 	mkdir new &&
 	git --git-dir=new/.git init &&
-	git fast-export --all |
+	git fast-export --all >actual &&
 	(cd new &&
 	 git fast-import &&
 	 test $MASTER = $(git rev-parse --verify refs/heads/master) &&
 	 test $REIN = $(git rev-parse --verify refs/tags/rein) &&
 	 test $WER = $(git rev-parse --verify refs/heads/wer) &&
-	 test $MUSS = $(git rev-parse --verify refs/tags/muss))
+	 test $MUSS = $(git rev-parse --verify refs/tags/muss)) < actual
 
 '
 
 test_expect_success 'fast-export master~2..master' '
 
-	git fast-export master~2..master |
-		sed "s/master/partial/" |
+	git fast-export master~2..master >actual2 &&
+	sed "s/master/partial/" actual2 |
 		(cd new &&
 		 git fast-import &&
 		 test $MASTER != $(git rev-parse --verify refs/heads/partial) &&
@@ -74,11 +74,12 @@ test_expect_success 'iso-8859-1' '
 	test_tick &&
 	echo rosten >file &&
 	git commit -s -m den file &&
-	git fast-export wer^..wer |
-		sed "s/wer/i18n/" |
-		(cd new &&
-		 git fast-import &&
-		 git cat-file commit i18n | grep "Áéí óú")
+	git fast-export wer^..wer >actual3 &&
+	sed "s/wer/i18n/" actual3 |
+	    (cd new &&
+		git fast-import &&
+		git cat-file commit i18n >actual4 &&
+		grep "Áéí óú" actual4)
 
 '
 test_expect_success 'import/export-marks' '
@@ -87,18 +88,18 @@ test_expect_success 'import/export-marks' '
 	git fast-export --export-marks=tmp-marks HEAD &&
 	test -s tmp-marks &&
 	test_line_count = 3 tmp-marks &&
+	git fast-export --import-marks=tmp-marks\
+		--export-marks=tmp-marks HEAD >actual &&
 	test $(
-		git fast-export --import-marks=tmp-marks\
-		--export-marks=tmp-marks HEAD |
-		grep ^commit |
+		grep ^commit actual |
 		wc -l) \
 	-eq 0 &&
 	echo change > file &&
 	git commit -m "last commit" file &&
+	git fast-export --import-marks=tmp-marks \
+		--export-marks=tmp-marks HEAD >actual2 &&
 	test $(
-		git fast-export --import-marks=tmp-marks \
-		--export-marks=tmp-marks HEAD |
-		grep ^commit\  |
+		grep ^commit\  actual2 |
 		wc -l) \
 	-eq 1 &&
 	test_line_count = 4 tmp-marks
@@ -184,7 +185,7 @@ test_expect_success 'submodule fast-export | fast-import' '
 	rm -rf new &&
 	mkdir new &&
 	git --git-dir=new/.git init &&
-	git fast-export --signed-tags=strip --all |
+	git fast-export --signed-tags=strip --all >actual &&
 	(cd new &&
 	 git fast-import &&
 	 test "$SUBENT1" = "$(git ls-tree refs/heads/master^ sub)" &&
@@ -192,7 +193,7 @@ test_expect_success 'submodule fast-export | fast-import' '
 	 git checkout master &&
 	 git submodule init &&
 	 git submodule update &&
-	 cmp sub/file ../sub/file)
+	 cmp sub/file ../sub/file) < actual
 
 '
 
@@ -361,18 +362,20 @@ test_expect_failure 'no exact-ref revisions included' '
 	)
 '
 
-test_expect_success 'path limiting with import-marks does not lose unmodified files'        '
+test_expect_success 'path limiting with import-marks does not lose unmodified files'	    '
 	git checkout -b simple marks~2 &&
 	git fast-export --export-marks=marks simple -- file > /dev/null &&
 	echo more content >> file &&
 	test_tick &&
 	git commit -mnext file &&
-	git fast-export --import-marks=marks simple -- file file0 | grep file0
+	git fast-export --import-marks=marks simple -- file file0 >actual &&
+	grep file0 actual
 '
 
-test_expect_success 'full-tree re-shows unmodified files'        '
+test_expect_success 'full-tree re-shows unmodified files'	 '
 	git checkout -f simple &&
-	test $(git fast-export --full-tree simple | grep -c file0) -eq 3
+	git fast-export --full-tree simple >actual &&
+	test $(grep -c file0 actual) -eq 3
 '
 
 test_expect_success 'set-up a few more tags for tag export tests' '
@@ -505,8 +508,8 @@ test_expect_success 'refs are updated even if no commits need to be exported' '
 '
 
 test_expect_success 'use refspec' '
-	git fast-export --refspec refs/heads/master:refs/heads/foobar master | \
-		grep "^commit " | sort | uniq > actual &&
+	git fast-export --refspec refs/heads/master:refs/heads/foobar master >actual2 &&
+	grep "^commit " actual2 | sort | uniq > actual &&
 	echo "commit refs/heads/foobar" > expected &&
 	test_cmp expected actual
 '
@@ -534,7 +537,8 @@ test_expect_success 'when using -C, do not declare copy when source of copy is a
 	git -C src commit -m 2nd_commit &&
 
 	test_create_repo dst &&
-	git -C src fast-export --all -C | git -C dst fast-import &&
+	git -C src fast-export --all -C > actual &&
+	git -C dst fast-import < actual &&
 	git -C src show >expected &&
 	git -C dst show >actual &&
 	test_cmp expected actual
-- 
2.16.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [GSoC][PATCH] test: avoid pipes in git related commands for test suite
  2018-03-19 17:32         ` [GSoC][PATCH] " Pratik Karki
@ 2018-03-21 11:02           ` Eric Sunshine
  0 siblings, 0 replies; 7+ messages in thread
From: Eric Sunshine @ 2018-03-21 11:02 UTC (permalink / raw)
  To: Pratik Karki; +Cc: Git List

On Mon, Mar 19, 2018 at 1:32 PM, Pratik Karki <predatoramigo@gmail.com> wrote:
> Thank you Eric Sunshine,
> I have done as you had instructed me. I look forward to more
> understanding of the codebase and would love to fix
> "git rev-parse" problems in my follow-on patches.
> Thank you for the professional review comment.
>
> Sorry for late follow-on patch, I got tied up with my university stuffs.

No need to apologize; this is not a race. It's better to take time
preparing submissions carefully, than trying to rush them out.

> Please do review this patch as before. I will correct it if needed.

Below comments are meant to be instructive and constructive.

> Cheers,
> Pratik Karki

Place a "-- >8--" scissor line right here so that git-am knows where
the commit message begins; otherwise, all of the above commentary will
undesirably be included in the commit message.

> [PATCH] test: avoid pipes in git related commands for test suite

As this is the second attempt at this patch, the subject should be
"[PATCH v2]". Also, as an aid to reviewers -- who see a lot of patches
each day and are likely to forget details of each submission -- please
include a link in the commentary (not in the actual commit message)
pointing at the previous iteration, like this[1].

[1]: https://public-inbox.org/git/20180313201945.8409-1-predatoramigo@gmail.com/

> Avoid using pipes downstream of Git commands since the exit codes
> of commands upstream of pipes get swallowed, thus potentially
> hiding failure of those commands. Instead, capture Git command
> output to a file apply the downstream command(s) to that file.

This rewrite of the commit message which I suggested in [2] has a
grammatical error (which I noticed immediately after hitting "Send").
Unfortunately, you copied it verbatim, so the error is reproduced
here. Specifically, you want to insert "and" between "file" and
"apply":

    ... capture Git command output to a file _and_ apply...

[2]: https://public-inbox.org/git/CAPig+cRPzyw525ODC4=-E7w=zbpbhVN2eqxSYDSLij5wfW8S_A@mail.gmail.com/

> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---
>  t/t5300-pack-object.sh                     | 10 +++---
>  t/t5510-fetch.sh                           |  8 ++---
>  t/t7001-mv.sh                              | 22 ++++++-------
>  t/t7003-filter-branch.sh                   |  9 ++++--
>  t/t9104-git-svn-follow-parent.sh           | 16 +++++----
>  t/t9108-git-svn-glob.sh                    | 14 ++++----
>  t/t9109-git-svn-multi-glob.sh              | 28 +++++++++-------
>  t/t9110-git-svn-use-svm-props.sh           | 42 ++++++++++++------------
>  t/t9111-git-svn-use-svnsync-props.sh       | 36 ++++++++++-----------
>  t/t9114-git-svn-dcommit-merge.sh           | 10 +++---
>  t/t9130-git-svn-authors-file.sh            | 28 +++++++++-------
>  t/t9138-git-svn-authors-prog.sh            | 31 +++++++++---------
>  t/t9153-git-svn-rewrite-uuid.sh            |  8 ++---
>  t/t9168-git-svn-partially-globbed-names.sh | 34 +++++++++++--------
>  t/t9350-fast-export.sh                     | 52 ++++++++++++++++--------------
>  15 files changed, 187 insertions(+), 161 deletions(-)

The goal of iterating a patch or patch series is to converge to a
point at which the submission is in good enough shape to be accepted.
Ideally, each iteration should involve fewer changes than the previous
attempt.

Version 1 of this patch touched only 8 files, however, this version
touches 15, and is now uncomfortably large and difficult to review in
a single sitting (it took over 1.5 hours). Rather than converging, it
has instead diverged, and is thus potentially further from being in an
acceptable state than it would have been if v2 had merely addressed
the problems identified by the v1 review.

While the desire to address these additional cases is admirable, it is
better to focus on "landing" the current patch (getting it accepted)
before expanding your efforts; it's also more reviewer-friendly to
stay focused, especially with patches, such as this, which involve
primarily mechanical changes (which tend to be mind-numbing to
review).

> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> @@ -311,9 +311,9 @@ test_expect_success 'unpacking with --strict' '
>         rm -f .git/index &&
>         tail -n 10 LIST | git update-index --index-info &&
>         ST=$(git write-tree) &&
> -       PACK5=$( git rev-list --objects "$LIST" "$LI" "$ST" | \
> -               git pack-objects test-5 ) &&
> -       PACK6=$( (
> +       git rev-list --objects "$LIST" "$LI" "$ST" >actual &&
> +       PACK5=$(git pack-objects test-5 < actual) &&
> +       PACK6=$((

Losing the space between the two left parentheses is wrong. $( ( foo )
), which captures the output of subshell running 'foo', has very
different meaning than $((foo)), which performs arithmetic. This
change turns it into $(( foo) ), which, at best, is undefined.
Although bash seems to tolerate this change, other more strict shells
barf on it.

>                         echo "$LIST"
>                         echo "$LI"
>                         echo "$ST"
> @@ -358,8 +358,8 @@ test_expect_success 'index-pack with --strict' '
>         rm -f .git/index &&
>         tail -n 10 LIST | git update-index --index-info &&
>         ST=$(git write-tree) &&
> -       PACK5=$( git rev-list --objects "$LIST" "$LI" "$ST" | \
> -               git pack-objects test-5 ) &&
> +       git rev-list --objects "$LIST" "$LI" "$ST" >actual &&
> +       PACK5=$(git pack-objects test-5 < actual) &&
>         PACK6=$( (
>                         echo "$LIST"
>                         echo "$LI"
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> @@ -187,7 +187,8 @@ test_expect_success 'author information is preserved' '
>                         test \$GIT_COMMIT != $(git rev-parse master) || \
>                         echo Hallo" \
>                 preserved-author) &&
> -       test 1 = $(git rev-list --author="B V Uips" preserved-author | wc -l)
> +       git rev-list --author="B V Uips" preserved-author > actual &&

Style: drop space after '>'

> +       test 1 = $(wc -l < actual)

Style: drop space after '<'

You could also take advantage of test_line_count() rather than using 'wc':

    test_line_count = 1 actual

It's a judgment call whether or not to convert this to use
test_line_count(), however, since you made such a conversion later in
this file, you should do it here too. Or, don't use test_line_count()
in this file. It's not so important whether you do or not, but it is
important that you be consistent about it (which is not currently the
case).

> @@ -205,7 +206,8 @@ test_expect_success "remove a certain author's commits" '
>         cnt1=$(git rev-list master | wc -l) &&
>         cnt2=$(git rev-list removed-author | wc -l) &&
>         test $cnt1 -eq $(($cnt2 + 1)) &&
> -       test 0 = $(git rev-list --author="B V Uips" removed-author | wc -l)
> +       git rev-list --author="B V Uips" removed-author >actual &&
> +       test 0 = $(wc -l < actual)

Ditto: drop space before '<'
Ditto: test_line_count = 0 actual

> @@ -258,7 +260,8 @@ test_expect_success 'Subdirectory filter with disappearing trees' '
>         git filter-branch -f --subdirectory-filter foo &&
> -       test $(git rev-list master | wc -l) = 3
> +       git rev-list master >actual &&
> +       test_line_count = 3 actual
>  '

Here you used test_line_count().

> diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
> @@ -204,8 +205,9 @@ test_expect_success "follow-parent is atomic" '
>  test_expect_success "track multi-parent paths" '
>         svn_cmd cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
>         git svn multi-fetch &&
> -       test $(git cat-file commit refs/remotes/glob | \
> -              grep "^parent " | wc -l) -eq 2
> +       git cat-file commit refs/remotes/glob >actual &&
> +       grep "^parent " actual > actual2 &&

Style: drop space after '>'

> +       test_line_count = 2 actual2
> diff --git a/t/t9108-git-svn-glob.sh b/t/t9108-git-svn-glob.sh
> @@ -47,8 +47,8 @@ test_expect_success 'test refspec globbing' '
>         git config --add svn-remote.svn.tags\
>                          "tags/*/src/a:refs/remotes/tags/*" &&
>         git svn multi-fetch &&
> -       git log --pretty=oneline refs/remotes/tags/end | \
> -           sed -e "s/^.\{41\}//" > output.end &&
> +       git log --pretty=oneline refs/remotes/tags/end >actual &&
> +       sed -e "s/^.\{41\}//" actual > output.end &&

This is not a new problem, but since you're touching it, drop space after '>'.

>         test_cmp expect.end output.end &&
> @@ -75,14 +75,16 @@ test_expect_success 'test left-hand-side only globbing' '
>         git svn fetch two &&
> -       test $(git rev-list refs/remotes/two/tags/end | wc -l) -eq 6 &&
> -       test $(git rev-list refs/remotes/two/branches/start | wc -l) -eq 3 &&
> +       git rev-list refs/remotes/two/tags/end >actual &&
> +       test_line_count = 6 actual &&
> +       git rev-list refs/remotes/two/branches/start >actual2 &&
> +       test_line_count = 3 actual2 &&

It's better to name all these files "actual", rather than inventing
names "actual2", "actual3", etc. Those invented names mislead readers
into thinking that there might be some interrelation between the files
which requires them all to exist at once. But, this is not the case.
Those files serve no purpose after their associated test_line_count(),
and you can clearly indicate such by overwriting each time by reusing
the name "actual".

>         test $(git rev-parse refs/remotes/two/branches/start~2) = \
>              $(git rev-parse refs/remotes/two/trunk) &&
>         test $(git rev-parse refs/remotes/two/tags/end~3) = \
>              $(git rev-parse refs/remotes/two/branches/start) &&
> -       git log --pretty=oneline refs/remotes/two/tags/end | \
> -           sed -e "s/^.\{41\}//" > output.two &&
> +       git log --pretty=oneline refs/remotes/two/tags/end >actual3 &&
> +       sed -e "s/^.\{41\}//" actual3 > output.two &&

Style: drop space after '>'

>         test_cmp expect.two output.two
> diff --git a/t/t9109-git-svn-multi-glob.sh b/t/t9109-git-svn-multi-glob.sh
> @@ -47,8 +47,8 @@ test_expect_success 'test refspec globbing' '
>         git config --add svn-remote.svn.tags\
>                          "tags/*/src/a:refs/remotes/tags/*" &&
>         git svn multi-fetch &&
> -       git log --pretty=oneline refs/remotes/tags/end | \
> -           sed -e "s/^.\{41\}//" > output.end &&
> +       git log --pretty=oneline refs/remotes/tags/end >actual &&
> +       sed -e "s/^.\{41\}//" actual > output.end &&

Style: drop space after '>'

>         test_cmp expect.end output.end &&
> @@ -75,14 +75,16 @@ test_expect_success 'test left-hand-side only globbing' '
>         git svn fetch two &&
> -       test $(git rev-list refs/remotes/two/tags/end | wc -l) -eq 6 &&
> -       test $(git rev-list refs/remotes/two/branches/v1/start | wc -l) -eq 3 &&
> +       git rev-list refs/remotes/two/tags/end >actual2 &&
> +       test_line_count = 6 actual2 &&
> +       git rev-list refs/remotes/two/branches/v1/start >actual3 &&
> +       test_line_count = 3 actual3 &&

Ditto: name all these files "actual" rather than unnecessarily
inventing unique names

>         test $(git rev-parse refs/remotes/two/branches/v1/start~2) = \
>              $(git rev-parse refs/remotes/two/trunk) &&
>         test $(git rev-parse refs/remotes/two/tags/end~3) = \
>              $(git rev-parse refs/remotes/two/branches/v1/start) &&
> -       git log --pretty=oneline refs/remotes/two/tags/end | \
> -           sed -e "s/^.\{41\}//" > output.two &&
> +       git log --pretty=oneline refs/remotes/two/tags/end >actual4 &&
> +       sed -e "s/^.\{41\}//" actual4 > output.two &&

Style: drop space after '>'

>         test_cmp expect.two output.two
> @@ -120,18 +122,20 @@ test_expect_success 'test another branch' '
>         git config --add svn-remote.four.url "$svnrepo" &&
>         git config --add svn-remote.four.fetch trunk:refs/remotes/four/trunk &&
>         git config --add svn-remote.four.branches \
> -                        "branches/*/*:refs/remotes/four/branches/*/*" &&
> +                        "branches/*/*:refs/remotes/four/branches/*/*" &&
>         git config --add svn-remote.four.tags \
> -                        "tags/*:refs/remotes/four/tags/*" &&
> +                        "tags/*:refs/remotes/four/tags/*" &&

I guess you sneaked in a whitespace change here which is unrelated to
the stated purpose of this patch, thus acted as a speed bump during
review. If this was the only instance in this test script of
whitespace needing correction, then it _might_ be okay to include it
in this patch, however, that's not the case. There are many other such
instance in this test script which could be corrected, so it doesn't
make sense to single out these two and ignore all the others.
Therefore, you should omit this change.

>         git svn fetch four &&
> -       test $(git rev-list refs/remotes/four/tags/next | wc -l) -eq 5 &&
> -       test $(git rev-list refs/remotes/four/branches/v2/start | wc -l) -eq 3 &&
> +       git rev-list refs/remotes/four/tags/next >actual &&
> +       test_line_count = 5 actual &&
> +       git rev-list refs/remotes/four/branches/v2/start >actual2 &&
> +       test_line_count = 3 actual2 &&

Ditto: name all these files "actual" rather than unnecessarily
inventing unique names

>         test $(git rev-parse refs/remotes/four/branches/v2/start~2) = \
>              $(git rev-parse refs/remotes/four/trunk) &&
>         test $(git rev-parse refs/remotes/four/tags/next~2) = \
>              $(git rev-parse refs/remotes/four/branches/v2/start) &&
> -       git log --pretty=oneline refs/remotes/four/tags/next | \
> -           sed -e "s/^.\{41\}//" > output.four &&
> +       git log --pretty=oneline refs/remotes/four/tags/next >actual3 &&
> +       sed -e "s/^.\{41\}//" actual3 > output.four &&

Style: drop space after '>'

>         test_cmp expect.four output.four
> diff --git a/t/t9110-git-svn-use-svm-props.sh b/t/t9110-git-svn-use-svm-props.sh
> @@ -21,38 +21,38 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
>  test_expect_success 'verify metadata for /bar' "
> -       git cat-file commit refs/remotes/bar | \
> -          grep '^git-svn-id: $bar_url@12 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~1 | \
> -          grep '^git-svn-id: $bar_url@11 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~2 | \
> -          grep '^git-svn-id: $bar_url@10 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~3 | \
> -          grep '^git-svn-id: $bar_url@9 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~4 | \
> -          grep '^git-svn-id: $bar_url@6 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~5 | \
> -          grep '^git-svn-id: $bar_url@1 $uuid$'
> +       git cat-file commit refs/remotes/bar >actual &&
> +       grep '^git-svn-id: $bar_url@12 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~1 >actual1 &&
> +       grep '^git-svn-id: $bar_url@11 $uuid$' actual1 &&
> +       git cat-file commit refs/remotes/bar~2 >actual2 &&
> +       grep '^git-svn-id: $bar_url@10 $uuid$' actual2 &&
> +       git cat-file commit refs/remotes/bar~3 >actual3 &&
> +       grep '^git-svn-id: $bar_url@9 $uuid$' actual3 &&
> +       git cat-file commit refs/remotes/bar~4 >actual4 &&
> +       grep '^git-svn-id: $bar_url@6 $uuid$' actual4 &&
> +       git cat-file commit refs/remotes/bar~5 >actual5 &&
> +       grep '^git-svn-id: $bar_url@1 $uuid$' actual5
>         "

Ditto: name all these files "actual" rather than unnecessarily
inventing unique names

Same comment applies to many more tests below; I won't be repeating it
beyond this point.

> diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
> @@ -102,8 +104,10 @@ test_expect_success !MINGW 'fresh clone with svn.authors-file in config' '
>                 test x"$HOME"/svn-authors = x"$(git config svn.authorsfile)" &&
>                 git svn clone "$svnrepo" gitconfig.clone &&
>                 cd gitconfig.clone &&
> -               nr_ex=$(git log | grep "^Author:.*example.com" | wc -l) &&
> -               nr_rev=$(git rev-list HEAD | wc -l) &&
> +               nr_ex=$(git log >actual &&
> +                           grep "^Author:.*example.com" actual | wc -l) &&
> +               nr_rev=$(git rev-list HEAD >actual &&
> +                            wc -l < actual) &&
>                 test $nr_rev -eq $nr_ex

This transformation is effectively bogus, as explained already in my
review[3] of v1. If git-log or git-rev-list fails, variable nr_ex or
nr_rev will have an empty value, thus 'test' will error out. Move the
git-log and git-rev-list invocations out of the $(...):

    git log >actual &&
    nr_ex=$(grep "..." actual | wc -l) &&
    git rev-list HEAD >actual &&
    nr_rev=$(wc -l <actual) &&
    test $nr_rev -eq $nr_ex

Also, style: drop space after '<'

[3]: https://public-inbox.org/git/CAPig+cRPzyw525ODC4=-E7w=zbpbhVN2eqxSYDSLij5wfW8S_A@mail.gmail.com/

> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> @@ -43,20 +43,20 @@ test_expect_success 'fast-export | fast-import' '
>         MUSS=$(git rev-parse --verify muss) &&
>         mkdir new &&
>         git --git-dir=new/.git init &&
> -       git fast-export --all |
> +       git fast-export --all >actual &&
>         (cd new &&
>          git fast-import &&
>          test $MASTER = $(git rev-parse --verify refs/heads/master) &&
>          test $REIN = $(git rev-parse --verify refs/tags/rein) &&
>          test $WER = $(git rev-parse --verify refs/heads/wer) &&
> -        test $MUSS = $(git rev-parse --verify refs/tags/muss))
> +        test $MUSS = $(git rev-parse --verify refs/tags/muss)) < actual

Style: drop space after '<'

>  '
>
>  test_expect_success 'fast-export master~2..master' '
>
> -       git fast-export master~2..master |
> -               sed "s/master/partial/" |
> +       git fast-export master~2..master >actual2 &&
> +       sed "s/master/partial/" actual2 |

Not sure why you named this "actual2" rather than just "actual".

>                 (cd new &&
>                  git fast-import &&
>                  test $MASTER != $(git rev-parse --verify refs/heads/partial) &&
> @@ -74,11 +74,12 @@ test_expect_success 'iso-8859-1' '
>         git commit -s -m den file &&
> -       git fast-export wer^..wer |
> -               sed "s/wer/i18n/" |
> -               (cd new &&
> -                git fast-import &&
> -                git cat-file commit i18n | grep "Áéí óú")
> +       git fast-export wer^..wer >actual3 &&
> +       sed "s/wer/i18n/" actual3 |

Ditto: Why "actual3" rather than "actual"?

> +           (cd new &&
> +               git fast-import &&
> +               git cat-file commit i18n >actual4 &&
> +               grep "Áéí óú" actual4)
> @@ -87,18 +88,18 @@ test_expect_success 'import/export-marks' '
>         git fast-export --export-marks=tmp-marks HEAD &&
>         test -s tmp-marks &&
>         test_line_count = 3 tmp-marks &&
> +       git fast-export --import-marks=tmp-marks\
> +               --export-marks=tmp-marks HEAD >actual &&

Style: not a new problem, but add space before \ since you're touching it

>         test $(
> -               git fast-export --import-marks=tmp-marks\
> -               --export-marks=tmp-marks HEAD |
> -               grep ^commit |
> +               grep ^commit actual |
>                 wc -l) \
>         -eq 0 &&
> @@ -192,7 +193,7 @@ test_expect_success 'submodule fast-export | fast-import' '
>          git checkout master &&
>          git submodule init &&
>          git submodule update &&
> -        cmp sub/file ../sub/file)
> +        cmp sub/file ../sub/file) < actual

Style: drop space after '<'

> @@ -361,18 +362,20 @@ test_expect_failure 'no exact-ref revisions included' '
> -test_expect_success 'path limiting with import-marks does not lose unmodified files'        '
> +test_expect_success 'path limiting with import-marks does not lose unmodified files'       '

It's probably better not to sneak in whitespace changes, especially if
you're not fixing all of them in this script (and this isn't even the
correct fix). Same comment applies below.

> -test_expect_success 'full-tree re-shows unmodified files'        '
> +test_expect_success 'full-tree re-shows unmodified files'       '
> @@ -505,8 +508,8 @@ test_expect_success 'refs are updated even if no commits need to be exported' '
>  test_expect_success 'use refspec' '
> -       git fast-export --refspec refs/heads/master:refs/heads/foobar master | \
> -               grep "^commit " | sort | uniq > actual &&
> +       git fast-export --refspec refs/heads/master:refs/heads/foobar master >actual2 &&
> +       grep "^commit " actual2 | sort | uniq > actual &&

Style: drop space before '>' (and probably the next line too)

>         echo "commit refs/heads/foobar" > expected &&
> @@ -534,7 +537,8 @@ test_expect_success 'when using -C, do not declare copy when source of copy is a
>         test_create_repo dst &&
> -       git -C src fast-export --all -C | git -C dst fast-import &&
> +       git -C src fast-export --all -C > actual &&
> +       git -C dst fast-import < actual &&

Style: drop space before '>' and '<'

>         git -C src show >expected &&
>         git -C dst show >actual &&
>         test_cmp expected actual

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 20:19 [GSoC] [PATCH] test: avoid pipes in git related commands for test suite Pratik Karki
2018-03-14  7:30 ` Eric Sunshine
2018-03-14  9:57   ` Ævar Arnfjörð Bjarmason
2018-03-14 18:22     ` Eric Sunshine
2018-03-15 17:04       ` Junio C Hamano
2018-03-19 17:32         ` [GSoC][PATCH] " Pratik Karki
2018-03-21 11:02           ` Eric Sunshine

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox