Re: [PATCH] t9811: replace 'test -f' and '! test -f' with 'test_path_*'

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jul 02, 2026 at 11:07:04AM -0300, Marcelo Machado Lage wrote:
> Replace the basic shell commands 'test -f', with more modern test
> helpers 'test_path_is_file' and 'test_path_is_missing'.

Nit: it might make sense to briefly mention why we do this exercise.
Like, what does `test_path_is_file` et al give us over `test -f`?

> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
> index 7614dfbd95..93d6b4c479 100755
> --- a/t/t9811-git-p4-label-import.sh
> +++ b/t/t9811-git-p4-label-import.sh
> @@ -62,9 +62,9 @@ test_expect_success 'basic p4 labels' '
>  
>  		cd main &&
>  		git checkout TAG_F1_ONLY &&
> -		! test -f f2 &&
> +		test_path_is_missing f2 &&
>  		git checkout TAG_WITH\$_SHELL_CHAR &&
> -		test -f f1 && test -f f2 && test -f file_with_\$metachar &&
> +		test_path_is_file f1 && test_path_is_file f2 && test_path_is_file file_with_\$metachar &&

While at it we could split this line into three lines -- it's getting
overly long, and we typically don't chain multiple commands on one line
nowadays.

> @@ -135,9 +135,9 @@ test_expect_success 'export git tags to p4' '
>  		p4 labels ... | grep LIGHTWEIGHT_TAG &&
>  		p4 label -o GIT_TAG_1 | grep "tag created in git:xyzzy" &&
>  		p4 sync ...@GIT_TAG_1 &&
> -		! test -f main/f10 &&
> +		test_path_is_missing main/f10 &&

This is a stronger guarantee compared to before, as we only checked
whether the path is not a file. Now we verify that it doesn't exist at
all, which would be equivalent to `test -e`. That's a strict improvement
though, but may be worth pointing out in the commit message so that the
reviewer is not surprised.

> @@ -168,9 +168,9 @@ test_expect_success 'export git tags to p4 with deletion' '
>  		cd "$cli" &&
>  		p4 sync ... &&
>  		p4 sync ...@GIT_TAG_ON_DELETED &&
> -		test -f main/deleted_file &&
> +		test_path_is_file main/deleted_file &&
>  		p4 sync ...@GIT_TAG_AFTER_DELETION &&
> -		! test -f main/deleted_file &&
> +		test_path_is_missing main/deleted_file &&

Same here.

Other than that the patch looks good to me, thanks!

Patrick