[PATCH] t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing.

[PATCH] t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing.

[matteobagnolini]

`test -f` and `! test -f` do not provide clear error messages when they fail.
To enanche debuggability, use `test_path_is_file` and `test_path_is_missing`,
which instead provides 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: matteobagnolini <matteobagnolini2003@gmail.com>
---
 t/t7110-reset-merge.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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 &&
-- 
2.39.2 (Apple Git-143)

Re: [PATCH] t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing.

[Patrick Steinhardt]

On Wed, Jan 01, 2025 at 11:59:14PM +0100, matteobagnolini wrote:

The commit subject is overly long. We typically try to restrict them to
at most 72 characters. Furthermore, the part after the subsystem should
start with a lower-case letter. Suggestion:

    t7110: replace `test -f` with `test_path_is_*` helpers

> `test -f` and `! test -f` do not provide clear error messages when they fail.
> To enanche debuggability, use `test_path_is_file` and `test_path_is_missing`,

s/enanche/enhance

> which instead provides more informative error messages.

s/provides/provide

It looks like you intended to start a new paragraph here. These should
typically be separated by an empty line.

> 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.

Makes sense.

> Signed-off-by: matteobagnolini <matteobagnolini2003@gmail.com>

We typically prefer proper legal names in the SOB.

> 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 &&

The changes themselves look obviously good to me, thanks!

Patrick

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

[Matteo Bagnolini]

From: matteobagnolini <matteobagnolini2003@gmail.com>

`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 <matteobagnolini2003@gmail.com>
---
Sorry for avoidable spelling mistakes.
Updated commit message according to review.
 t/t7110-reset-merge.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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 &&
-- 
2.39.2 (Apple Git-143)

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

[Patrick Steinhardt]

On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:
> From: matteobagnolini <matteobagnolini2003@gmail.com>
> 
> `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

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:
>> From: matteobagnolini <matteobagnolini2003@gmail.com>

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.

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

[Matteo Bagnolini]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
> > On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:
> >> From: matteobagnolini <matteobagnolini2003@gmail.com>
>
> 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

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

[Junio C Hamano]

Matteo Bagnolini <matteobagnolini2003@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:
>> >> From: matteobagnolini <matteobagnolini2003@gmail.com>
>>
>> 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.

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

[Matteo Bagnolini]

Junio C Hamano <gitster@pobox.com> 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