Thanks for your feedback, I now have a better understanding. I have made the changes you requested. On Sun, Oct 18, 2020 at 7:11 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > 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. -- Cheers! Amanda Shafack
