[v3] t2200,t9832: avoid using 'git' upstream in a pipe

Commit Message

Amanda Shafack Oct. 18, 2020, 8:48 p.m. UTC

From: Amanda Shafack <[e-mail]>

Avoid placing `git` upstream in a pipe since doing so throws away
its exit code, thus an unexpected failure may go unnoticed.

Signed-off-by: Amanda Shafack <[e-mail]>
---
    [Outreachy][Patch v3] t2200,t9832: avoid using 'git' upstream in a pipe
    
    Changes since v2:
    
     * Optimized commit message
    
    Signed-off-by: Amanda Shafack [e-mail]
    [[e-mail]]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-885%2Flkmandy%2Favoid-pipes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-885/lkmandy/avoid-pipes-v3
Pull-Request: https://github.com/git/git/pull/885

Range-diff vs v2:

 1:  bca393e69a ! 1:  d5f508b055 t9832,t2200: avoid using pipes in git commands
     @@ Metadata
      Author: Amanda Shafack <[e-mail]>
      
       ## Commit message ##
     -    t9832,t2200: avoid using pipes in git commands
     +    t2200,t9832: avoid using 'git' upstream in a pipe
      
     -    When a git command is upstream in a pipe, an unexpected failure of
     -    the git command will go unnoticed.
     -
     -    Write out the output of the git command to a file, so as to actively
     -    catch a failure of the git command.
     +    Avoid placing `git` upstream in a pipe since doing so throws away
     +    its exit code, thus an unexpected failure may go unnoticed.
      
          Signed-off-by: Amanda Shafack <[e-mail]>
      


 t/t2200-add-update.sh | 3 ++-
 t/t9832-unshelve.sh   | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)


base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a

Comments

Junio C Hamano Oct. 18, 2020, 9:01 p.m. UTC | #1

"Amanda  Shafack  via GitGitGadget" <[e-mail]> writes:

> From: Amanda Shafack <[e-mail]>
>
> Avoid placing `git` upstream in a pipe since doing so throws away
> its exit code, thus an unexpected failure may go unnoticed.

Well explained.

> Signed-off-by: Amanda Shafack <[e-mail]>
> ---



> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
> index f764b7e3f5..7cb7a70382 100755
> --- a/t/t2200-add-update.sh
> +++ b/t/t2200-add-update.sh
> @@ -179,7 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '
>  
>  test_expect_success '"add -u non-existent" should fail' '
>  	test_must_fail git add -u non-existent &&
> -	! (git ls-files | grep "non-existent")
> +	git ls-files >actual &&
> +	! grep "non-existent" actual
>  '

In the scope of this patch, the above change is a good rewrite.
Let's stop the iteration here---you've demonstrated through the
microproject that you can now comfortably be involved in the review
discussion.

In a larger scope of "can we write this particular line better?",
however, the above may not be the _best_ answer.  For example,

	test_must_fail git ls-files --error-unmatch non-existent

would be another and a more direct way to ensure that the named path
does not appear in the index.

Thanks.

Patch

diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index f764b7e3f5..7cb7a70382 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -179,7 +179,8 @@  test_expect_success 'add -u resolves unmerged paths' '
 
 test_expect_success '"add -u non-existent" should fail' '
 	test_must_fail git add -u non-existent &&
-	! (git ls-files | grep "non-existent")
+	git ls-files >actual &&
+	! grep "non-existent" actual
 '
 
 test_done
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 7194fb2855..6b3cb0414a 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -68,7 +68,8 @@  EOF
 		cd "$git" &&
 		change=$(last_shelved_change) &&
 		git p4 unshelve $change &&
-		git show refs/remotes/p4-unshelved/$change | grep -q "Further description" &&
+		git show refs/remotes/p4-unshelved/$change >actual &&
+		grep -q "Further description" actual &&
 		git cherry-pick refs/remotes/p4-unshelved/$change &&
 		test_path_is_file file2 &&
 		test_cmp file1 "$cli"/file1 &&