[PATCH] Modernize pack-refs-tests.sh with git's standard command like test_path_is_file, etc

[PATCH] Modernize pack-refs-tests.sh with git's standard command like test_path_is_file, etc

[Ritesh Singh Jadoun]

---
 t/pack-refs-tests.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 2fdaccb6c7..dca0c77ca1 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -61,13 +61,13 @@ 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_file .git/refs/heads/f
 '
 
 test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '
 	git branch r/s/t &&
 	git ${pack_refs} --all --prune &&
-	! test -e .git/refs/heads/r
+	! test_path_exists .git/refs/heads/r
 '
 
 test_expect_success 'git branch g should work when git branch g/h has been deleted' '
@@ -111,43 +111,43 @@ test_expect_success 'test excluded refs are not packed' '
 	git branch dont_pack2 &&
 	git branch pack_this &&
 	git ${pack_refs} --all --exclude "refs/heads/dont_pack*" &&
-	test -f .git/refs/heads/dont_pack1 &&
-	test -f .git/refs/heads/dont_pack2 &&
-	! test -f .git/refs/heads/pack_this'
+	test_path_is_file .git/refs/heads/dont_pack1 &&
+	test_path_is_file .git/refs/heads/dont_pack2 &&
+	! test_path_is_file .git/refs/heads/pack_this'
 
 test_expect_success 'test --no-exclude refs clears excluded refs' '
 	git branch dont_pack3 &&
 	git branch dont_pack4 &&
 	git ${pack_refs} --all --exclude "refs/heads/dont_pack*" --no-exclude &&
-	! test -f .git/refs/heads/dont_pack3 &&
-	! test -f .git/refs/heads/dont_pack4'
+	! test_path_is_file .git/refs/heads/dont_pack3 &&
+	! test_path_is_file .git/refs/heads/dont_pack4'
 
 test_expect_success 'test only included refs are packed' '
 	git branch pack_this1 &&
 	git branch pack_this2 &&
 	git tag dont_pack5 &&
 	git ${pack_refs} --include "refs/heads/pack_this*" &&
-	test -f .git/refs/tags/dont_pack5 &&
-	! test -f .git/refs/heads/pack_this1 &&
-	! test -f .git/refs/heads/pack_this2'
+	test_path_is_file .git/refs/tags/dont_pack5 &&
+	! test_path_is_file .git/refs/heads/pack_this1 &&
+	! test_path_is_file .git/refs/heads/pack_this2'
 
 test_expect_success 'test --no-include refs clears included refs' '
 	git branch pack1 &&
 	git branch pack2 &&
 	git ${pack_refs} --include "refs/heads/pack*" --no-include &&
-	test -f .git/refs/heads/pack1 &&
-	test -f .git/refs/heads/pack2'
+	test_path_is_file .git/refs/heads/pack1 &&
+	test_path_is_file .git/refs/heads/pack2'
 
 test_expect_success 'test --exclude takes precedence over --include' '
 	git branch dont_pack5 &&
 	git ${pack_refs} --include "refs/heads/pack*" --exclude "refs/heads/pack*" &&
-	test -f .git/refs/heads/dont_pack5'
+	test_path_is_file .git/refs/heads/dont_pack5'
 
 test_expect_success 'see if up-to-date packed refs are preserved' '
 	git branch q &&
 	git ${pack_refs} --all --prune &&
 	git update-ref refs/heads/q refs/heads/q &&
-	! test -f .git/refs/heads/q
+	! test_path_is_file .git/refs/heads/q
 '
 
 test_expect_success 'pack, prune and repack' '
-- 
2.46.0.windows.1

Re: [PATCH] Modernize pack-refs-tests.sh with git's standard command like test_path_is_file, etc

[Junio C Hamano]

Ritesh Singh Jadoun <riteshjd75@gmail.com> writes:

> Subject: Re: [PATCH] Modernize pack-refs-tests.sh with git's standard command like test_path_is_file, etc

Unusual patch title.

No justification given for these changes in the proposed log message.

Missing sign-off.

> ---
>  t/pack-refs-tests.sh | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)

Check CodingGuidelines and SubmittingPatches (both found in
the Documentation/ directory).

> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> index 2fdaccb6c7..dca0c77ca1 100644
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -61,13 +61,13 @@ 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_file .git/refs/heads/f
>  '

The point of "test_path_is_file" is "we expect this path to be a
file and there is something wrong if it isn't and we should report
to the person who is running the test loudly".  That is why

	test_path_is_file existing-file

is silent, while

	test_path_is_file missing-file
	test_path_is_file existing-directory/

both loudly report the failure.

But in this test, that expects "! test -f .git/refs/heads/f" to be
true, the story is the other way around.  The test expects that the
loose ref file for the branch 'f' on the filesystem should be gone.
In other words, it is not a notable event if .git/refs/heads/f did
*NOT* exist, and if .git/refs/heads/f existed, that is something you
want to report loudly, now you are using a better helper function.

I think the update should use test_path_is_missing instead, without
negation.

I did not look at the rest of the patch, but the above should be
a sufficient guideline to decide what replacement should be used.
Be careful to the original that uses negation and you'd do fine.

Thanks.