On Tue, Jan 09, 2024 at 02:40:50PM -0500, Eric Sunshine wrote:
> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@xxxxxx> 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? :)
> > 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
Attachment:
signature.asc
Description: PGP signature
