[v2] t7110: replace `test -f` with `test_path_is_*` helpers

Commit Message

Matteo Bagnolini Jan. 3, 2025, 1 p.m. UTC

From: matteobagnolini <[e-mail]>

`test -f` and `! test -f` do not provide clear error messages when they fail.
To enhance debuggability, use `test_path_is_file` and `test_path_is_missing`,
which instead provide more informative error messages.

Note that `! test -f` checks if a path is not a file, while
`test_path_is_missing` verifies that a path does not exist. In this specific
case the tests are meant to check the absence of the path, making
`test_path_is_missing` a valid replacement.

Signed-off-by: Matteo Bagnolini <[e-mail]>
---
Sorry for avoidable spelling mistakes.
Updated commit message according to review.
 t/t7110-reset-merge.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Patrick Steinhardt Jan. 3, 2025, 1:12 p.m. UTC | #1

On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:
> From: matteobagnolini <[e-mail]>
> 
> `test -f` and `! test -f` do not provide clear error messages when they fail.
> To enhance debuggability, use `test_path_is_file` and `test_path_is_missing`,
> which instead provide more informative error messages.
> 
> Note that `! test -f` checks if a path is not a file, while
> `test_path_is_missing` verifies that a path does not exist. In this specific
> case the tests are meant to check the absence of the path, making
> `test_path_is_missing` a valid replacement.

Thanks, this version looks good to me.

Patrick

Junio C Hamano Jan. 3, 2025, 4:25 p.m. UTC | #2

Patrick Steinhardt <[e-mail]> writes:

> On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:
>> From: matteobagnolini <[e-mail]>

This must match the author ident on the Signed-off-by: line.

>> `test -f` and `! test -f` do not provide clear error messages when they fail.
>> To enhance debuggability, use `test_path_is_file` and `test_path_is_missing`,
>> which instead provide more informative error messages.
>> 
>> Note that `! test -f` checks if a path is not a file, while
>> `test_path_is_missing` verifies that a path does not exist. In this specific
>> case the tests are meant to check the absence of the path, making
>> `test_path_is_missing` a valid replacement.
>
> Thanks, this version looks good to me.
>
> Patrick

Thanks for writing, and thanks for reviewing.

Matteo Bagnolini Jan. 3, 2025, 5:27 p.m. UTC | #3

Junio C Hamano <[e-mail]> writes:

> Patrick Steinhardt <[e-mail]> writes:
>
> > On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:
> >> From: matteobagnolini <[e-mail]>
>
> This must match the author ident on the Signed-off-by: line.

So should I send another patch with this correction?
Sorry for questioning, but as you might have noticed, this is my first time
contributing and I'm slowly trying to learn the process.

Matteo

Junio C Hamano Jan. 3, 2025, 6:06 p.m. UTC | #4

Matteo Bagnolini <[e-mail]> writes:

> Junio C Hamano <[e-mail]> writes:
>
>> Patrick Steinhardt <[e-mail]> writes:
>>
>> > On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:
>> >> From: matteobagnolini <[e-mail]>
>>
>> This must match the author ident on the Signed-off-by: line.
>
> So should I send another patch with this correction?
> Sorry for questioning, but as you might have noticed, this is my first time
> contributing and I'm slowly trying to learn the process.

Thanks for asking; if there are no other things that need fixing, I
can fix up while queuing.

Matteo Bagnolini Jan. 3, 2025, 7:07 p.m. UTC | #5

Junio C Hamano <[e-mail]> writes:

> Thanks for asking; if there are no other things that need fixing, I
> can fix up while queuing.

Yes, everything else should be OK. Thank you.

Matteo

Patch

diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 61669a2d21..9a335071af 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -270,13 +270,13 @@  test_expect_success '--merge is ok with added/deleted merge' '
 	git reset --hard third &&
 	rm -f file2 &&
 	test_must_fail git merge branch3 &&
-	! test -f file2 &&
-	test -f file3 &&
+	test_path_is_missing file2 &&
+	test_path_is_file file3 &&
 	git diff --exit-code file3 &&
 	git diff --exit-code branch3 file3 &&
 	git reset --merge HEAD &&
-	! test -f file3 &&
-	! test -f file2 &&
+	test_path_is_missing file3 &&
+	test_path_is_missing file2 &&
 	git diff --exit-code --cached
 '
 
@@ -284,8 +284,8 @@  test_expect_success '--keep fails with added/deleted merge' '
 	git reset --hard third &&
 	rm -f file2 &&
 	test_must_fail git merge branch3 &&
-	! test -f file2 &&
-	test -f file3 &&
+	test_path_is_missing file2 &&
+	test_path_is_file file3 &&
 	git diff --exit-code file3 &&
 	git diff --exit-code branch3 file3 &&
 	test_must_fail git reset --keep HEAD 2>err.log &&