[PATCH v2 00/14] refs: improvements and fixes for peeling tags
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 8, 2025, 15:50 UTC
- 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.imThanks!
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 demandbisect.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 <ps@pks.im>
## 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