[PATCH v4 00/14] refs: improvements and fixes for peeling tags
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 23, 2025, 07:16 UTC
- Message-ID
- <20251023-b4-pks-ref-filter-skip-parsing-objects-v4-0-2be68ce82c9a@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.
Changes in v4: - Improve `get_or_parse_object()` to have better ergonomics. - Link to v3: https://lore.kernel.org/r/20251022-b4-pks-ref-filter-skip-parsing-objects-v3-0-eb9f71985ef0@pks.im
Changes in v3:
- I've rebuilt the topic on 133d151831 (The twenty-first batch, 2025-10-20) with
- tb/incremental-midx-part-3.1 at 935ab44a0a (builtin/repack.c:
clean up unused `#include`s, 2025-10-15)
- jt/repo-structure 16a93c03c7 at (builtin/repo: add progress meter for
structure stats, 2025-10-21)
merged into it. This is done to fix a couple of merge conflicts with
"seen". Both of the topics are only in "seen" right now, but they
are close to be merged.
- Link to v2: https://lore.kernel.org/r/20251008-b4-pks-ref-filter-skip-parsing-objects-v2-0-76e30d5c9542@pks.imChanges 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/replace.c | 21 ++--- builtin/repo.c | 9 +- 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 | 225 ++++++++++++++++++++++++++++++-------------- 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 +++--- repack-midx.c | 16 ++-- 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 ++- 66 files changed, 783 insertions(+), 834 deletions(-)
Range-diff versus v3:
1: 3c95bfae6a0 = 1: a8aafa4392b refs: introduce wrapper struct for `each_ref_fn`
2: f8a24267571 = 2: cf5faae21bd refs: introduce `.ref` field for the base iterator
3: e4e576977bf = 3: c0715cf44d8 refs: fully reset `struct ref_iterator::ref` on iteration
4: ea00af0721f = 4: 52f65b25e89 refs: refactor reference status flags
5: c3254472ddc = 5: 230cc4e7431 refs: expose peeled object ID via the iterator
6: 9df9622a93c = 6: bca033ba86d upload-pack: convert to use `reference_get_peeled_oid()`
7: b09a02a7f53 = 7: fa914f4cede ref-filter: propagate peeled object ID
8: 160788e6a7c = 8: 1d9ba79bcf7 builtin/show-ref: convert to use `reference_get_peeled_oid()`
9: f5793b2f1af = 9: 8451059caff refs: drop `current_ref_iter` hack
10: a467e8f55d1 = 10: de3e67c31d2 refs: drop infrastructure to peel via iterators
11: 49f45d1d680 = 11: 8782d6729df object: add flag to `peel_object()` to verify object type
12: 4e355684a9a = 12: ba7f85c8fef refs: don't store peeled object IDs for invalid tags
13: 1f709cd4b9c = 13: 74ad4413535 ref-filter: detect broken tags when dereferencing them
14: ef24fc33852 ! 14: e833e9943e2 ref-filter: parse objects on demand
@@ ref-filter.c: static void grab_common_values(struct atom_value *val, int deref,
}
}
-+static int get_or_parse_object(struct expand_data *data, const char *refname,
-+ struct object **object, struct strbuf *err, int *eaten)
++static struct object *get_or_parse_object(struct expand_data *data, const char *refname,
++ struct strbuf *err, int *eaten)
+{
+ if (!data->maybe_object) {
+ data->maybe_object = parse_object_buffer(the_repository, &data->oid, data->type,
+ data->size, data->content, eaten);
-+ if (!data->maybe_object)
-+ return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
-+ oid_to_hex(&data->oid), refname);
++ if (!data->maybe_object) {
++ strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
++ oid_to_hex(&data->oid), refname);
++ return NULL;
++ }
+ }
+
-+ *object = data->maybe_object;
-+ return 0;
++ return data->maybe_object;
+}
+
/* See grab_values */
@@ ref-filter.c: static void grab_common_values(struct atom_value *val, int deref,
+ struct expand_data *data, const char *refname,
+ struct strbuf *err, int *eaten)
{
-- int i;
-- struct tag *tag = (struct tag *) obj;
+ struct tag *tag = NULL;
-+ int i, ret;
+ int i;
+- struct tag *tag = (struct tag *) obj;
for (i = 0; i < used_atom_cnt; i++) {
const char *name = used_atom[i].name;
@@ ref-filter.c: static void grab_tag_values(struct atom_value *val, int deref, str
continue;
+
+ if (!tag) {
-+ struct object *object;
-+
-+ ret = get_or_parse_object(data, refname, &object, err, eaten);
-+ if (ret < 0)
-+ return ret;
-+
-+ tag = (struct tag *) object;
++ tag = (struct tag *) get_or_parse_object(data, refname,
++ err, eaten);
++ if (!tag)
++ return -1;
+ }
+
if (deref)
@@ ref-filter.c: static void grab_tag_values(struct atom_value *val, int deref, str
+ struct expand_data *data, const char *refname,
+ struct strbuf *err, int *eaten)
{
-- int i;
+ int i;
- struct commit *commit = (struct commit *) obj;
-+ int i, ret;
+ struct commit *commit = NULL;
for (i = 0; i < used_atom_cnt; i++) {
@@ ref-filter.c: static void grab_tag_values(struct atom_value *val, int deref, str
name++;
+
+ if (!commit) {
-+ struct object *object;
-+
-+ ret = get_or_parse_object(data, refname, &object, err, eaten);
-+ if (ret < 0)
-+ return ret;
-+
-+ commit = (struct commit *) object;
++ commit = (struct commit *) get_or_parse_object(data, refname,
++ err, eaten);
++ if (!commit)
++ return -1;
+ }
+
if (atom_type == ATOM_TREE &&
@@ ref-filter.c: static void grab_person(const char *who, struct atom_value *val, i
+ struct expand_data *data, const char *refname,
+ struct strbuf *err, int *eaten)
{
-- int i;
+ int i;
- struct commit *commit = (struct commit *) obj;
-+ int i, ret;
+ struct commit *commit = NULL;
struct signature_check sigc = { 0 };
int signature_checked = 0;
@@ ref-filter.c: static void grab_signature(struct atom_value *val, int deref, stru
if (!signature_checked) {
+ if (!commit) {
-+ struct object *object;
-+
-+ ret = get_or_parse_object(data, refname, &object, err, eaten);
-+ if (ret < 0)
-+ return ret;
-+
-+ commit = (struct commit *) object;
++ commit = (struct commit *) get_or_parse_object(data, refname,
++ err, eaten);
++ if (!commit)
++ return -1;
+ }
+
check_commit_signature(commit, &sigc);--- base-commit: 5c120f01eb88f4be8b06fd4bc6893763204b78c4 change-id: 20250918-b4-pks-ref-filter-skip-parsing-objects-f0d1f6af4a9f