Re: [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file)

From: Martin Ågren <martin.agren@gmail.com>
Date: Feb 26, 2019, 16:30 UTC
Message-ID: <CAN0heSqSp-a0zUKT5EaGLBYnRtESTnu9GKWtGARz2kaOAhc1HQ@mail.gmail.com>
In-Reply-To: <bf5eb045795579dd5d996e787e246996688cf4bf.1551188524.git.gitgitgadget@gmail.com>

On Tue, 26 Feb 2019 at 14:43, Rohit Ashiwal via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 6 quoted lines

>
> 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