t/pack-refs-tests: drop '-f' from test_path_is_missing

From: jayesh0104 <[email protected]>

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.

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

index 2fdaccb6c7..4a85d96c6b 100644
@@ -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

gitgitgadget

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,

...

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

---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2fdaccb6c7..4a85d96c6b 100644
@@ -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

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

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

---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2fdaccb6c7..4a85d96c6b 100644
@@ -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

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?

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

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

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.

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

index 2fdaccb6c7..4a85d96c6b 100644
@@ -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

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.

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

index 2fdaccb6c7..4a85d96c6b 100644
@@ -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

Did you already say that on the commit title?

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

From: jayesh0104 <[email protected]>

Using plain "test" commands in a sequence of checks chained with && makes it difficult to determine which step failed, as "test" silently succeeds or fails.

In this case, we expect that .git/refs/heads/f no longer exists. Replacing ! test -f with test_path_is_missing preserves the expected behavior when the file is absent, but provides a clearer diagnostic when it unexpectedly exists, making test failures easier to debug.

---
[GSoC] :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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2248/jayesh0104/fix-pack-refs-test-v2
Pull-Request: https://github.com/git/git/pull/2248

Range-diff vs v1:

1: b6b9d11ed8 ! 1: 82c31d9257 t/pack-refs-tests: drop '-f' from test_path_is_missing
@@ Metadata
Author: jayesh0104 <[email protected]>

## Commit message ##
- t/pack-refs-tests: drop '-f' from test_path_is_missing
+ tests: use test_path_is_missing instead of '! test -f'

- 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.
+ Using plain "test" commands in a sequence of checks chained
+ with && makes it difficult to determine which step failed,
+ as "test" silently succeeds or fails.

- 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.
+ In this case, we expect that .git/refs/heads/f no longer
+ exists. Replacing ! test -f with test_path_is_missing
+ preserves the expected behavior when the file is absent,
+ but provides a clearer diagnostic when it unexpectedly
+ exists, making test failures easier to debug.

- Remove the extraneous '-f' to use the helper correctly and
- restore proper test behavior.
-
- Signed-off-by: Jayesh Daga <[email protected]>
+ Signed-off-by: Jayesh Daga [[email protected]]

## t/pack-refs-tests.sh ##
@@ t/pack-refs-tests.sh: test_expect_success 'see if a branch still exists after git ${pack_refs} --prune

 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2fdaccb6c7..4a85d96c6b 100644
@@ -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

gitgitgadget

From: jayesh0104 <[email protected]>

Using plain "test" commands in a sequence of checks chained with && makes it difficult to determine which step failed, as "test" silently succeeds or fails.

In this case, we expect that .git/refs/heads/f no longer exists. Replacing ! test -f with test_path_is_missing preserves the expected behavior when the file is absent, but provides a clearer diagnostic when it unexpectedly exists, making test failures easier to debug.

---
 t/pack-refs-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2fdaccb6c7..4a85d96c6b 100644
@@ -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