From: Tian Yuchen <a3205153416@gmail.com>
Date: Sun, 22 Mar 2026 16:37:12 GMT
Subject: Re: [PATCH] t/pack-refs-tests: drop '-f' from test_path_is_missing
Message-ID: <a26599ba-01b0-4587-ba0c-bd28a822c615@gmail.com>
In-Reply-To: <pull.2248.git.git.1774187447563.gitgitgadget@gmail.com>

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