From: Patrick Steinhardt Date: Tue, 07 Oct 2025 10:58:37 GMT Subject: [PATCH 00/13] refs: improvements and fixes for peeling tags Message-ID: <20251007-b4-pks-ref-filter-skip-parsing-objects-v1-0-916cc7c6886b@pks.im> Hi, originally, all I wanted to do was the last patch: a small performance optimization that stops parsing objects in git-for-each-ref(1) unless we really need to parse them. But that fix cause one specific test to fail, and only with the reftable backend. So this led me down the rabbit hole of tag peeling, ending up with this patch series. The series is structured like follows: - Patches 1 to 7 refactor our codebase so that we don't have the `peel_iterated_object()` hack anymore. I just found it hard to follow and thought it shouldn't be too hard to get rid of it. - Patches 8 and 9 remove infrastructure that we don't need anymore after the first couple of patches. - Patches 10 to 12 fix a couple of issues with peeled tags that I found. The underlying issue is that tags store both the tagged object and their type, but this information may not match. We never verify the actual object type though when allocating the tagged object, so this only blows up much later. - Patch 13 was my original motivation, a small performance optimization. I'm not particularly fond of the patches 10 to 12. It feels more like playing whack-a-mole, and I very much assume that there still are edge cases where we should properly verify the tagged object type. But changing it in `parse_tag_buffer()` itself causes a bunch of tests to fail where we intentionally create such corrupted tags. So I didn't really dare to touch that part, to be honest. If anybody has suggestions for an alternative approach I'd be very open to it. The topic is built on top of 45547b60ac (Merge branch 'master' of https://github.com/j6t/gitk, 2025-10-05). There is a merge conflict with tb/incremental-midx-part-3.1, which moves code from "builtin/repack.c" into "repack-*.c". The conflict can be solved by accepting "builtin/repack.c" from tb/incremental-midx-part-3.1 and adding the below patch to "repack-midx.c". I can also rebase on top of that series, but given that it is rather huge it may take a while before it lands. Thanks! Patrick diff --cc builtin/repack.c index 873e21c35d,ad60c4290d..0000000000 --- a/builtin/repack.c +++ b/builtin/repack.c diff --git a/repack-midx.c b/repack-midx.c index 6f6202c5bc..74bdfa3a6e 100644 --- a/repack-midx.c +++ b/repack-midx.c @@ -16,25 +16,23 @@ struct midx_snapshot_ref_data { int preferred; }; -static int midx_snapshot_ref_one(const char *refname UNUSED, - const char *referent UNUSED, - const struct object_id *oid, - int flag UNUSED, void *_data) +static int midx_snapshot_ref_one(const struct reference *ref, void *_data) { struct midx_snapshot_ref_data *data = _data; + const struct object_id *maybe_peeled = ref->oid; struct object_id peeled; - if (!peel_iterated_oid(data->repo, oid, &peeled)) - oid = &peeled; + if (!reference_get_peeled_oid(data->repo, ref, &peeled)) + maybe_peeled = &peeled; - if (oidset_insert(&data->seen, oid)) + if (oidset_insert(&data->seen, maybe_peeled)) return 0; /* already seen */ - if (odb_read_object_info(data->repo->objects, oid, NULL) != OBJ_COMMIT) + if (odb_read_object_info(data->repo->objects, maybe_peeled, NULL) != OBJ_COMMIT) return 0; fprintf(data->f->fp, "%s%s\n", data->preferred ? "+" : "", - oid_to_hex(oid)); + oid_to_hex(maybe_peeled)); return 0; } --- Patrick Steinhardt (13): refs: introduce wrapper struct for `each_ref_fn` refs: introduce `.ref` field for the base iterator refs: refactor reference status flags refs: expose peeled object ID via the iterator upload-pack: convert to use `reference_get_peeled_oid()` ref-filter: propagate peeled object ID builtin/show-ref: convert to use `reference_get_peeled_oid()` refs: drop `current_ref_iter` hack refs: drop infrastructure to peel via iterators object: add flag to `peel_object()` to verify object type refs: don't store peeled object IDs for invalid tags ref-filter: detect broken tags when dereferencing them ref-filter: parse objects on demand bisect.c | 24 ++--- builtin/bisect.c | 17 +--- builtin/checkout.c | 6 +- builtin/describe.c | 18 ++-- builtin/fetch.c | 13 +-- builtin/fsck.c | 33 +++--- builtin/gc.c | 15 ++- builtin/ls-remote.c | 2 +- builtin/name-rev.c | 17 ++-- builtin/pack-objects.c | 28 +++--- builtin/receive-pack.c | 13 ++- builtin/remote.c | 44 ++++---- builtin/repack.c | 16 ++- builtin/replace.c | 21 ++-- builtin/rev-parse.c | 12 +-- builtin/show-branch.c | 35 +++---- builtin/show-ref.c | 50 ++++----- builtin/submodule--helper.c | 10 +- builtin/tag.c | 2 +- builtin/verify-tag.c | 2 +- builtin/worktree.c | 6 +- commit-graph.c | 14 ++- delta-islands.c | 9 +- fetch-pack.c | 16 +-- help.c | 10 +- http-backend.c | 20 ++-- log-tree.c | 24 ++--- ls-refs.c | 36 ++++--- midx-write.c | 17 ++-- negotiator/default.c | 7 +- negotiator/skipping.c | 7 +- notes.c | 8 +- object-name.c | 10 +- object.c | 20 +++- object.h | 15 ++- pseudo-merge.c | 21 ++-- reachable.c | 9 +- ref-filter.c | 239 ++++++++++++++++++++++++++++++-------------- ref-filter.h | 5 +- reflog.c | 9 +- refs.c | 85 +++++++++------- refs.h | 84 ++++++++++------ refs/debug.c | 17 +--- refs/files-backend.c | 71 +++++-------- refs/iterator.c | 73 +++----------- refs/packed-backend.c | 72 +++++-------- refs/ref-cache.c | 18 +--- refs/refs-internal.h | 25 +---- refs/reftable-backend.c | 48 +++------ remote.c | 27 +++-- replace-object.c | 16 ++- revision.c | 12 +-- server-info.c | 12 +-- shallow.c | 16 +-- submodule.c | 12 +-- t/for-each-ref-tests.sh | 4 +- t/helper/test-reach.c | 2 +- t/helper/test-ref-store.c | 5 +- t/pack-refs-tests.sh | 32 ++++++ t/t0610-reftable-basics.sh | 28 ++++++ tag.c | 12 --- tag.h | 1 - upload-pack.c | 49 ++++----- walker.c | 8 +- worktree.c | 11 +- 65 files changed, 791 insertions(+), 829 deletions(-) --- base-commit: 45547b60aca32b45d2f1ef93462cf9df28637c13 change-id: 20250918-b4-pks-ref-filter-skip-parsing-objects-f0d1f6af4a9f