Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

From: Jeff King <peff@peff.net>
Date: Feb 26, 2019, 17:35 UTC
Message-ID: <20190226173542.GC19606@sigill.intra.peff.net>
In-Reply-To: <87sgwav8cp.fsf@evledraar.gmail.com>

On Tue, Feb 26, 2019 at 05:10:30PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 28 quoted lines

> But 4 years after this was added in a136f6d8ff ("test-lib.sh: support -x
> option for shell-tracing", 2014-10-10) we got -x, and then with "-i -v -x":
> 
>     expecting success:
>             test_path_is_dir .git &&
>             test_path_is_file doesnotexist &&
>             test_path_is_file .git/config
> 
>     + test_path_is_dir .git
>     + test -d .git
>     + test_path_is_file doesnotexist
>     + test -f doesnotexist
>     + echo File doesnotexist doesn't exist.
>     File doesnotexist doesn't exist.
>     + false
>     error: last command exited with $?=1
>     not ok 1 - check files
> 
> But by just using "test -d/-e": the much shorter:
> 
>     + test -d .git
>     + test -f doesnotexist
>     error: last command exited with $?=1
>     not ok 1 - check files
> 
> So I wonder if these days we shouldn't do this the other way around and
> get rid of these. Every test_* wrapper we add adds a bit of cognitive
> overload when you have to remember Git's specific shellscript dialect.

I don't have a strong opinion, but I do agree that with "-x" it's nicer without the wrappers. I typically re-run with just "-v" on a failure, and only turn to "-x" if the verbose output isn't helpful. However, with the rise of multi-platform CI jobs which try to collect as much information as possible in the initial run, I do find myself looking at "-x" more often.

As Gábor notes, you can't run every script with "-x". But I find it's pretty consistent these days (and totally so if you have a recent bash). I dunno. Maybe people on other platforms (who might not have bash) would care more.

I had a vague notion that there was some reason (portability?) that we preferred to have the wrappers. But as your patch shows, they really are just calling "test" and nothing else.

Show 7 quoted lines

>      test_must_be_empty () {
>     +	# We don't want to remove this as noted in ec10b018e7 ("tests:
>     +	# use 'test_must_be_empty' instead of '! test -s'",
>     +	# 2018-08-19)
>      	test_path_is_file "$1" &&
>      	if test -s "$1"
>      	then

You'd still want it to become "test -f" though, right?

-Peff