| Message ID | d7c6b8b2a7b3b4d776f06ce577bdbdbaff66f225.1704802213.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: mark "files"-backend specific tests | expand |
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/ > 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?
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@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? :) > > 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
Patrick Steinhardt <ps@pks.im> writes: On just this (non-technical) part... >> > 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? :) As a non-native, I often find this https://trends.google.com/trends/explore?q=consequentially,consequently&hl=en fairly useful.
On Wed, Jan 10, 2024 at 08:27:29AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > On just this (non-technical) part... > > >> > 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? :) > > As a non-native, I often find this > > https://trends.google.com/trends/explore?q=consequentially,consequently&hl=en > > fairly useful. Good idea, thanks! Patrick
diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh index 5d8c86b657..1359574419 100755 --- a/t/t1419-exclude-refs.sh +++ b/t/t1419-exclude-refs.sh @@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh +if test_have_prereq !REFFILES +then + skip_all='skipping `git for-each-ref --exclude` tests; need files backend' + test_done +fi + for_each_ref__exclude () { GIT_TRACE2_PERF=1 test-tool ref-store main \ for-each-ref--exclude "$@" >actual.raw
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. 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. 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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t1419-exclude-refs.sh | 6 ++++++ 1 file changed, 6 insertions(+)