Thanks for the patch. See below for some comments...
On Sun, Jun 15, 2025 at 10:08 PM Rodrigo Michelassi
<rodmichelassi@xxxxxxxxx> wrote:
> From: rodrigocmichelassi <rodmichelassi@xxxxxxxxx>
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@xxxxxxxxx>
>
> Co-authored-by: Isabella Caselli <icaselli@xxxxxx>
> Signed-off-by: Isabella Caselli <icaselli@xxxxxx>
You'll probably want to order these trailers like this:
Co-authored-by: Isabella Caselli <icaselli@xxxxxx>
Signed-off-by: Isabella Caselli <icaselli@xxxxxx>
Signed-off-by: Rodrigo Michelassi <rodmichelassi@xxxxxxxxx>
> 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.
> @@ -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.
