threads / patch / 62721

patcht7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing.

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

## tl;dr

8 messages between Jan 1, 2025 and Jan 3, 2025. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

matteobagnolini· Jan 1, 2025, 22:59 UTC · lore

`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(-)

Show changes to t/t7110-reset-merge.sh +6 −6

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)

Patrick Steinhardt· Jan 3, 2025, 11:41 UTC · re: matteobagnolini · lore

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

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.

Show 33 quoted lines

> 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

Matteo Bagnolini· Jan 3, 2025, 13:00 UTC · re: Patrick Steinhardt · lore

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

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(-)

Show changes to t/t7110-reset-merge.sh +6 −6

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)

Patrick Steinhardt· Jan 3, 2025, 13:12 UTC · re: Matteo Bagnolini · lore

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

On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:

Show 10 quoted lines

> 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

Junio C Hamano· Jan 3, 2025, 16:25 UTC · re: Patrick Steinhardt · lore

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

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.

Show 12 quoted lines

>> `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, 17:27 UTC · re: Junio C Hamano · lore

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

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

Show 6 quoted lines

> 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

Junio C Hamano· Jan 3, 2025, 18:06 UTC · re: Matteo Bagnolini · lore

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

Matteo Bagnolini <matteobagnolini2003@gmail.com> writes:

Show 12 quoted lines

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

Matteo Bagnolini· Jan 3, 2025, 19:07 UTC · re: Junio C Hamano · lore

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

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