On Sun, Feb 11, 2024 at 9:53 AM Chandra Pratap via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
> The helper functions test_path_is_* provide better debugging
> information than test -d/-e/-f.
>
> Replace "! test -d" with "test_path_is_missing" at places where
> we check for non-existent directories.
>
> Replace "test -f" with "test_path_is_file" and "test -d" with
> "test_path_is_dir" at places where we expect files or directories
> to exist.
The aim of this patch makes sense, but the implementation needs some
refinement...
> diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh
> @@ -20,7 +20,7 @@ test_expect_success 'empty directories exist' '
> for i in a b c d d/e d/e/f "weird file name"
> do
> - if ! test -d "$i"
> + if test_path_is_missing "$i"
> then
> echo >&2 "$i does not exist" &&
> exit 1
The point of functions such as test_path_is_missing() is to _assert_
that some condition is true, thus allowing the test to succeed; if the
condition is not true, then the function prints an error message and
the test aborts and fails.
test_path_is_missing () {
if test -e "$1"
then
echo "Path exists:"
ls -ld "$1"
false
fi
}
It is meant to replace noisy code such as:
if ! test -f bloop
then
echo >&2 "error message" &&
exit 1
fi &&
other-code
with much simpler:
test_path_exists bloop &&
other-code
So, the changes made by this patch are incorrect in two ways...
First, the patch retains code which prints an error message even
though this code becomes redundant since the test_path_foo() functions
already take care of printing the error message.
Second, and more problematic, the patch incorrectly inverts the sense
of what is being tested. For instance, the title of this test is
"empty directories exist", and the body of the test asserts that the
empty directories _do_ exist, but the patch changes the condition to
assert that the directories do _not_ exist, which is wrong.
Taking these observations into account, this test should become:
test_expect_success 'empty directories exist' '
(
cd cloned &&
for i in a b c d d/e d/e/f "weird file name"
do
test_path_exists "$i" || exit 1
done
)
'
Many of the other changes made by this patch suffer similar problems
> @@ -138,16 +138,16 @@ test_expect_success 'git svn gc-ed files work' '
> : Compress::Zlib may not be available &&
> - if test -f "$unhandled".gz
> + if test_path_is_file "$unhandled".gz
> then
> svn_cmd mkdir -m gz "$svnrepo"/gz &&
> git reset --hard $(git rev-list HEAD | tail -1) &&
This change is wrong/undesirable for a different reason. Taking into
account what was said above about test_path_is_file() being an
_assertion_ that some condition is true, coupled with the comment
above this `if` statement which says "Compress::Zlib may not be
available", then this `test -f` is legitimately part of the
control-flow of the function. It is not a mere assertion. Thus,
replacing it with the assertion function test_path_is_file() breaks
the test for the case when Compress::Zlib is not available.
> - test -f "$unhandled".gz &&
> - test -f "$unhandled" &&
> + test_path_is_file "$unhandled".gz &&
> + test_path_is_file "$unhandled" &&
These replacements are correct in that they replace the _assertion_
`test -f` with the equivalent assertion `test_path_is_file`.
