From: Patrick Steinhardt Date: Wed, 08 Oct 2025 15:50:15 GMT Subject: [PATCH v2 00/14] refs: improvements and fixes for peeling tags Message-ID: <20251008-b4-pks-ref-filter-skip-parsing-objects-v2-0-76e30d5c9542@pks.im> In-Reply-To: <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 8 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 9 and 10 remove infrastructure that we don't need anymore after the first couple of patches. - Patches 11 to 13 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 14 was my original motivation, a small performance optimization. I'm not particularly fond of the patches 11 to 13. 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". Changes in v2: - A couple of improvements to commit messages. - A new commit that ensures that `struct ref_iterator::ref` is always zeroed out to protect against stale state. - Link to v1: https://lore.kernel.org/r/20251007-b4-pks-ref-filter-skip-parsing-objects-v1-0-916cc7c6886b@pks.im Thanks! Patrick --- Patrick Steinhardt (14): refs: introduce wrapper struct for `each_ref_fn` refs: introduce `.ref` field for the base iterator refs: fully reset `struct ref_iterator::ref` on iteration 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 | 88 ++++++++++------ refs/debug.c | 17 +--- refs/files-backend.c | 71 +++++-------- refs/iterator.c | 73 +++----------- refs/packed-backend.c | 71 +++++-------- refs/ref-cache.c | 18 +--- refs/refs-internal.h | 25 +---- refs/reftable-backend.c | 47 +++------ 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(+), 831 deletions(-) Range-diff versus v1: 1: 4f4de68657 ! 1: 3d0c5110b3 refs: introduce wrapper struct for `each_ref_fn` @@ Commit message more opaque. While it is obvious which callsites need to be fixed up when we change the function type, it's not obvious anymore once we use a structure. That being said, we only have a handful of sites that - actually need to populate this wrapper structure: our ref backends and - "refs/iterator.c". + actually need to populate this wrapper structure: our ref backends, + "refs/iterator.c" as well as very few sites that invoke the iterator + callback functions directly. Introduce this wrapper structure so that we can adapt the iterator interfaces more readily. @@ refs.h: struct ref_transaction; + /* * The signature for the callback function for the for_each_*() - * functions below. The memory pointed to by the refname and oid - * arguments is only guaranteed to be valid for the duration of a +- * functions below. The memory pointed to by the refname and oid +- * arguments is only guaranteed to be valid for the duration of a ++ * functions below. The memory pointed to by the `struct reference` ++ * argument is only guaranteed to be valid for the duration of a * single callback invocation. */ -typedef int each_ref_fn(const char *refname, const char *referent, 2: e90c26f12c ! 2: 412818b8d1 refs: introduce `.ref` field for the base iterator @@ Commit message refs: introduce `.ref` field for the base iterator The base iterator has a couple of fields that tracks the name, target, - object ID and flags for the current reference. Due do this design we + object ID and flags for the current reference. Due to this design we have to create a new `struct reference` whenever we want to hand over that reference to the callback function, which is tedious and not very efficient. - Convert the structure to instead contain a `stuct reference` as member. + Convert the structure to instead contain a `struct reference` as member. This member is expected to be populated by the implementations of the iterator and is handed over to the callback directly. + While at it, simplify `should_pack_ref()` to take a `struct reference` + directly instead of passing its respective fields. + Signed-off-by: Patrick Steinhardt ## refs.c ## -: ---------- > 3: 231adc4960 refs: fully reset `struct ref_iterator::ref` on iteration 3: bc0432dc35 = 4: 1d2189e7a9 refs: refactor reference status flags 4: aba078d59c ! 5: 1f975f1079 refs: expose peeled object ID via the iterator @@ refs.h: struct reference { + /* * The signature for the callback function for the for_each_*() - * functions below. The memory pointed to by the refname and oid + * functions below. The memory pointed to by the `struct reference` ## refs/packed-backend.c ## @@ refs/packed-backend.c: static int next_record(struct packed_ref_iterator *iter) - if ((iter->base.ref.flags & REF_ISBROKEN)) { - oidclr(&iter->peeled, iter->repo->hash_algo); iter->base.ref.flags &= ~REF_KNOWS_PEELED; -+ iter->base.ref.peeled_oid = NULL; } else { iter->base.ref.flags |= REF_KNOWS_PEELED; + iter->base.ref.peeled_oid = &iter->peeled; } } else { oidclr(&iter->peeled, iter->repo->hash_algo); -+ iter->base.ref.peeled_oid = NULL; - } - - return ITER_OK; - - ## refs/ref-cache.c ## -@@ refs/ref-cache.c: static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) - iter->base.ref.name = entry->name; - iter->base.ref.target = entry->u.value.referent; - iter->base.ref.oid = &entry->u.value.oid; -+ iter->base.ref.peeled_oid = NULL; - iter->base.ref.flags = entry->flag; - return ITER_OK; - } ## refs/reftable-backend.c ## @@ refs/reftable-backend.c: struct reftable_ref_iterator { @@ refs/reftable-backend.c: static int reftable_ref_iterator_advance(struct ref_ite iter->base.ref.oid = &iter->oid; + if (iter->ref.value_type == REFTABLE_REF_VAL2) + iter->base.ref.peeled_oid = &iter->peeled_oid; -+ else -+ iter->base.ref.peeled_oid = NULL; iter->base.ref.flags = flags; break; 5: 664775be02 ! 6: 8f3b757726 upload-pack: convert to use `reference_get_peeled_oid()` @@ Commit message The `write_v0_ref()` callback is invoked from two callsites: - Once via `send_ref()` which is a callback passed to - `for_each_namespaced_ref_1()`. + `for_each_namespaced_ref_1()` and `refs_head_ref_namespaced()`. - Once manually to announce capabilities. 6: e0c95a3df2 = 7: 603213919f ref-filter: propagate peeled object ID 7: 9ef21d25d1 = 8: 8d3cae65f8 builtin/show-ref: convert to use `reference_get_peeled_oid()` 8: 2d32a2410d = 9: 35226e7a7d refs: drop `current_ref_iter` hack 9: a9fbbe6b1a = 10: 2e73f3b227 refs: drop infrastructure to peel via iterators 10: 68b3a85ade ! 11: ade4d5875a object: add flag to `peel_object()` to verify object type @@ Commit message tagged object. The relevant code path here eventually ends up in `parse_tag_buffer()`. - Here, we parset he various fields of the tag, including the "type". Once + Here, we parse the various fields of the tag, including the "type". Once we've figured out the type and the tagged object ID, we call one of the `lookup_${type}()` functions for whatever type we have found. There is two possible outcomes in the successful case: 11: 80ec80883e = 12: c1ccb8e6e1 refs: don't store peeled object IDs for invalid tags 12: 8056c68337 = 13: e5db08d3fa ref-filter: detect broken tags when dereferencing them 13: 40cc049093 ! 14: 617d03d8c1 ref-filter: parse objects on demand @@ Metadata ## Commit message ## ref-filter: parse objects on demand - When formatting an arbitray object we parse that object regardless of + When formatting an arbitrary object we parse that object regardless of whether or not we actually need any parsed data. In fact, many of the atoms we have don't require any. --- base-commit: 45547b60aca32b45d2f1ef93462cf9df28637c13 change-id: 20250918-b4-pks-ref-filter-skip-parsing-objects-f0d1f6af4a9f