On Tue, Feb 26 2019, Duy Nguyen wrote:
> On Tue, Feb 26, 2019 at 8:42 PM Rohit Ashiwal via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
>>
>> From: Rohit Ashiwal <rohit.ashiwal265@xxxxxxxxx>
>>
>> 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
>
> It's not just the same (no point replacing then). It's better. When
> test_path_is_xxx fails, you get an error message. If "test -xxx"
> fails, you get a failed test with no clue what caused it.
I swear I'm not just on a mission to ruin everyone's GSOC projects. This
patch definitely looks good, and given that we have this / document it
makes sense.
However. I wonder in general if we've re-visited the utility of these
wrappers and maybe other similar wrappers after -x was added.
Back when this was added in 2caf20c52b ("test-lib: user-friendly
alternatives to test [-d|-f|-e]", 2010-08-10) we didn't have -x. So we'd
at best fail like this:
$ ./t0001-init.sh -v -i
Initialized empty Git repository in /home/avar/g/git/t/trash directory.t0001-init/.git/
expecting success:
test -d .git &&
test -f doesnotexist &&
test -f .git/config
not ok 1 - check files
#
# test -d .git &&
# test -f doesnotexist &&
# test -f .git/config
#
At that point this was a definite improvement:
expecting success:
test_path_is_dir .git &&
test_path_is_file doesnotexist &&
test_path_is_file .git/config
File doesnotexist doesn't exist.
not ok 1 - check files
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.
And at least to me whenever I have a test failure the first thing I do
is try with -x (if I wasn't already using it). Under that the wrapper
output is more verbose and no more helpful. It's immediately clear
what's going on with:
+ test -f doesnotexist
error: last command exited with $?=1
Whereas:
+ test -f doesnotexist
+ echo File doesnotexist doesn't exist.
File doesnotexist doesn't exist.
+ false
error: last command exited with $?=1
Gives me the same thing, but I have to read 5 lines instead of 2 that
ultimately don't tell me any more (and a bit of "huh, 'false' returned
1? Of course! Oh! It's faking things up and it's the 'echo' that
matters...").
Looking over test-lib-functions.sh this patch would do it. I couldn't
spot any other functions redundant to -x:
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 80402a428f..b3a95b4968 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -555,33 +555,6 @@ test_external_without_stderr () {
fi
}
-# debugging-friendly alternatives to "test [-f|-d|-e]"
-# The commands test the existence or non-existence of $1. $2 can be
-# given to provide a more precise diagnosis.
-test_path_is_file () {
- if ! test -f "$1"
- then
- echo "File $1 doesn't exist. $2"
- false
- fi
-}
-
-test_path_is_dir () {
- if ! test -d "$1"
- then
- echo "Directory $1 doesn't exist. $2"
- false
- fi
-}
-
-test_path_exists () {
- if ! test -e "$1"
- then
- echo "Path $1 doesn't exist. $2"
- false
- fi
-}
-
# Check if the directory exists and is empty as expected, barf otherwise.
test_dir_is_empty () {
test_path_is_dir "$1" &&
@@ -593,19 +566,6 @@ test_dir_is_empty () {
fi
}
-test_path_is_missing () {
- if test -e "$1"
- then
- echo "Path exists:"
- ls -ld "$1"
- if test $# -ge 1
- then
- echo "$*"
- fi
- false
- fi
-}
-
# test_line_count checks that a file has the number of lines it
# ought to. For example:
#
@@ -849,6 +809,9 @@ verbose () {
# otherwise.
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
