Re: [PATCH v2 07/11] t1405: mark test for 'git pack-refs' as REFFILES

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2021, 17:28 UTC
Message-ID: <xmqqsg05ndn9.fsf@gitster.g>
In-Reply-To: <05dead16f1cbc8d967edf101e170049bbe121cb4.1626989327.git.gitgitgadget@gmail.com>

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> The outcome of the pack-refs operation is not checked. This was apparently
> forgotten in the commit introducing this test: 16feb99d (Mar 26 2017, "t1405:
> some basic tests on main ref store").
>
> I tried adding the obvious check this, but it seems to fail on the freebsd_12
> builder.

These two paragraphs may state the truth, but readers would appreciate if it is clarified that they are both side notes that do not necessarily help understanding the change being made with the patch. Something like this

    The number of loose ref files under .git/refs/ directory before
    and after running the "git pack-refs" command only matter when
    the files ref-backend is in use, so hide the test behind the
    REFFILES prerequisite.

would be needed before these sidenotes.

With a larger context, as you said,

    diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
    index 92b0487324..3517f54961 100755
    --- a/t/t1405-main-ref-store.sh
    +++ b/t/t1405-main-ref-store.sh
    @@ -9,10 +9,14 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

     RUN="test-tool ref-store main"

    -test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
    -	test_commit one &&
    +
    +test_expect_success 'setup' '
    +	test_commit one
    +'
    +
    +test_expect_success REFFILES 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
            N=`find .git/refs -type f | wc -l` &&
            test "$N" != 0 &&
            $RUN pack-refs 3 &&
            N=`find .git/refs -type f | wc -l`
     '

we do count the number of regular files in .git/refs hierarchy after running "test-tool ref-store main pack-refs 3" (whatever that means).

I think your "obvious check" would have been something like the attached? How would that break? It is not the "'wc -l' pads the output with extra spaces" we often see on macOS, is it?

Thanks.

 t/t1405-main-ref-store.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git c/t/t1405-main-ref-store.sh w/t/t1405-main-ref-store.sh
index 3517f54961..9dc88d7580 100755
--- c/t/t1405-main-ref-store.sh
+++ w/t/t1405-main-ref-store.sh
@@ -15,10 +15,11 @@ test_expect_success 'setup' '
 '
 
 test_expect_success REFFILES 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
-	N=`find .git/refs -type f | wc -l` &&
-	test "$N" != 0 &&
+	N=$(find .git/refs -type f | wc -l) &&
+	test $N -ne 0 &&
 	$RUN pack-refs 3 &&
-	N=`find .git/refs -type f | wc -l`
+	N=$(find .git/refs -type f | wc -l) &&
+	test $N -eq 0
 '
 
 test_expect_success 'create_symref(FOO, refs/heads/main)' '