On Sun, Mar 22, 2026 at 7:20 PM Jayesh Daga via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: jayesh0104 <jayeshdaga99@gmail.com>
>
> test_path_is_missing expects exactly one argument: the path to
> check for absence. Passing '-f' is incorrect and results in
> "bug in the test script: 1 param" during test execution.
>
> The '-f' flag appears to have been carried over from the
> equivalent 'test -f' usage, but test_path_is_missing does not
> accept such flags.
>
> Remove the extraneous '-f' to use the helper correctly and
> restore proper test behavior.
>
> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>


While the code itself is now fine in my eyes, you aren't actually
removing a -f flag here as described in the commit message.
In the diff, you are entirely replacing the raw command with the
test_path_is_missing helper.

I did a similar microproject earlier this year,
and you can look at my commit message here for a reference [1]

Also, if this is for your GSoC microproject,
you should probably add a tag in your patch subject line (something
like [GSoC] ).

One other thing I should mention: you should make sure to CC the mentors
for the specific project you are applying to so they see your work!
or if you think the change is directly based on someone's work you can
CC them as well.

I am happy to review the code and help out,
but just letting you know I am a fellow GSoC applicant and not an
official mentor.

Regards,
- Jayatheerth

1 - https://lore.kernel.org/git/CALE2CrS0Q2NS1DbFv4pyRQsuypu=KH6Kurs=m4yWrFbR9QosoA@mail.gmail.com/T/#mbbd865b0c73a93096df476621d485f15674f475b

Hi Jayesh,

> old mode 100755
> new mode 100644
> index fa27d43a58..4a85d96c6b
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -1,9 +1,3 @@
> -#!/bin/sh
> -
> -test_description='test pack-refs'
> -
> -. ./test-lib.sh
> -
>  pack_refs=${pack_refs:-pack-refs}

Above lines are included in the your 3/22/26 18:56 pm patch.

Here, you not only changed the file permission from 755 to 644, but also removed the shebang testing framework. That was clearly incorrect — fortunately, you seem to have realized this and sent another patch. ;)

> From: jayesh0104 <jayeshdaga99@gmail.com>
> > test_path_is_missing expects exactly one argument: the path to
> check for absence. Passing '-f' is incorrect and results in
> "bug in the test script: 1 param" during test execution.
> > The '-f' flag appears to have been carried over from the
> equivalent 'test -f' usage, but test_path_is_missing does not
> accept such flags.
> > Remove the extraneous '-f' to use the helper correctly and
> restore proper test behavior.
> > Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
> ---
>      t/pack-refs-tests: fix helper usage
>      >      >      High-level (Intent & Context)
>      =============================
>      >      The test script t/pack-refs-tests.sh has two issues that prevent it from
>      running correctly.
>      >      It uses: ! test -f .git/refs/heads/f
>      >      This is inconsistent with the Git test framework, where helper functions
>      such as test_path_is_missing should be used instead of raw test checks.
>      >      >      Low-level (Implementation & Justification)
>      ==========================================
>      >      Without sourcing test-lib.sh, the test framework is not initialized,
>      leading to errors such as: test_expect_success: not found
>      >      Replaced raw file check with the appropriate helper:
>      >      - ! test -f .git/refs/heads/f
>      + test_path_is_missing .git/refs/heads/f
>      >      >      >      Summary
>      =======
>      >      Replace test -f with test_path_is_missing
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2248%2Fjayesh0104%2Ffix-pack-refs-test-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2248/jayesh0104/fix-pack-refs-test-v1
> Pull-Request: https://github.com/git/git/pull/2248
> >   t/pack-refs-tests.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> > diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> index 2fdaccb6c7..4a85d96c6b 100644
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
>   test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
>   	git branch f &&
>   	git ${pack_refs} --all --prune &&
> -	! test -f .git/refs/heads/f
> +	test_path_is_missing .git/refs/heads/f
>   '
>   >   test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
> > base-commit: 6e8d538aab8fe4dd07ba9fb87b5c7edcfa5706ad

...

I have no objections to the changes mentioned above, but I think you should name this patch V2, which is the community standard. Also, I think it would be great if you replied to the reviewers.

Thanks,

Yuchen

Hi Tian Yuchen,

Thanks for the review!

You're absolutely right, the earlier version accidentally removed the
shebang and test framework lines along with changing the file mode.
That was unintended, and I corrected it in the updated patch.

I'll make sure to properly version future updates as v2.

I appreciate the guidance.

Thanks,
Jayesh

test_path_is_missing expects exactly one argument: the path to
check for absence. Passing '-f' is incorrect and results in
"bug in the test script: 1 param" during test execution.

The '-f' flag appears to have been carried over from the
equivalent 'test -f' usage, but test_path_is_missing does not
accept such flags.

Remove the extraneous '-f' to use the helper correctly and
restore proper test behavior.

v2:
- Fix unintended removal of shebang and test framework lines
- Keep file mode unchanged

Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 2fdaccb6c7..4a85d96c6b 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
 test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
 	git branch f &&
 	git ${pack_refs} --all --prune &&
-	! test -f .git/refs/heads/f
+	test_path_is_missing .git/refs/heads/f
 '
 
 test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
-- 
2.43.0

On Tue, Mar 24, 2026 at 12:22 AM jayesh0104 <jayeshdaga99@gmail.com> wrote:
> test_path_is_missing expects exactly one argument: the path to
> check for absence. Passing '-f' is incorrect and results in
> "bug in the test script: 1 param" during test execution.
>
> The '-f' flag appears to have been carried over from the
> equivalent 'test -f' usage, but test_path_is_missing does not
> accept such flags.
>
> Remove the extraneous '-f' to use the helper correctly and
> restore proper test behavior.

This commit message which talks about changing `test_path_is_missing
-f <path>` into `test_path_is_missing <path>`...

> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
> ---
> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
>         git branch f &&
>         git ${pack_refs} --all --prune &&
> -       ! test -f .git/refs/heads/f
> +       test_path_is_missing .git/refs/heads/f
>  '

...does not reflect the code change at all.

Replace the raw file existence check:

    ! test -f .git/refs/heads/f

with the Git test helper:

    test_path_is_missing .git/refs/heads/f

This aligns the test with Git’s testing conventions and avoids
direct use of shell test constructs.

v3:
- Fix commit message to accurately describe the change

Signed-off-by: jayesh0104 <jayeshdaga99@gmail.com>
---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 2fdaccb6c7..4a85d96c6b 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
 test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
 	git branch f &&
 	git ${pack_refs} --all --prune &&
-	! test -f .git/refs/heads/f
+	test_path_is_missing .git/refs/heads/f
 '
 
 test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
-- 
2.43.0

jayesh0104 <jayeshdaga99@gmail.com> writes:

> Replace the raw file existence check:
>
>     ! test -f .git/refs/heads/f
>
> with the Git test helper:
>
>     test_path_is_missing .git/refs/heads/f
>
> This aligns the test with Git’s testing conventions and avoids
> direct use of shell test constructs.

That makes it sound like "avoiding direct use" is a goal on its own.
Adhering to the conventions is good, but the ultimate reason is
something else, isn't it?

> v3:
> - Fix commit message to accurately describe the change

The above two lines plus a blank line should come below the three
dash line ...

> Signed-off-by: jayesh0104 <jayeshdaga99@gmail.com>
> ---

... and placed here.  After getting committed, "git log" readers
are not interested in learning how many wrong turns you took or what
mistake you made until you finally got to an acceptable patch.

The name of the game is to pretend as if you were a perfect
developer ;-).

>  t/pack-refs-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> index 2fdaccb6c7..4a85d96c6b 100644
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
>  	git branch f &&
>  	git ${pack_refs} --all --prune &&
> -	! test -f .git/refs/heads/f
> +	test_path_is_missing .git/refs/heads/f
>  '
>  
>  test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '

Replace a raw '! test -f' check with test_path_is_missing
to use the standard test helper.

This improves consistency with other tests and provides
better diagnostics on failure.

Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
---
v4:
- Correct commit message to match actual change
- Improve rationale (diagnostics, consistency)
- Move version notes below '---'
- Fix author name to match sign-off

v3:
- Fix commit message wording
---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 2fdaccb6c7..4a85d96c6b 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
 test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
 	git branch f &&
 	git ${pack_refs} --all --prune &&
-	! test -f .git/refs/heads/f
+	test_path_is_missing .git/refs/heads/f
 '
 
 test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
-- 
2.43.0

On 3/25/26 00:12, Jayesh Daga wrote:
> Replace a raw '! test -f' check with test_path_is_missing
> to use the standard test helper.
> > This improves consistency with other tests and provides
> better diagnostics on failure.
> > Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>

I think what Junio meant is that it would be better if you explain in more detail *why* such change is nice.

For example, under what specific circumstances might the original approach lead to bugs? How does the new approach address this issue? What exactly do the codes do?

To me, phrases like “improving consistency” and “provides better diagnostics” are essentially empty rhetoric unless they are backed up by the specific explanations. Even though this is just a simple one-line change, I think the principle still applies here — if a future developer (let say 50 years from now, human programmers will no longer be writing shell scripts by hand) sees this code, he/she likely won’t be able to quickly understand the intent and purpose of the change just from the commit message, right? :P

Regards, Yuchen

Replace a raw '! test -f' check with test_path_is_missing.

The test_path_is_missing helper integrates with Git’s test
framework and produces clearer failure output. In contrast,
a plain shell '! test -f' check only reports a generic failure
status, which makes it harder to understand whether the file
unexpectedly exists or if another issue caused the test to fail.

It also avoids relying on negated shell conditions, making the
test easier to read and understand.

Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
---
v5:
- Clarify rationale for using test helper
- Explain diagnostic improvement and negation issues
- Address review comments on vague wording

v4:
- Correct commit message to match actual change
- Improve rationale (diagnostics, consistency)
- Move version notes below '---'
- Fix author name to match sign-off

v3:
- Fix commit message wording
---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 2fdaccb6c7..4a85d96c6b 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
 test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
 	git branch f &&
 	git ${pack_refs} --all --prune &&
-	! test -f .git/refs/heads/f
+	test_path_is_missing .git/refs/heads/f
 '
 
 test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
-- 
2.43.0

Jayesh Daga <jayeshdaga99@gmail.com> writes:

> Replace a raw '! test -f' check with test_path_is_missing.

Did you already say that on the commit title?

> The test_path_is_missing helper integrates with Git’s test
> framework and produces clearer failure output.
>
> In contrast,
> a plain shell '! test -f' check only reports a generic failure
> status, which makes it harder to understand whether the file
> unexpectedly exists or if another issue caused the test to fail.

"clearer" probably is not clear enough, but don't add more words on
it.

The problem with using "test", whether negated or not, is that they
*silently* succeed or fail.  Take a typical test that does a bunch
of things like this ...

	do something &&
	do something else &&
	test -f this_must_be_a_file &&
	test ! -e this_must_not_exist &&
	do yet another thing &&
	! test -d this_should_not_be_a_directory

... and expects all of them to succeed.  If it fails in one of the
steps, it is impossible to see from the test output, even when you
are running with the "-v" option , e.g., "sh t/0601-*.sh -v", where
in the sequence it failed.  Maybe "do something" and "do something
else" shows different messages so you can tell these two steps
succeeded, but did the test fail because this_must_be_a_file did not
exist, or was it because a filesystem entity this_must_not_exist
existed?

Our test helpers improve by being loud when the expectation is not
met.  When "test ! -e this_must_not_exist" is rewritten with
"test_path_is_missing this_must_not_exist", and when that thing is
missing from the filesystem, test_path_is_missing will succeed
silently.  But whe it exists, it loudly reports "We did not want to
see it, but it exists!", when it fails.

    Using plain "test" commands in a series of tests concatenated
    with && makes it hard to tell from the failure output which one
    of the steps failed, since "test" silently succeeds and fails.

    In this partciular instance, we expect that ".git/refs/heads/f"
    should no longer exist in the filesystem.  test_path_is_missing
    helper function silently succeeds, as does "! test -f", when it
    finds that the file is not there, but it will loudly report when
    the file exists, contrary to our expectation, which makes it
    easier to debug a test failure.

or something like that.

> It also avoids relying on negated shell conditions, making the
> test easier to read and understand.

It is not a single test being "hard to understand".  As a developer,
you are expected to know what "! test -f .git/refs/heads/f" expects
(i.e., it does not want to see a file there).


> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
> ---
> v5:
> - Clarify rationale for using test helper
> - Explain diagnostic improvement and negation issues
> - Address review comments on vague wording
>
> v4:
> - Correct commit message to match actual change
> - Improve rationale (diagnostics, consistency)
> - Move version notes below '---'
> - Fix author name to match sign-off
>
> v3:
> - Fix commit message wording
> ---
>  t/pack-refs-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> index 2fdaccb6c7..4a85d96c6b 100644
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
>  	git branch f &&
>  	git ${pack_refs} --all --prune &&
> -	! test -f .git/refs/heads/f
> +	test_path_is_missing .git/refs/heads/f
>  '
>  
>  test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '

On Sun, Mar 22, 2026 at 7:20 PM Jayesh Daga via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: jayesh0104 <jayeshdaga99@gmail.com>
>
> test_path_is_missing expects exactly one argument: the path to
> check for absence. Passing '-f' is incorrect and results in
> "bug in the test script: 1 param" during test execution.
>
> The '-f' flag appears to have been carried over from the
> equivalent 'test -f' usage, but test_path_is_missing does not
> accept such flags.
>
> Remove the extraneous '-f' to use the helper correctly and
> restore proper test behavior.
>
> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>


While the code itself is now fine in my eyes, you aren't actually
removing a -f flag here as described in the commit message.
In the diff, you are entirely replacing the raw command with the
test_path_is_missing helper.

I did a similar microproject earlier this year,
and you can look at my commit message here for a reference [1]

Also, if this is for your GSoC microproject,
you should probably add a tag in your patch subject line (something
like [GSoC] ).

One other thing I should mention: you should make sure to CC the mentors
for the specific project you are applying to so they see your work!
or if you think the change is directly based on someone's work you can
CC them as well.

I am happy to review the code and help out,
but just letting you know I am a fellow GSoC applicant and not an
official mentor.

Regards,
- Jayatheerth

1 - https://lore.kernel.org/git/CALE2CrS0Q2NS1DbFv4pyRQsuypu=KH6Kurs=m4yWrFbR9QosoA@mail.gmail.com/T/#mbbd865b0c73a93096df476621d485f15674f475b

Hi Jayesh,

> old mode 100755
> new mode 100644
> index fa27d43a58..4a85d96c6b
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -1,9 +1,3 @@
> -#!/bin/sh
> -
> -test_description='test pack-refs'
> -
> -. ./test-lib.sh
> -
>  pack_refs=${pack_refs:-pack-refs}

Above lines are included in the your 3/22/26 18:56 pm patch.

Here, you not only changed the file permission from 755 to 644, but also removed the shebang testing framework. That was clearly incorrect — fortunately, you seem to have realized this and sent another patch. ;)

> From: jayesh0104 <jayeshdaga99@gmail.com>
> > test_path_is_missing expects exactly one argument: the path to
> check for absence. Passing '-f' is incorrect and results in
> "bug in the test script: 1 param" during test execution.
> > The '-f' flag appears to have been carried over from the
> equivalent 'test -f' usage, but test_path_is_missing does not
> accept such flags.
> > Remove the extraneous '-f' to use the helper correctly and
> restore proper test behavior.
> > Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
> ---
>      t/pack-refs-tests: fix helper usage
>      >      >      High-level (Intent & Context)
>      =============================
>      >      The test script t/pack-refs-tests.sh has two issues that prevent it from
>      running correctly.
>      >      It uses: ! test -f .git/refs/heads/f
>      >      This is inconsistent with the Git test framework, where helper functions
>      such as test_path_is_missing should be used instead of raw test checks.
>      >      >      Low-level (Implementation & Justification)
>      ==========================================
>      >      Without sourcing test-lib.sh, the test framework is not initialized,
>      leading to errors such as: test_expect_success: not found
>      >      Replaced raw file check with the appropriate helper:
>      >      - ! test -f .git/refs/heads/f
>      + test_path_is_missing .git/refs/heads/f
>      >      >      >      Summary
>      =======
>      >      Replace test -f with test_path_is_missing
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2248%2Fjayesh0104%2Ffix-pack-refs-test-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2248/jayesh0104/fix-pack-refs-test-v1
> Pull-Request: https://github.com/git/git/pull/2248
> >   t/pack-refs-tests.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> > diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> index 2fdaccb6c7..4a85d96c6b 100644
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
>   test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
>   	git branch f &&
>   	git ${pack_refs} --all --prune &&
> -	! test -f .git/refs/heads/f
> +	test_path_is_missing .git/refs/heads/f
>   '
>   >   test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
> > base-commit: 6e8d538aab8fe4dd07ba9fb87b5c7edcfa5706ad

...

I have no objections to the changes mentioned above, but I think you should name this patch V2, which is the community standard. Also, I think it would be great if you replied to the reviewers.

Thanks,

Yuchen

Hi Tian Yuchen,

Thanks for the review!

You're absolutely right, the earlier version accidentally removed the
shebang and test framework lines along with changing the file mode.
That was unintended, and I corrected it in the updated patch.

I'll make sure to properly version future updates as v2.

I appreciate the guidance.

Thanks,
Jayesh

test_path_is_missing expects exactly one argument: the path to
check for absence. Passing '-f' is incorrect and results in
"bug in the test script: 1 param" during test execution.

The '-f' flag appears to have been carried over from the
equivalent 'test -f' usage, but test_path_is_missing does not
accept such flags.

Remove the extraneous '-f' to use the helper correctly and
restore proper test behavior.

v2:
- Fix unintended removal of shebang and test framework lines
- Keep file mode unchanged

Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 2fdaccb6c7..4a85d96c6b 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
 test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
 	git branch f &&
 	git ${pack_refs} --all --prune &&
-	! test -f .git/refs/heads/f
+	test_path_is_missing .git/refs/heads/f
 '
 
 test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
-- 
2.43.0

On Tue, Mar 24, 2026 at 12:22 AM jayesh0104 <jayeshdaga99@gmail.com> wrote:
> test_path_is_missing expects exactly one argument: the path to
> check for absence. Passing '-f' is incorrect and results in
> "bug in the test script: 1 param" during test execution.
>
> The '-f' flag appears to have been carried over from the
> equivalent 'test -f' usage, but test_path_is_missing does not
> accept such flags.
>
> Remove the extraneous '-f' to use the helper correctly and
> restore proper test behavior.

This commit message which talks about changing `test_path_is_missing
-f <path>` into `test_path_is_missing <path>`...

> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
> ---
> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
>         git branch f &&
>         git ${pack_refs} --all --prune &&
> -       ! test -f .git/refs/heads/f
> +       test_path_is_missing .git/refs/heads/f
>  '

...does not reflect the code change at all.

Replace the raw file existence check:

    ! test -f .git/refs/heads/f

with the Git test helper:

    test_path_is_missing .git/refs/heads/f

This aligns the test with Git’s testing conventions and avoids
direct use of shell test constructs.

v3:
- Fix commit message to accurately describe the change

Signed-off-by: jayesh0104 <jayeshdaga99@gmail.com>
---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 2fdaccb6c7..4a85d96c6b 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
 test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
 	git branch f &&
 	git ${pack_refs} --all --prune &&
-	! test -f .git/refs/heads/f
+	test_path_is_missing .git/refs/heads/f
 '
 
 test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
-- 
2.43.0

jayesh0104 <jayeshdaga99@gmail.com> writes:

> Replace the raw file existence check:
>
>     ! test -f .git/refs/heads/f
>
> with the Git test helper:
>
>     test_path_is_missing .git/refs/heads/f
>
> This aligns the test with Git’s testing conventions and avoids
> direct use of shell test constructs.

That makes it sound like "avoiding direct use" is a goal on its own.
Adhering to the conventions is good, but the ultimate reason is
something else, isn't it?

> v3:
> - Fix commit message to accurately describe the change

The above two lines plus a blank line should come below the three
dash line ...

> Signed-off-by: jayesh0104 <jayeshdaga99@gmail.com>
> ---

... and placed here.  After getting committed, "git log" readers
are not interested in learning how many wrong turns you took or what
mistake you made until you finally got to an acceptable patch.

The name of the game is to pretend as if you were a perfect
developer ;-).

>  t/pack-refs-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> index 2fdaccb6c7..4a85d96c6b 100644
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
>  	git branch f &&
>  	git ${pack_refs} --all --prune &&
> -	! test -f .git/refs/heads/f
> +	test_path_is_missing .git/refs/heads/f
>  '
>  
>  test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '

Replace a raw '! test -f' check with test_path_is_missing
to use the standard test helper.

This improves consistency with other tests and provides
better diagnostics on failure.

Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
---
v4:
- Correct commit message to match actual change
- Improve rationale (diagnostics, consistency)
- Move version notes below '---'
- Fix author name to match sign-off

v3:
- Fix commit message wording
---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 2fdaccb6c7..4a85d96c6b 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
 test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
 	git branch f &&
 	git ${pack_refs} --all --prune &&
-	! test -f .git/refs/heads/f
+	test_path_is_missing .git/refs/heads/f
 '
 
 test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
-- 
2.43.0

On 3/25/26 00:12, Jayesh Daga wrote:
> Replace a raw '! test -f' check with test_path_is_missing
> to use the standard test helper.
> > This improves consistency with other tests and provides
> better diagnostics on failure.
> > Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>

I think what Junio meant is that it would be better if you explain in more detail *why* such change is nice.

For example, under what specific circumstances might the original approach lead to bugs? How does the new approach address this issue? What exactly do the codes do?

To me, phrases like “improving consistency” and “provides better diagnostics” are essentially empty rhetoric unless they are backed up by the specific explanations. Even though this is just a simple one-line change, I think the principle still applies here — if a future developer (let say 50 years from now, human programmers will no longer be writing shell scripts by hand) sees this code, he/she likely won’t be able to quickly understand the intent and purpose of the change just from the commit message, right? :P

Regards, Yuchen

Replace a raw '! test -f' check with test_path_is_missing.

The test_path_is_missing helper integrates with Git’s test
framework and produces clearer failure output. In contrast,
a plain shell '! test -f' check only reports a generic failure
status, which makes it harder to understand whether the file
unexpectedly exists or if another issue caused the test to fail.

It also avoids relying on negated shell conditions, making the
test easier to read and understand.

Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
---
v5:
- Clarify rationale for using test helper
- Explain diagnostic improvement and negation issues
- Address review comments on vague wording

v4:
- Correct commit message to match actual change
- Improve rationale (diagnostics, consistency)
- Move version notes below '---'
- Fix author name to match sign-off

v3:
- Fix commit message wording
---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 2fdaccb6c7..4a85d96c6b 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
 test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
 	git branch f &&
 	git ${pack_refs} --all --prune &&
-	! test -f .git/refs/heads/f
+	test_path_is_missing .git/refs/heads/f
 '
 
 test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
-- 
2.43.0

Jayesh Daga <jayeshdaga99@gmail.com> writes:

> Replace a raw '! test -f' check with test_path_is_missing.

Did you already say that on the commit title?

> The test_path_is_missing helper integrates with Git’s test
> framework and produces clearer failure output.
>
> In contrast,
> a plain shell '! test -f' check only reports a generic failure
> status, which makes it harder to understand whether the file
> unexpectedly exists or if another issue caused the test to fail.

"clearer" probably is not clear enough, but don't add more words on
it.

The problem with using "test", whether negated or not, is that they
*silently* succeed or fail.  Take a typical test that does a bunch
of things like this ...

	do something &&
	do something else &&
	test -f this_must_be_a_file &&
	test ! -e this_must_not_exist &&
	do yet another thing &&
	! test -d this_should_not_be_a_directory

... and expects all of them to succeed.  If it fails in one of the
steps, it is impossible to see from the test output, even when you
are running with the "-v" option , e.g., "sh t/0601-*.sh -v", where
in the sequence it failed.  Maybe "do something" and "do something
else" shows different messages so you can tell these two steps
succeeded, but did the test fail because this_must_be_a_file did not
exist, or was it because a filesystem entity this_must_not_exist
existed?

Our test helpers improve by being loud when the expectation is not
met.  When "test ! -e this_must_not_exist" is rewritten with
"test_path_is_missing this_must_not_exist", and when that thing is
missing from the filesystem, test_path_is_missing will succeed
silently.  But whe it exists, it loudly reports "We did not want to
see it, but it exists!", when it fails.

    Using plain "test" commands in a series of tests concatenated
    with && makes it hard to tell from the failure output which one
    of the steps failed, since "test" silently succeeds and fails.

    In this partciular instance, we expect that ".git/refs/heads/f"
    should no longer exist in the filesystem.  test_path_is_missing
    helper function silently succeeds, as does "! test -f", when it
    finds that the file is not there, but it will loudly report when
    the file exists, contrary to our expectation, which makes it
    easier to debug a test failure.

or something like that.

> It also avoids relying on negated shell conditions, making the
> test easier to read and understand.

It is not a single test being "hard to understand".  As a developer,
you are expected to know what "! test -f .git/refs/heads/f" expects
(i.e., it does not want to see a file there).


> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
> ---
> v5:
> - Clarify rationale for using test helper
> - Explain diagnostic improvement and negation issues
> - Address review comments on vague wording
>
> v4:
> - Correct commit message to match actual change
> - Improve rationale (diagnostics, consistency)
> - Move version notes below '---'
> - Fix author name to match sign-off
>
> v3:
> - Fix commit message wording
> ---
>  t/pack-refs-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> index 2fdaccb6c7..4a85d96c6b 100644
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
>  	git branch f &&
>  	git ${pack_refs} --all --prune &&
> -	! test -f .git/refs/heads/f
> +	test_path_is_missing .git/refs/heads/f
>  '
>  
>  test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '