On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > When the upstream of a pipe throws an error, the downstream still > executes normally. This happens because the exit code of the upstream > in a pipe is ignored. This behavior can make debugging very hard > incase a test fails. To be more precise, we're not worried about difficulty of debugging when a Git command is upstream in a pipe. What we are more concerned with is that an unexpected failure of the Git command will go unnoticed (unless it also happens to produce wrong output which is later caught somewhere downstream). By avoiding placing Git upstream in a pipe, we can actively catch a failure of the Git command. The commit message should focus on that reason instead. > Also, pipes are prone to deadlocks. If the > upstream gets full, the commands downstream will never start. I don't think this is ever an issue in Git tests, so talking about it here probably only muddies the waters, thus makes the commit message less precise and more hand-wavy. Dropping this sentence is recommended. > Commit c6f44e1da5 (t9813: avoid using pipes, 2017-01-04) noticed that > the exit code of upstream in the pipe is ignored, thus using pipes > should be avoided. There are a lot of commits in the project history which perform this sort of transformation, so it's not clear to the reader why this particular commit is called out as being important. If it's not, then I'd recommend dropping this sentence, as well. > diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh > 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 > + ) > ' This transformation appears to be incorrect. See my review of patch [2/2] for details. > diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh > @@ -68,7 +68,8 @@ EOF > - git show refs/remotes/p4-unshelved/$change | grep -q "Further description" && > + git show refs/remotes/p4-unshelved/$change >actual && > + grep -q "Further description" actual && This transformation looks fine.
