On Tue, Feb 26, 2019 at 05:10:30PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.
> 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
