diff mbox series

[v2,21/21] t1415: set REFFILES for test specific to storage format

Message ID d7e5de8dba465098afa421d162df5ca7b0fb1a33.1619519903.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Prepare tests for reftable backend | expand

Commit Message

Han-Wen Nienhuys April 27, 2021, 10:38 a.m. UTC 
From: Han-Wen Nienhuys <hanwen@google.com>

Packing refs (and therefore checking that certain refs are not packed) is a
property of the packed/loose ref storage. Add a comment to explain what the test
checks.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/t1415-worktree-refs.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Ævar Arnfjörð Bjarmason May 20, 2021, 3:50 p.m. UTC | #1 
On Tue, Apr 27 2021, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Packing refs (and therefore checking that certain refs are not packed) is a
> property of the packed/loose ref storage. Add a comment to explain what the test
> checks.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  t/t1415-worktree-refs.sh | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh
> index 7ab91241ab7c..d6e6e523bbba 100755
> --- a/t/t1415-worktree-refs.sh
> +++ b/t/t1415-worktree-refs.sh
> @@ -16,7 +16,10 @@ test_expect_success 'setup' '
>  	git -C wt2 update-ref refs/worktree/foo HEAD
>  '
>  
> -test_expect_success 'refs/worktree must not be packed' '
> +# The 'packed-refs' files is stored directly in .git/. This means it is global
> +# to the repository, and can only contain refs that are shared across all
> +# worktrees.
> +test_expect_success REFFILES 'refs/worktree must not be packed' '
>  	git pack-refs --all &&
>  	test_path_is_missing .git/refs/tags/wt1 &&
>  	test_path_is_file .git/refs/worktree/foo &&

Nit but also chicken & egg: Let's keep the "pack-refs --all" though
under reftable in its own test, and do the test_path_* assertions just
under REFFILES, i.e. does/should pack-refs warn/error under reftable as
redundant, succeed silently?
Han-Wen Nienhuys May 31, 2021, 2:16 p.m. UTC | #2 
On Thu, May 20, 2021 at 5:51 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > -test_expect_success 'refs/worktree must not be packed' '
> > +# The 'packed-refs' files is stored directly in .git/. This means it is global
> > +# to the repository, and can only contain refs that are shared across all
> > +# worktrees.
> > +test_expect_success REFFILES 'refs/worktree must not be packed' '
> >       git pack-refs --all &&
> >       test_path_is_missing .git/refs/tags/wt1 &&
> >       test_path_is_file .git/refs/worktree/foo &&
>
> Nit but also chicken & egg: Let's keep the "pack-refs --all" though
> under reftable in its own test, and do the test_path_* assertions just
> under REFFILES, i.e. does/should pack-refs warn/error under reftable as
> redundant, succeed silently?

I don't understand your comment. This test is checking constraints on
worktrees that are only relevant for loose/packed storage. In fact,
under reftable, there is no such thing as a "packed ref" storage
class. This test is just not relevant for reftable, and I tried
explaining in the comment why this is so. Happy to hear how to further
clarify this comment.
Ævar Arnfjörð Bjarmason May 31, 2021, 11:36 p.m. UTC | #3 
On Mon, May 31 2021, Han-Wen Nienhuys wrote:

> On Thu, May 20, 2021 at 5:51 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> > -test_expect_success 'refs/worktree must not be packed' '
>> > +# The 'packed-refs' files is stored directly in .git/. This means it is global
>> > +# to the repository, and can only contain refs that are shared across all
>> > +# worktrees.
>> > +test_expect_success REFFILES 'refs/worktree must not be packed' '
>> >       git pack-refs --all &&
>> >       test_path_is_missing .git/refs/tags/wt1 &&
>> >       test_path_is_file .git/refs/worktree/foo &&
>>
>> Nit but also chicken & egg: Let's keep the "pack-refs --all" though
>> under reftable in its own test, and do the test_path_* assertions just
>> under REFFILES, i.e. does/should pack-refs warn/error under reftable as
>> redundant, succeed silently?
>
> I don't understand your comment. This test is checking constraints on
> worktrees that are only relevant for loose/packed storage. In fact,
> under reftable, there is no such thing as a "packed ref" storage
> class. This test is just not relevant for reftable, and I tried
> explaining in the comment why this is so. Happy to hear how to further
> clarify this comment.

The chicken & egg part is that reftable isn't landed in-tree yet, so
it's probably pointless to discuss at this point.

But what I was getting at is that while the test when written was trying
to test that the file backend is doing some particular thing, when we go
through our tests to find those cases we should at least make a mental
note (hence my E-Mail) to eventually positively assert the new behavior.

I.e. if pack-refs is a noop shouldn't it warn or something, and then the
tests should check if reftable warn, else do_old_stuff().

In this case one can imagine that e.g. a naïve buggy implementation
would check if .git/reftable or whatever exists, which of course with
".git" being a plain file in worktrees won't work, so testing this
particular case somewhere in the worktree tests is probably prudent...
diff mbox series

Patch

diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh
index 7ab91241ab7c..d6e6e523bbba 100755
--- a/t/t1415-worktree-refs.sh
+++ b/t/t1415-worktree-refs.sh
@@ -16,7 +16,10 @@  test_expect_success 'setup' '
 	git -C wt2 update-ref refs/worktree/foo HEAD
 '
 
-test_expect_success 'refs/worktree must not be packed' '
+# The 'packed-refs' files is stored directly in .git/. This means it is global
+# to the repository, and can only contain refs that are shared across all
+# worktrees.
+test_expect_success REFFILES 'refs/worktree must not be packed' '
 	git pack-refs --all &&
 	test_path_is_missing .git/refs/tags/wt1 &&
 	test_path_is_file .git/refs/worktree/foo &&