Re: [GSoC][PATCH] test: avoid pipes in git related commands for test suite
On Mon, Mar 19, 2018 at 1:32 PM, Pratik Karki 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-predatoram...@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=zbpbhvn2eqxsydslij5wfw8...@mail.gmail.com/ > Signed-off-by: Pratik Karki > --- > 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" >
[GSoC][PATCH] test: avoid pipes in git related commands for test suite
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 --- 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 long-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_expec
Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
Eric Sunshine 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-zjubo4bru...@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 ;-)
Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
On Wed, Mar 14, 2018 at 5:57 AM, Ævar Arnfjörð Bjarmason wrote: > On Wed, Mar 14 2018, Eric Sunshine jotted: >> On Tue, Mar 13, 2018 at 4:19 PM, Pratik Karki >> 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-zjubo4bru...@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.
Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
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 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 >> --- >> 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.
Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
Thanks for the patch. See comments below... On Tue, Mar 13, 2018 at 4:19 PM, Pratik Karki 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 > --- > 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 '^g
[GSoC] [PATCH] test: avoid pipes in git related commands for test suite
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 --- 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
