From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 13 Mar 2026 17:51:54 GMT
Subject: Re: [PATCH] Modernize pack-refs-tests.sh with git's standard command like test_path_is_file, etc
Message-ID: <xmqq4imj62it.fsf@gitster.g>
In-Reply-To: <20260313161808.1242-1-riteshjd75@gmail.com>

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.