On Wed, Feb 27, 2019 at 5:49 AM Rohit Ashiwal via GitGitGadget <[email protected]> wrote: > > From: Rohit Ashiwal <[email protected]> > > Replace `test -(d|f|e)` calls in t3600-rm.sh > > Previously we were using `test -(d|f|e)` to verify > the presence of a directory/file, but we already > have helper functions, viz, `test_path_is_dir`, > `test_path_is_file` and `test_path_is_missing` > with better functionality. > > These helper functions make code more readable > and informative to someone new to code, also > these functions have better error messages > > Signed-off-by: Rohit Ashiwal <[email protected]> > --- > t/t3600-rm.sh | 138 +++++++++++++++++++++++++------------------------- > 1 file changed, 69 insertions(+), 69 deletions(-) > > diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh > index 04e5d42bd3..ad638490ac 100755 > --- a/t/t3600-rm.sh > +++ b/t/t3600-rm.sh > @@ -83,7 +83,7 @@ test_expect_success \ > > test_expect_success \ > 'Post-check that bar does not exist and is not in index after "git rm -f > bar"' \ > - '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar' > + 'test_path_is_missing bar && test_must_fail git ls-files --error-unmatch > bar'
This line should be broken down in two. It was reasonably short before, but now getting long and two checks in one line seem easy to miss. I was a bit worried that the "test ! something" could be incorrectly converted because for example, "test ! -d foo" is not always the same as "test_path_is_missing". If "foo" is intended to be a file, then the conversion is wrong. But I don't think you made any wrong conversion here. All these negative "test" are preceded by "git rm" so the expectation is always "test ! -e". -- Duy
