I agree with you. I have made these changes as well. Thanks On Sun, Oct 18, 2020 at 6:56 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: > > Using the logical not operator on both the git and grep command is > > redundant, since the main check is done at the level of the grep > > command. > > > > Apply the logical not operator only to the grep command. > > --- > > diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh > > @@ -179,10 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' ' > > - ! ( > > - git ls-files >actual && > > - grep "non-existent" actual > > - ) > > + git ls-files >actual && > > + ! grep "non-existent" actual > > The commit message talks only about redundancy of applying the logical > NOT operator to the combined subshell content, but what it misses is > that transformation of: > > ! (git ls-files | grep "non-existent") > > in patch [1/2] into: > > ! ( > git ls-files >actual && > grep "non-existent" actual > ) > > is actively wrong. In particular, if `git ls-files` itself fails, then > the `grep` will never get run, and the subshell will exit with a > failure, however, the NOT then turns that failure into a success, > which is quite undesirable. The test should fail if either `git > ls-files` fails or if the `grep` expectation is not met; it should not > succeed when `git ls-files` fails. > > So, the correct thing to do is to merge [1/2] and [2/2] into a single > patch which directly transforms: > > ! (git ls-files | grep "non-existent") > > into: > > git ls-files >actual && > ! grep "non-existent" actual -- Cheers! Amanda Shafack
