From 89baa52da612dde6da031acfa2cb957d4297d544 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 23 Oct 2025 09:16:11 +0200 Subject: 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 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 `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 Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index a8c22a0a7f..7987acdc96 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -908,7 +908,7 @@ static int next_record(struct packed_ref_iterator *iter) if (iter->pos == iter->eof) return ITER_DONE; - iter->base.flags = REF_ISPACKED; + iter->base.ref.flags = REF_ISPACKED; p = iter->pos; if (iter->eof - p < snapshot_hexsz(iter->snapshot) + 2 || @@ -923,22 +923,22 @@ static int next_record(struct packed_ref_iterator *iter) iter->pos, iter->eof - iter->pos); strbuf_add(&iter->refname_buf, p, eol - p); - iter->base.refname = iter->refname_buf.buf; + iter->base.ref.name = iter->refname_buf.buf; if (refname_contains_nul(&iter->refname_buf)) - die("packed refname contains embedded NULL: %s", iter->base.refname); + die("packed refname contains embedded NULL: %s", iter->base.ref.name); - if (check_refname_format(iter->base.refname, REFNAME_ALLOW_ONELEVEL)) { - if (!refname_is_safe(iter->base.refname)) + if (check_refname_format(iter->base.ref.name, REFNAME_ALLOW_ONELEVEL)) { + if (!refname_is_safe(iter->base.ref.name)) die("packed refname is dangerous: %s", - iter->base.refname); + iter->base.ref.name); oidclr(&iter->oid, iter->repo->hash_algo); - iter->base.flags |= REF_BAD_NAME | REF_ISBROKEN; + iter->base.ref.flags |= REF_BAD_NAME | REF_ISBROKEN; } if (iter->snapshot->peeled == PEELED_FULLY || (iter->snapshot->peeled == PEELED_TAGS && - starts_with(iter->base.refname, "refs/tags/"))) - iter->base.flags |= REF_KNOWS_PEELED; + starts_with(iter->base.ref.name, "refs/tags/"))) + iter->base.ref.flags |= REF_KNOWS_PEELED; iter->pos = eol + 1; @@ -956,11 +956,11 @@ static int next_record(struct packed_ref_iterator *iter) * definitely know the value of *this* reference. But * we suppress it if the reference is broken: */ - if ((iter->base.flags & REF_ISBROKEN)) { + if ((iter->base.ref.flags & REF_ISBROKEN)) { oidclr(&iter->peeled, iter->repo->hash_algo); - iter->base.flags &= ~REF_KNOWS_PEELED; + iter->base.ref.flags &= ~REF_KNOWS_PEELED; } else { - iter->base.flags |= REF_KNOWS_PEELED; + iter->base.ref.flags |= REF_KNOWS_PEELED; } } else { oidclr(&iter->peeled, iter->repo->hash_algo); @@ -976,15 +976,15 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator) int ok; while ((ok = next_record(iter)) == ITER_OK) { - const char *refname = iter->base.refname; + const char *refname = iter->base.ref.name; const char *prefix = iter->prefix; if (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY && - !is_per_worktree_ref(iter->base.refname)) + !is_per_worktree_ref(iter->base.ref.name)) continue; if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && - !ref_resolves_to_object(iter->base.refname, iter->repo, + !ref_resolves_to_object(iter->base.ref.name, iter->repo, &iter->oid, iter->flags)) continue; @@ -1033,10 +1033,10 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator, struct packed_ref_iterator *iter = (struct packed_ref_iterator *)ref_iterator; - if ((iter->base.flags & REF_KNOWS_PEELED)) { + if ((iter->base.ref.flags & REF_KNOWS_PEELED)) { oidcpy(peeled, &iter->peeled); return is_null_oid(&iter->peeled) ? -1 : 0; - } else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) { + } else if ((iter->base.ref.flags & (REF_ISBROKEN | REF_ISSYMREF))) { return -1; } else { return peel_object(iter->repo, &iter->oid, peeled) ? -1 : 0; @@ -1194,7 +1194,7 @@ static struct ref_iterator *packed_ref_iterator_begin( iter->snapshot = snapshot; acquire_snapshot(snapshot); strbuf_init(&iter->refname_buf, 0); - iter->base.oid = &iter->oid; + iter->base.ref.oid = &iter->oid; iter->repo = ref_store->repo; iter->flags = flags; @@ -1436,7 +1436,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re if (!iter) cmp = +1; else - cmp = strcmp(iter->refname, update->refname); + cmp = strcmp(iter->ref.name, update->refname); } if (!cmp) { @@ -1459,11 +1459,11 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re } goto error; - } else if (!oideq(&update->old_oid, iter->oid)) { + } else if (!oideq(&update->old_oid, iter->ref.oid)) { strbuf_addf(err, "cannot update ref '%s': " "is at %s but expected %s", update->refname, - oid_to_hex(iter->oid), + oid_to_hex(iter->ref.oid), oid_to_hex(&update->old_oid)); ret = REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; @@ -1527,8 +1527,8 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re struct object_id peeled; int peel_error = ref_iterator_peel(iter, &peeled); - if (write_packed_entry(out, iter->refname, - iter->oid, + if (write_packed_entry(out, iter->ref.name, + iter->ref.oid, peel_error ? NULL : &peeled)) goto write_error; -- cgit v1.2.3 From 4cea0422879f6a64c0f7ad0ddac6d43897a53e94 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 23 Oct 2025 09:16:12 +0200 Subject: refs: fully reset `struct ref_iterator::ref` on iteration With the introduction of the `struct ref_iterator::ref` field it now is a whole lot easier to introduce new fields that become accessible to the caller without having to adapt every single callsite. But there's a downside: when a new field is introduced we always have to adapt all backends to set that field. This isn't something we can avoid in the general case: when the new field is expected to be populated by all backends we of course cannot avoid doing so. But new fields may be entirely optional, in which case we'd still have such churn. And furthermore, it is very easy right now to leak state from a previous iteration into the next iteration. Address this issue by ensuring that the reference backends all fully reset the field on every single iteration. This ensures that no state from previous iterations can leak into the next one. And it ensures that any newly introduced fields will be zeroed out by default. Note that we don't have to explicitly adapt the "files" backend, as it uses the `cache_ref_iterator` internally. Furthermore, other "wrapping" iterators like for example the `prefix_ref_iterator` copy around the whole reference, so these don't need to be adapted either. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 3 ++- refs/ref-cache.c | 1 + refs/reftable-backend.c | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) (limited to 'refs/packed-backend.c') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 7987acdc96..711e07f832 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -882,6 +882,7 @@ static int next_record(struct packed_ref_iterator *iter) { const char *p, *eol; + memset(&iter->base.ref, 0, sizeof(iter->base.ref)); strbuf_reset(&iter->refname_buf); /* @@ -916,6 +917,7 @@ static int next_record(struct packed_ref_iterator *iter) !isspace(*p++)) die_invalid_line(iter->snapshot->refs->path, iter->pos, iter->eof - iter->pos); + iter->base.ref.oid = &iter->oid; eol = memchr(p, '\n', iter->eof - p); if (!eol) @@ -1194,7 +1196,6 @@ static struct ref_iterator *packed_ref_iterator_begin( iter->snapshot = snapshot; acquire_snapshot(snapshot); strbuf_init(&iter->refname_buf, 0); - iter->base.ref.oid = &iter->oid; iter->repo = ref_store->repo; iter->flags = flags; diff --git a/refs/ref-cache.c b/refs/ref-cache.c index f1abc39624..e427848879 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -425,6 +425,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) level->prefix_state = entry_prefix_state; level->index = -1; } else { + memset(&iter->base.ref, 0, sizeof(iter->base.ref)); iter->base.ref.name = entry->name; iter->base.ref.target = entry->u.value.referent; iter->base.ref.oid = &entry->u.value.oid; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 0e47986cb5..728886eafd 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -704,6 +704,7 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator) &iter->oid, flags)) continue; + memset(&iter->base.ref, 0, sizeof(iter->base.ref)); iter->base.ref.name = iter->ref.refname; iter->base.ref.target = referent; iter->base.ref.oid = &iter->oid; -- cgit v1.2.3 From f89866163704528f1a6570e134853dbb99120e7c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 23 Oct 2025 09:16:14 +0200 Subject: refs: expose peeled object ID via the iterator Both the "files" and "reftable" backend are able to store peeled values for tags in the respective formats. This allows for a more efficient lookup of the target object of such a tag without having to manually peel via the object database. The infrastructure to access these peeled object IDs is somewhat funky though. When iterating through objects, we store a pointer reference to the current iterator in a global variable. The callbacks invoked by that iterator are then expected to call `peel_iterated_oid()`, which checks whether the globally-stored iterator's current reference refers to the one handed into that function. If so, we ask the iterator to peel the object, otherwise we manually peel the object via the object database. Depending on global state like this is somewhat weird and also quite fragile. Introduce a new `struct reference::peeled_oid` field that can be populated by the reference backends. This field can be accessed via a new function `reference_get_peeled_oid()` that either uses that value, if set, or alternatively peels via the ODB. With this change we don't have to rely on global state anymore, but make the peeled object ID available to the callback functions directly. Adjust trivial callers that already have a `struct reference` available. Remaining callers will be adjusted in subsequent commits. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/describe.c | 2 +- builtin/gc.c | 2 +- builtin/pack-objects.c | 7 ++++--- commit-graph.c | 2 +- ls-refs.c | 2 +- midx-write.c | 2 +- pseudo-merge.c | 2 +- refs.c | 12 ++++++++++++ refs.h | 19 +++++++++++++++++++ refs/packed-backend.c | 1 + refs/reftable-backend.c | 5 +++++ repack-midx.c | 2 +- 12 files changed, 48 insertions(+), 10 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/builtin/describe.c b/builtin/describe.c index 7954535044..443546aaac 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -208,7 +208,7 @@ static int get_name(const struct reference *ref, void *cb_data UNUSED) } /* Is it annotated? */ - if (!peel_iterated_oid(the_repository, ref->oid, &peeled)) { + if (!reference_get_peeled_oid(the_repository, ref, &peeled)) { is_annotated = !oideq(ref->oid, &peeled); } else { oidcpy(&peeled, ref->oid); diff --git a/builtin/gc.c b/builtin/gc.c index 9de5de175f..f0cf20d423 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1109,7 +1109,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data) struct commit_list *stack = NULL; struct commit *commit; - if (!peel_iterated_oid(the_repository, ref->oid, &peeled)) + if (!reference_get_peeled_oid(the_repository, ref, &peeled)) maybe_peeled = &peeled; if (odb_read_object_info(the_repository->objects, maybe_peeled, NULL) != OBJ_COMMIT) return 0; diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 39633a0158..1613fecb66 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -838,7 +838,7 @@ static int mark_tagged(const struct reference *ref, void *cb_data UNUSED) if (entry) entry->tagged = 1; - if (!peel_iterated_oid(the_repository, ref->oid, &peeled)) { + if (!reference_get_peeled_oid(the_repository, ref, &peeled)) { entry = packlist_find(&to_pack, &peeled); if (entry) entry->tagged = 1; @@ -3309,7 +3309,8 @@ static int add_ref_tag(const struct reference *ref, void *cb_data UNUSED) { struct object_id peeled; - if (!peel_iterated_oid(the_repository, ref->oid, &peeled) && obj_is_packed(&peeled)) + if (!reference_get_peeled_oid(the_repository, ref, &peeled) && + obj_is_packed(&peeled)) add_tag_chain(ref->oid); return 0; } @@ -4537,7 +4538,7 @@ static int mark_bitmap_preferred_tip(const struct reference *ref, void *data UNU struct object_id peeled; struct object *object; - if (!peel_iterated_oid(the_repository, ref->oid, &peeled)) + if (!reference_get_peeled_oid(the_repository, ref, &peeled)) maybe_peeled = &peeled; object = parse_object_or_die(the_repository, maybe_peeled, ref->name); diff --git a/commit-graph.c b/commit-graph.c index f91af41625..80be2ff2c3 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1857,7 +1857,7 @@ static int add_ref_to_set(const struct reference *ref, void *cb_data) struct object_id peeled; struct refs_cb_data *data = (struct refs_cb_data *)cb_data; - if (!peel_iterated_oid(data->repo, ref->oid, &peeled)) + if (!reference_get_peeled_oid(data->repo, ref, &peeled)) maybe_peeled = &peeled; if (odb_read_object_info(data->repo->objects, maybe_peeled, NULL) == OBJ_COMMIT) oidset_insert(data->commits, maybe_peeled); diff --git a/ls-refs.c b/ls-refs.c index 64d0272369..8641281b86 100644 --- a/ls-refs.c +++ b/ls-refs.c @@ -110,7 +110,7 @@ static int send_ref(const struct reference *ref, void *cb_data) if (data->peel && ref->oid) { struct object_id peeled; - if (!peel_iterated_oid(the_repository, ref->oid, &peeled)) + if (!reference_get_peeled_oid(the_repository, ref, &peeled)) strbuf_addf(&data->buf, " peeled:%s", oid_to_hex(&peeled)); } diff --git a/midx-write.c b/midx-write.c index f4dd875747..23e61cb000 100644 --- a/midx-write.c +++ b/midx-write.c @@ -709,7 +709,7 @@ static int add_ref_to_pending(const struct reference *ref, void *cb_data) return 0; } - if (!peel_iterated_oid(revs->repo, ref->oid, &peeled)) + if (!reference_get_peeled_oid(revs->repo, ref, &peeled)) maybe_peeled = &peeled; object = parse_object_or_die(revs->repo, maybe_peeled, ref->name); diff --git a/pseudo-merge.c b/pseudo-merge.c index 0abd51b42c..a2d5bd85f9 100644 --- a/pseudo-merge.c +++ b/pseudo-merge.c @@ -230,7 +230,7 @@ static int find_pseudo_merge_group_for_ref(const struct reference *ref, void *_d uint32_t i; int has_bitmap; - if (!peel_iterated_oid(the_repository, ref->oid, &peeled)) + if (!reference_get_peeled_oid(the_repository, ref, &peeled)) maybe_peeled = &peeled; c = lookup_commit(the_repository, maybe_peeled); diff --git a/refs.c b/refs.c index f96cf43b12..1b1551f981 100644 --- a/refs.c +++ b/refs.c @@ -2334,6 +2334,18 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct return peel_object(r, base, peeled) ? -1 : 0; } +int reference_get_peeled_oid(struct repository *repo, + const struct reference *ref, + struct object_id *peeled_oid) +{ + if (ref->peeled_oid) { + oidcpy(peeled_oid, ref->peeled_oid); + return 0; + } + + return peel_object(repo, ref->oid, peeled_oid) ? -1 : 0; +} + int refs_update_symref(struct ref_store *refs, const char *ref, const char *target, const char *logmsg) { diff --git a/refs.h b/refs.h index 4f0a685714..886ed2c0f4 100644 --- a/refs.h +++ b/refs.h @@ -371,10 +371,29 @@ struct reference { */ const struct object_id *oid; + /* + * An optional peeled object ID. This field _may_ be set for tags in + * case the peeled value is present in the backend. Please refer to + * `reference_get_peeled_oid()`. + */ + const struct object_id *peeled_oid; + /* A bitfield of `enum reference_status` flags. */ unsigned flags; }; +/* + * Peel the tag to a non-tag commit. If present, this uses the peeled object ID + * exposed by the reference backend. Otherwise, the object is peeled via the + * object database, which is less efficient. + * + * Return `0` if the reference could be peeled, a negative error code + * otherwise. + */ +int reference_get_peeled_oid(struct repository *repo, + const struct reference *ref, + struct object_id *peeled_oid); + /* * The signature for the callback function for the for_each_*() * functions below. The memory pointed to by the `struct reference` diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 711e07f832..1fefefd54e 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -963,6 +963,7 @@ static int next_record(struct packed_ref_iterator *iter) iter->base.ref.flags &= ~REF_KNOWS_PEELED; } else { iter->base.ref.flags |= REF_KNOWS_PEELED; + iter->base.ref.peeled_oid = &iter->peeled; } } else { oidclr(&iter->peeled, iter->repo->hash_algo); diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 728886eafd..e214e120d7 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -547,6 +547,7 @@ struct reftable_ref_iterator { struct reftable_iterator iter; struct reftable_ref_record ref; struct object_id oid; + struct object_id peeled_oid; char *prefix; size_t prefix_len; @@ -671,6 +672,8 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator) case REFTABLE_REF_VAL2: oidread(&iter->oid, iter->ref.value.val2.value, refs->base.repo->hash_algo); + oidread(&iter->peeled_oid, iter->ref.value.val2.target_value, + refs->base.repo->hash_algo); break; case REFTABLE_REF_SYMREF: referent = refs_resolve_ref_unsafe(&iter->refs->base, @@ -708,6 +711,8 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator) iter->base.ref.name = iter->ref.refname; iter->base.ref.target = referent; iter->base.ref.oid = &iter->oid; + if (iter->ref.value_type == REFTABLE_REF_VAL2) + iter->base.ref.peeled_oid = &iter->peeled_oid; iter->base.ref.flags = flags; break; diff --git a/repack-midx.c b/repack-midx.c index 349f7e20b5..74bdfa3a6e 100644 --- a/repack-midx.c +++ b/repack-midx.c @@ -22,7 +22,7 @@ static int midx_snapshot_ref_one(const struct reference *ref, void *_data) const struct object_id *maybe_peeled = ref->oid; struct object_id peeled; - if (!peel_iterated_oid(data->repo, ref->oid, &peeled)) + if (!reference_get_peeled_oid(data->repo, ref, &peeled)) maybe_peeled = &peeled; if (oidset_insert(&data->seen, maybe_peeled)) -- cgit v1.2.3 From 705114772e0a0741c3288329bd9ac4e11e38db9a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 23 Oct 2025 09:16:19 +0200 Subject: refs: drop infrastructure to peel via iterators Now that the peeled object ID gets propagated via the `struct reference` there is no need anymore to call into the reference iterator itself to dereference an object. Remove this infrastructure. Most of the changes are straight-forward deletions of code. There is one exception though in `refs/packed-backend.c::write_with_updates()`. Here we stop peeling the iterator and instead just pass the peeled object ID of that iterator directly. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.h | 14 -------------- refs/debug.c | 11 ----------- refs/files-backend.c | 17 ----------------- refs/iterator.c | 36 ------------------------------------ refs/packed-backend.c | 24 +----------------------- refs/ref-cache.c | 9 --------- refs/refs-internal.h | 7 ------- refs/reftable-backend.c | 24 ------------------------ 8 files changed, 1 insertion(+), 141 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/refs.h b/refs.h index 886ed2c0f4..2dd7ac1a16 100644 --- a/refs.h +++ b/refs.h @@ -1289,10 +1289,6 @@ int repo_migrate_ref_storage_format(struct repository *repo, * to the next entry, ref_iterator_advance() aborts the iteration, * frees the ref_iterator, and returns ITER_ERROR. * - * The reference currently being looked at can be peeled by calling - * ref_iterator_peel(). This function is often faster than peel_ref(), - * so it should be preferred when iterating over references. - * * Putting it all together, a typical iteration looks like this: * * int ok; @@ -1307,9 +1303,6 @@ int repo_migrate_ref_storage_format(struct repository *repo, * // Access information about the current reference: * if (!(iter->flags & REF_ISSYMREF)) * printf("%s is %s\n", iter->refname, oid_to_hex(iter->oid)); - * - * // If you need to peel the reference: - * ref_iterator_peel(iter, &oid); * } * * if (ok != ITER_DONE) @@ -1400,13 +1393,6 @@ enum ref_iterator_seek_flag { int ref_iterator_seek(struct ref_iterator *ref_iterator, const char *refname, unsigned int flags); -/* - * If possible, peel the reference currently being viewed by the - * iterator. Return 0 on success. - */ -int ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled); - /* Free the reference iterator and any associated resources. */ void ref_iterator_free(struct ref_iterator *ref_iterator); diff --git a/refs/debug.c b/refs/debug.c index 67718bd1f4..01499b9033 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -177,16 +177,6 @@ static int debug_ref_iterator_seek(struct ref_iterator *ref_iterator, return res; } -static int debug_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) -{ - struct debug_ref_iterator *diter = - (struct debug_ref_iterator *)ref_iterator; - int res = diter->iter->vtable->peel(diter->iter, peeled); - trace_printf_key(&trace_refs, "iterator_peel: %s: %d\n", diter->iter->ref.name, res); - return res; -} - static void debug_ref_iterator_release(struct ref_iterator *ref_iterator) { struct debug_ref_iterator *diter = @@ -198,7 +188,6 @@ static void debug_ref_iterator_release(struct ref_iterator *ref_iterator) static struct ref_iterator_vtable debug_ref_iterator_vtable = { .advance = debug_ref_iterator_advance, .seek = debug_ref_iterator_seek, - .peel = debug_ref_iterator_peel, .release = debug_ref_iterator_release, }; diff --git a/refs/files-backend.c b/refs/files-backend.c index fac53fa052..5aeb454fb4 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -993,15 +993,6 @@ static int files_ref_iterator_seek(struct ref_iterator *ref_iterator, return ref_iterator_seek(iter->iter0, refname, flags); } -static int files_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) -{ - struct files_ref_iterator *iter = - (struct files_ref_iterator *)ref_iterator; - - return ref_iterator_peel(iter->iter0, peeled); -} - static void files_ref_iterator_release(struct ref_iterator *ref_iterator) { struct files_ref_iterator *iter = @@ -1012,7 +1003,6 @@ static void files_ref_iterator_release(struct ref_iterator *ref_iterator) static struct ref_iterator_vtable files_ref_iterator_vtable = { .advance = files_ref_iterator_advance, .seek = files_ref_iterator_seek, - .peel = files_ref_iterator_peel, .release = files_ref_iterator_release, }; @@ -2388,12 +2378,6 @@ static int files_reflog_iterator_seek(struct ref_iterator *ref_iterator UNUSED, BUG("ref_iterator_seek() called for reflog_iterator"); } -static int files_reflog_iterator_peel(struct ref_iterator *ref_iterator UNUSED, - struct object_id *peeled UNUSED) -{ - BUG("ref_iterator_peel() called for reflog_iterator"); -} - static void files_reflog_iterator_release(struct ref_iterator *ref_iterator) { struct files_reflog_iterator *iter = @@ -2404,7 +2388,6 @@ static void files_reflog_iterator_release(struct ref_iterator *ref_iterator) static struct ref_iterator_vtable files_reflog_iterator_vtable = { .advance = files_reflog_iterator_advance, .seek = files_reflog_iterator_seek, - .peel = files_reflog_iterator_peel, .release = files_reflog_iterator_release, }; diff --git a/refs/iterator.c b/refs/iterator.c index 072c6aacdb..d79aa5ec82 100644 --- a/refs/iterator.c +++ b/refs/iterator.c @@ -21,12 +21,6 @@ int ref_iterator_seek(struct ref_iterator *ref_iterator, const char *refname, return ref_iterator->vtable->seek(ref_iterator, refname, flags); } -int ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) -{ - return ref_iterator->vtable->peel(ref_iterator, peeled); -} - void ref_iterator_free(struct ref_iterator *ref_iterator) { if (ref_iterator) { @@ -60,12 +54,6 @@ static int empty_ref_iterator_seek(struct ref_iterator *ref_iterator UNUSED, return 0; } -static int empty_ref_iterator_peel(struct ref_iterator *ref_iterator UNUSED, - struct object_id *peeled UNUSED) -{ - BUG("peel called for empty iterator"); -} - static void empty_ref_iterator_release(struct ref_iterator *ref_iterator UNUSED) { } @@ -73,7 +61,6 @@ static void empty_ref_iterator_release(struct ref_iterator *ref_iterator UNUSED) static struct ref_iterator_vtable empty_ref_iterator_vtable = { .advance = empty_ref_iterator_advance, .seek = empty_ref_iterator_seek, - .peel = empty_ref_iterator_peel, .release = empty_ref_iterator_release, }; @@ -240,18 +227,6 @@ static int merge_ref_iterator_seek(struct ref_iterator *ref_iterator, return 0; } -static int merge_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) -{ - struct merge_ref_iterator *iter = - (struct merge_ref_iterator *)ref_iterator; - - if (!iter->current) { - BUG("peel called before advance for merge iterator"); - } - return ref_iterator_peel(*iter->current, peeled); -} - static void merge_ref_iterator_release(struct ref_iterator *ref_iterator) { struct merge_ref_iterator *iter = @@ -263,7 +238,6 @@ static void merge_ref_iterator_release(struct ref_iterator *ref_iterator) static struct ref_iterator_vtable merge_ref_iterator_vtable = { .advance = merge_ref_iterator_advance, .seek = merge_ref_iterator_seek, - .peel = merge_ref_iterator_peel, .release = merge_ref_iterator_release, }; @@ -412,15 +386,6 @@ static int prefix_ref_iterator_seek(struct ref_iterator *ref_iterator, return ref_iterator_seek(iter->iter0, refname, flags); } -static int prefix_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) -{ - struct prefix_ref_iterator *iter = - (struct prefix_ref_iterator *)ref_iterator; - - return ref_iterator_peel(iter->iter0, peeled); -} - static void prefix_ref_iterator_release(struct ref_iterator *ref_iterator) { struct prefix_ref_iterator *iter = @@ -432,7 +397,6 @@ static void prefix_ref_iterator_release(struct ref_iterator *ref_iterator) static struct ref_iterator_vtable prefix_ref_iterator_vtable = { .advance = prefix_ref_iterator_advance, .seek = prefix_ref_iterator_seek, - .peel = prefix_ref_iterator_peel, .release = prefix_ref_iterator_release, }; diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 1fefefd54e..6fa229edd0 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1030,22 +1030,6 @@ static int packed_ref_iterator_seek(struct ref_iterator *ref_iterator, return 0; } -static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) -{ - struct packed_ref_iterator *iter = - (struct packed_ref_iterator *)ref_iterator; - - if ((iter->base.ref.flags & REF_KNOWS_PEELED)) { - oidcpy(peeled, &iter->peeled); - return is_null_oid(&iter->peeled) ? -1 : 0; - } else if ((iter->base.ref.flags & (REF_ISBROKEN | REF_ISSYMREF))) { - return -1; - } else { - return peel_object(iter->repo, &iter->oid, peeled) ? -1 : 0; - } -} - static void packed_ref_iterator_release(struct ref_iterator *ref_iterator) { struct packed_ref_iterator *iter = @@ -1059,7 +1043,6 @@ static void packed_ref_iterator_release(struct ref_iterator *ref_iterator) static struct ref_iterator_vtable packed_ref_iterator_vtable = { .advance = packed_ref_iterator_advance, .seek = packed_ref_iterator_seek, - .peel = packed_ref_iterator_peel, .release = packed_ref_iterator_release, }; @@ -1525,13 +1508,8 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re if (cmp < 0) { /* Pass the old reference through. */ - - struct object_id peeled; - int peel_error = ref_iterator_peel(iter, &peeled); - if (write_packed_entry(out, iter->ref.name, - iter->ref.oid, - peel_error ? NULL : &peeled)) + iter->ref.oid, iter->ref.peeled_oid)) goto write_error; if ((ok = ref_iterator_advance(iter)) != ITER_OK) { diff --git a/refs/ref-cache.c b/refs/ref-cache.c index e427848879..ffef01a597 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -546,14 +546,6 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator, return 0; } -static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) -{ - struct cache_ref_iterator *iter = - (struct cache_ref_iterator *)ref_iterator; - return peel_object(iter->repo, ref_iterator->ref.oid, peeled) ? -1 : 0; -} - static void cache_ref_iterator_release(struct ref_iterator *ref_iterator) { struct cache_ref_iterator *iter = @@ -565,7 +557,6 @@ static void cache_ref_iterator_release(struct ref_iterator *ref_iterator) static struct ref_iterator_vtable cache_ref_iterator_vtable = { .advance = cache_ref_iterator_advance, .seek = cache_ref_iterator_seek, - .peel = cache_ref_iterator_peel, .release = cache_ref_iterator_release, }; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index f4f845bbea..4671517dad 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -357,12 +357,6 @@ typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator); typedef int ref_iterator_seek_fn(struct ref_iterator *ref_iterator, const char *refname, unsigned int flags); -/* - * Peels the current ref, returning 0 for success or -1 for failure. - */ -typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator, - struct object_id *peeled); - /* * Implementations of this function should free any resources specific * to the derived class. @@ -372,7 +366,6 @@ typedef void ref_iterator_release_fn(struct ref_iterator *ref_iterator); struct ref_iterator_vtable { ref_iterator_advance_fn *advance; ref_iterator_seek_fn *seek; - ref_iterator_peel_fn *peel; ref_iterator_release_fn *release; }; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index e214e120d7..e329d4a423 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -744,21 +744,6 @@ static int reftable_ref_iterator_seek(struct ref_iterator *ref_iterator, return iter->err; } -static int reftable_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) -{ - struct reftable_ref_iterator *iter = - (struct reftable_ref_iterator *)ref_iterator; - - if (iter->ref.value_type == REFTABLE_REF_VAL2) { - oidread(peeled, iter->ref.value.val2.target_value, - iter->refs->base.repo->hash_algo); - return 0; - } - - return -1; -} - static void reftable_ref_iterator_release(struct ref_iterator *ref_iterator) { struct reftable_ref_iterator *iter = @@ -776,7 +761,6 @@ static void reftable_ref_iterator_release(struct ref_iterator *ref_iterator) static struct ref_iterator_vtable reftable_ref_iterator_vtable = { .advance = reftable_ref_iterator_advance, .seek = reftable_ref_iterator_seek, - .peel = reftable_ref_iterator_peel, .release = reftable_ref_iterator_release, }; @@ -2098,13 +2082,6 @@ static int reftable_reflog_iterator_seek(struct ref_iterator *ref_iterator UNUSE return -1; } -static int reftable_reflog_iterator_peel(struct ref_iterator *ref_iterator UNUSED, - struct object_id *peeled UNUSED) -{ - BUG("reftable reflog iterator cannot be peeled"); - return -1; -} - static void reftable_reflog_iterator_release(struct ref_iterator *ref_iterator) { struct reftable_reflog_iterator *iter = @@ -2117,7 +2094,6 @@ static void reftable_reflog_iterator_release(struct ref_iterator *ref_iterator) static struct ref_iterator_vtable reftable_reflog_iterator_vtable = { .advance = reftable_reflog_iterator_advance, .seek = reftable_reflog_iterator_seek, - .peel = reftable_reflog_iterator_peel, .release = reftable_reflog_iterator_release, }; -- cgit v1.2.3 From 7ec85185b197ce1cd28721a6f4415fb9db5cd42f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 23 Oct 2025 09:16:20 +0200 Subject: object: add flag to `peel_object()` to verify object type When peeling a tag to a non-tag object we repeatedly call `parse_object()` on the tagged object until we find the first object that isn't a tag. While this feels sensible at first, there is a big catch here: `parse_object()` doesn't actually verify the type of the tagged object. The relevant code path here eventually ends up in `parse_tag_buffer()`. 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: 1. The object is already part of our cached objects. In that case we double-check whether the type we're trying to look up matches the type that was cached. 2. The object is _not_ part of our cached objects. In that case, we simply create a new object with the expected type, but we don't parse that object. In the first case we might notice type mismatches, but only in the case where our cache has the object with the correct type. In the second case, we'll blindly assume that the type is correct and then go with it. We'll only notice that the type might be wrong when we try to parse the object at a later point. Now arguably, we could change `parse_tag_buffer()` to verify the tagged object's type for us. But that would have the effect that such a tag cannot be parsed at all anymore, and we have a small bunch of tests for exactly this case that assert we still can open such tags. So this change does not feel like something we can retroactively tighten, even though one shouldn't ever hit such corrupted tags. Instead, add a new `flags` field to `peel_object()` that allows the caller to opt in to strict object verification. This will be wired up at a subset of callsites over the next few commits. Note that this change also inlines `deref_tag_noverify()`. There's only been two callsites of that function, the one we're changing and one in our test helpers. The latter callsite can trivially use `deref_tag()` instead, so by inlining the function we avoid having to pass down the flag. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- object.c | 20 +++++++++++++++++--- object.h | 15 ++++++++++++++- ref-filter.c | 2 +- refs.c | 2 +- refs/packed-backend.c | 5 ++--- refs/reftable-backend.c | 4 ++-- t/helper/test-reach.c | 2 +- tag.c | 12 ------------ tag.h | 1 - 9 files changed, 38 insertions(+), 25 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/object.c b/object.c index 986114a6db..e72b0ed436 100644 --- a/object.c +++ b/object.c @@ -209,11 +209,12 @@ struct object *lookup_object_by_type(struct repository *r, enum peel_status peel_object(struct repository *r, const struct object_id *name, - struct object_id *oid) + struct object_id *oid, + unsigned flags) { struct object *o = lookup_unknown_object(r, name); - if (o->type == OBJ_NONE) { + if (o->type == OBJ_NONE || flags & PEEL_OBJECT_VERIFY_OBJECT_TYPE) { int type = odb_read_object_info(r->objects, name, NULL); if (type < 0 || !object_as_type(o, type, 0)) return PEEL_INVALID; @@ -222,7 +223,20 @@ enum peel_status peel_object(struct repository *r, if (o->type != OBJ_TAG) return PEEL_NON_TAG; - o = deref_tag_noverify(r, o); + while (o && o->type == OBJ_TAG) { + o = parse_object(r, &o->oid); + if (o && o->type == OBJ_TAG && ((struct tag *)o)->tagged) { + o = ((struct tag *)o)->tagged; + + if (flags & PEEL_OBJECT_VERIFY_OBJECT_TYPE) { + int type = odb_read_object_info(r->objects, &o->oid, NULL); + if (type < 0 || !object_as_type(o, type, 0)) + return PEEL_INVALID; + } + } else { + o = NULL; + } + } if (!o) return PEEL_INVALID; diff --git a/object.h b/object.h index 8c3c1c46e1..1499f63d50 100644 --- a/object.h +++ b/object.h @@ -287,6 +287,17 @@ enum peel_status { PEEL_BROKEN = -4 }; +enum peel_object_flags { + /* + * Always verify the object type, even in the case where the looked-up + * object already has an object type. This can be useful when the + * stored object type may be invalid. One such case is when looking up + * objects via tags, where we blindly trust the object type declared by + * the tag. + */ + PEEL_OBJECT_VERIFY_OBJECT_TYPE = (1 << 0), +}; + /* * Peel the named object; i.e., if the object is a tag, resolve the * tag recursively until a non-tag is found. If successful, store the @@ -295,7 +306,9 @@ enum peel_status { * and leave oid unchanged. */ enum peel_status peel_object(struct repository *r, - const struct object_id *name, struct object_id *oid); + const struct object_id *name, + struct object_id *oid, + unsigned flags); struct object_list *object_list_insert(struct object *item, struct object_list **list_p); diff --git a/ref-filter.c b/ref-filter.c index 7fd8babec8..9a8ed8c8fc 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2581,7 +2581,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) if (need_tagged) { if (!is_null_oid(&ref->peeled_oid)) { oidcpy(&oi_deref.oid, &ref->peeled_oid); - } else if (!peel_object(the_repository, &obj->oid, &oi_deref.oid)) { + } else if (!peel_object(the_repository, &oi.oid, &oi_deref.oid, 0)) { /* We managed to peel the object ourselves. */ } else { die("bad tag"); diff --git a/refs.c b/refs.c index 9d8f0a9ca4..a41a94ae55 100644 --- a/refs.c +++ b/refs.c @@ -2333,7 +2333,7 @@ int reference_get_peeled_oid(struct repository *repo, return 0; } - return peel_object(repo, ref->oid, peeled_oid) ? -1 : 0; + return peel_object(repo, ref->oid, peeled_oid, 0) ? -1 : 0; } int refs_update_symref(struct ref_store *refs, const char *ref, diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 6fa229edd0..4752d3f398 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1527,9 +1527,8 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re i++; } else { struct object_id peeled; - int peel_error = peel_object(refs->base.repo, - &update->new_oid, - &peeled); + int peel_error = peel_object(refs->base.repo, &update->new_oid, + &peeled, 0); if (write_packed_entry(out, update->refname, &update->new_oid, diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index e329d4a423..9febb2322c 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1632,7 +1632,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data ref.refname = (char *)u->refname; ref.update_index = ts; - peel_error = peel_object(arg->refs->base.repo, &u->new_oid, &peeled); + peel_error = peel_object(arg->refs->base.repo, &u->new_oid, &peeled, 0); if (!peel_error) { ref.value_type = REFTABLE_REF_VAL2; memcpy(ref.value.val2.target_value, peeled.hash, GIT_MAX_RAWSZ); @@ -2497,7 +2497,7 @@ static int write_reflog_expiry_table(struct reftable_writer *writer, void *cb_da ref.refname = (char *)arg->refname; ref.update_index = ts; - if (!peel_object(arg->refs->base.repo, &arg->update_oid, &peeled)) { + if (!peel_object(arg->refs->base.repo, &arg->update_oid, &peeled, 0)) { ref.value_type = REFTABLE_REF_VAL2; memcpy(ref.value.val2.target_value, peeled.hash, GIT_MAX_RAWSZ); memcpy(ref.value.val2.value, arg->update_oid.hash, GIT_MAX_RAWSZ); diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c index 028ec00306..c58c93800f 100644 --- a/t/helper/test-reach.c +++ b/t/helper/test-reach.c @@ -63,7 +63,7 @@ int cmd__reach(int ac, const char **av) die("failed to resolve %s", buf.buf + 2); orig = parse_object(r, &oid); - peeled = deref_tag_noverify(the_repository, orig); + peeled = deref_tag(the_repository, orig, NULL, 0); if (!peeled) die("failed to load commit for input %s resulting in oid %s", diff --git a/tag.c b/tag.c index 1d52686ee1..f5c232d2f1 100644 --- a/tag.c +++ b/tag.c @@ -94,18 +94,6 @@ struct object *deref_tag(struct repository *r, struct object *o, const char *war return o; } -struct object *deref_tag_noverify(struct repository *r, struct object *o) -{ - while (o && o->type == OBJ_TAG) { - o = parse_object(r, &o->oid); - if (o && o->type == OBJ_TAG && ((struct tag *)o)->tagged) - o = ((struct tag *)o)->tagged; - else - o = NULL; - } - return o; -} - struct tag *lookup_tag(struct repository *r, const struct object_id *oid) { struct object *obj = lookup_object(r, oid); diff --git a/tag.h b/tag.h index c49d7c19ad..ef12a61037 100644 --- a/tag.h +++ b/tag.h @@ -16,7 +16,6 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u int parse_tag(struct tag *item); void release_tag_memory(struct tag *t); struct object *deref_tag(struct repository *r, struct object *, const char *, int); -struct object *deref_tag_noverify(struct repository *r, struct object *); int gpg_verify_tag(const struct object_id *oid, const char *name_to_report, unsigned flags); struct object_id *get_tagged_oid(struct tag *tag); -- cgit v1.2.3 From 6ec4c0b45ba916770c6fbc03df94018ca06fb77e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 23 Oct 2025 09:16:21 +0200 Subject: refs: don't store peeled object IDs for invalid tags Both the "files" and "reftable" backend store peeled object IDs for references that point to tags: - The "files" backend stores the value when packing refs, where each peeled object ID is prefixed with "^". - The "reftable" backend stores the value whenever writing a new reference that points to a tag via a special ref record type. Both of these backends use `peel_object()` to find the peeled object ID. But as explained in the preceding commit, that function does not detect the case where the tag's tagged object and its claimed type mismatch. The consequence of storing these bogus peeled object IDs is that we're less likely to detect such corruption in other parts of Git. git-for-each-ref(1) for example does not notice anymore that the tag is broken when using "--format=%(*objectname)" to dereference tags. One could claim that this is good, because it still allows us to mostly use the tag as intended. But the biggest problem here is that we now have different behaviour for such a broken tag depending on whether or not we have its peeled value in the refdb. Fix the issue by verifying the object type when peeling the object. If that verification fails we simply skip storing the peeled value in either of the reference formats. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 2 +- refs/reftable-backend.c | 3 ++- t/pack-refs-tests.sh | 32 ++++++++++++++++++++++++++++++++ t/t0610-reftable-basics.sh | 28 ++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 2 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 4752d3f398..1ab0c50393 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1528,7 +1528,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re } else { struct object_id peeled; int peel_error = peel_object(refs->base.repo, &update->new_oid, - &peeled, 0); + &peeled, PEEL_OBJECT_VERIFY_OBJECT_TYPE); if (write_packed_entry(out, update->refname, &update->new_oid, diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 9febb2322c..6bbfd5618d 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1632,7 +1632,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data ref.refname = (char *)u->refname; ref.update_index = ts; - peel_error = peel_object(arg->refs->base.repo, &u->new_oid, &peeled, 0); + peel_error = peel_object(arg->refs->base.repo, &u->new_oid, &peeled, + PEEL_OBJECT_VERIFY_OBJECT_TYPE); if (!peel_error) { ref.value_type = REFTABLE_REF_VAL2; memcpy(ref.value.val2.target_value, peeled.hash, GIT_MAX_RAWSZ); diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh index 3dbcc01718..095823d915 100644 --- a/t/pack-refs-tests.sh +++ b/t/pack-refs-tests.sh @@ -428,4 +428,36 @@ do ' done +test_expect_success 'pack-refs does not store invalid peeled tag value' ' + test_when_finished rm -rf repo && + git init repo && + ( + cd repo && + git commit --allow-empty --message initial && + + echo garbage >blob-content && + blob_id=$(git hash-object -w -t blob blob-content) && + + # Write an invalid tag into the object database. The tag itself + # is well-formed, but the tagged object is a blob while we + # claim that it is a commit. + cat >tag-content <<-EOF && + object $blob_id + type commit + tag bad-tag + tagger C O Mitter 1112354055 +0200 + + annotated + EOF + tag_id=$(git hash-object -w -t tag tag-content) && + git update-ref refs/tags/bad-tag "$tag_id" && + + # The packed-refs file should not contain the peeled object ID. + # If it did this would cause commands that use the peeled value + # to not notice this corrupted tag. + git pack-refs --all && + test_grep ! "^\^" .git/packed-refs + ) +' + test_done diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 3ea5d51532..6575528f21 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -1135,4 +1135,32 @@ test_expect_success 'fetch: accessing FETCH_HEAD special ref works' ' test_cmp expect actual ' +test_expect_success 'writes do not persist peeled value for invalid tags' ' + test_when_finished rm -rf repo && + git init repo && + ( + cd repo && + git commit --allow-empty --message initial && + + # We cannot easily verify that the peeled value is not stored + # in the tables. Instead, we test this indirectly: we create + # two tags that both point to the same object, but they claim + # different object types. If we parse both tags we notice that + # the parsed tagged object has a mismatch between the two tags + # and bail out. + # + # If we instead use the persisted peeled value we would not + # even parse the tags. As such, we would not notice the + # discrepancy either and thus listing these tags would succeed. + git tag tag-1 -m "tag 1" && + git cat-file tag tag-1 >raw-tag && + sed "s/^type commit$/type blob/" broken-tag && + broken_tag_id=$(git hash-object -w -t tag broken-tag) && + git update-ref refs/tags/tag-2 $broken_tag_id && + + test_must_fail git for-each-ref --format="%(*objectname)" refs/tags/ 2>err && + test_grep "bad tag pointer" err + ) +' + test_done -- cgit v1.2.3