Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'
Thanks for the patch. See below for some comments...
On Sun, Jun 15, 2025 at 10:08 PM Rodrigo Michelassi <rodmichelassi@gmail.com> wrote:
> From: rodrigocmichelassi <rodmichelassi@gmail.com>
The From: header name/address should match your Signed-off-by: trailer, so you'll probably need to adjust your mailer settings.
> replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'
Let's prefix the subject with the area you're touching. In this case, the test number would be appropriate, so:
t2400: replace 'test -[efd]' with test_path_* calls
> 'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach
A better way to convince reviewers that this is a good idea is to explain why these functions are superior. In this case, it's because they emit useful diagnostic information when they detect a failing condition, whereas `test` itself does not.
Please wrap the commit message at about the 72-column mark.
> Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>
>
> Co-authored-by: Isabella Caselli <icaselli@usp.br>
> Signed-off-by: Isabella Caselli <icaselli@usp.br>
You'll probably want to order these trailers like this:
Co-authored-by: Isabella Caselli <icaselli@usp.br>
Signed-off-by: Isabella Caselli <icaselli@usp.br>
Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>Show 8 quoted lines
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -42,8 +42,8 @@ test_expect_success '"add" using - shorthand' '
> test_expect_success '"add" refuses to checkout locked branch' '
> test_must_fail git worktree add zere main &&
> - ! test -d zere &&
> - ! test -d .git/worktrees/zere
> + ! test_path_is_dir zere &&
> + ! test_path_is_dir .git/worktrees/zere
Take a look at the definition of `test_path_is_dir` from t/test-lib-functions.sh to see why this change is undesirable:
test_path_is_dir () {
test "$#" -ne 1 && BUG "1 param"
if ! test -d "$1"
then
echo "Directory $1 doesn't exist"
false
fi
}The test wants to assert that those directories do *not* exist, which means that if `git worktree add` is working correctly, then the directories indeed will not exist. However, `test_path_is_dir` is going to complain that they don't exist, which is the opposite of what we want. So, even though the test will continue to pass following this change (due to the negation `!`), it's going to be spewing unwanted and confusing error messages.
What you want instead is a test_path_* function which asserts that a path does not exist. Probably the closest we have is `test_path_is_missing` which seems a good semantic match for what this test intends with regards to those directories.
Show 5 quoted lines
> @@ -474,7 +474,7 @@ test_expect_success 'local clone --shared from linked checkout' '
> test_expect_success '"add" worktree with --no-checkout' '
> git worktree add --no-checkout -b swamp swamp &&
> - ! test -e swamp/init.t &&
> + ! test_path_is_executable swamp/init.t &&
If you look at the definition of `test_path_is_executable` in t/test-lib-functions.sh, you'll see that this change is similarly undesirable.
The same comments apply to several other changes in this patch.
Rodrigo Michelassi
