Re: [PATCH 4/6] t1419: mark test suite as files-backend specific

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 10, 2024, 07:30 UTC
Message-ID: <ZZ5HrY76O1x8QABG@tanuki>
In-Reply-To: <CAPig+cTAiEFu9p1nRe9LC3mxyPmfQ9m4r7aQUj_9OC8pSbwbig@mail.gmail.com>

On Tue, Jan 09, 2024 at 02:40:50PM -0500, Eric Sunshine wrote:

Show 17 quoted lines

> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
> > excluded pattern(s), 2023-07-10) we have implemented logic to handle
> > excluded refs more efficiently in the "packed" ref backend. This logic
> > allows us to skip emitting refs completely which we know to not be of
> > any interest to the caller, which can avoid quite some allocaitons and
> > object lookups.
> 
> s/allocaitons/allocations/
> 
> > This was wired up via a new `exclude_patterns` parameter passed to the
> > backend's ref iterator. The backend only needs to handle them on a best
> > effort basis though, and in fact we only handle it for the "packed-refs"
> > file, but not for loose references. Consequentially, all callers must
> > still filter emitted refs with those exclude patterns.
> 
> s/Consequentially/Consequently/

Hum. I had the last time when you mentioned the in mind while writing the commit message, but seemingly misremembered the outcome. So I now looked things up in a dictionary, and both words seem to be used in equivalent ways. As a non-native speaker, could you maybe elaborate a bit to help me out? :)

Show 8 quoted lines

> > The result is that handling exclude patterns is completely optional in
> > the ref backend, and any future backends may or may not implement it.
> > Let's thus mark the test for t1419 to depend on the REFFILES prereq.
> 
> This change seems to be abusing the meaning of the REFFILES
> prerequisite. Instead the above description argues for introduction of
> a new prerequisite which indicates whether or not the backend honors
> the exclude patterns. Or, am I misunderstanding this?

I wouldn't say that this is abuse. We know the logic is only implemented by certain backends, and for the time being the only backend that does is the "files" backend. Furthermore, no test outside of t1419 ever cares for whether the backend knows to handle exclude patterns, so introducing a separate prereq that simply maps to REFFILES doesn't really feel worth it. If we ever implement this behaviour in the "reftable" backend, then we can easily extend the prereq like follows:

```
if ! test_have_prereq REFFILES && ! test_have_prereq REFTABLE
then
       skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
       test_done
fi
```

Now we could of course make the prereq clever and auto-detect whether the ref backend supports excludes. But this has the downside that it could lead to silent failures in case the exclude pattern handling ever breaks because now the prereq would potentially evaluate to "false".

Patrick