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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 31, 2021, 23:36 UTC
Message-ID: <87sg221nb0.fsf@evledraar.gmail.com>
In-Reply-To: <CAFQ2z_N=RtSeD6f3a2VPVdDUBxFS1ekC=na=5iVcsWOpkDdORw@mail.gmail.com>

On Mon, May 31 2021, Han-Wen Nienhuys wrote:

Show 22 quoted lines

> 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...