On Tue, 26 Feb 2019 at 14:43, Rohit Ashiwal via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > t3600-rm.sh: Previously we were using `test -(d|f)` > to verify the presencee of a directory/file, but we > already have helper functions, viz, test_path_is_dir > and test_path_is_file with same functionality. This > patch will replace `test -(d|f)` calls in t3600-rm.sh. I think this makes a lot of sense. If a test breaks, we'll get some helpful error message. Thank you for working on this. > - ! test -d submod && > + ! test_path_is_dir submod && Now, here I wonder. This (and other changes like this) means that every time the test passes, we see "Directory submod doesn't exist.", which is perhaps not too irritating. But more importantly, when the test fails, we don't get any hint. So a failure is just as silent and "non-helpful" as before. I can think of a few approaches: 1 Teach `test_path_is_dir` and friends to handle "!" in a clever way, and write these as `test_path_is_dir ! foo`. (We already have helpers that do this, see, e.g., `test_i18ngrep`.) 2 Don't be clever, and just introduce `test_path_is_not_dir`. 3 Don't bother, because this small change here doesn't make the error case any worse. 4 Don't do this small change here, and leave cases like this for a later change (something like 1 or 2 above). What do you think? There are a few of these "!". The other changes look good to me. Cheers Martin
