git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/13] refs: improvements and fixes for peeling tags
@ 2025-10-07 10:58 Patrick Steinhardt
  2025-10-07 10:58 ` [PATCH 01/13] refs: introduce wrapper struct for `each_ref_fn` Patrick Steinhardt
                   ` (17 more replies)
  0 siblings, 18 replies; 106+ messages in thread
From: Patrick Steinhardt @ 2025-10-07 10:58 UTC (permalink / raw)
  To: git

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


^ permalink raw reply related	[flat|nested] 106+ messages in thread

end of thread, other threads:[~2025-11-04 23:54 UTC | newest]

Thread overview: 106+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-07 10:58 [PATCH 00/13] refs: improvements and fixes for peeling tags Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 01/13] refs: introduce wrapper struct for `each_ref_fn` Patrick Steinhardt
2025-10-07 18:05   ` Justin Tobler
2025-10-08 13:42     ` Patrick Steinhardt
2025-10-07 21:56   ` Taylor Blau
2025-10-08 15:52   ` shejialuo
2025-10-09  6:03     ` Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 02/13] refs: introduce `.ref` field for the base iterator Patrick Steinhardt
2025-10-07 14:24   ` Karthik Nayak
2025-10-08 13:44     ` Patrick Steinhardt
2025-10-08 15:03       ` Patrick Steinhardt
2025-10-07 20:19   ` Justin Tobler
2025-10-07 21:57   ` Taylor Blau
2025-10-07 10:58 ` [PATCH 03/13] refs: refactor reference status flags Patrick Steinhardt
2025-10-07 14:27   ` Karthik Nayak
2025-10-08 13:44     ` Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 04/13] refs: expose peeled object ID via the iterator Patrick Steinhardt
2025-10-07 14:52   ` Karthik Nayak
2025-10-08 13:45     ` Patrick Steinhardt
2025-10-15  8:28       ` Karthik Nayak
2025-10-07 10:58 ` [PATCH 05/13] upload-pack: convert to use `reference_get_peeled_oid()` Patrick Steinhardt
2025-10-07 16:18   ` Karthik Nayak
2025-10-08 13:45     ` Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 06/13] ref-filter: propagate peeled object ID Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 07/13] builtin/show-ref: convert to use `reference_get_peeled_oid()` Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 08/13] refs: drop `current_ref_iter` hack Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 09/13] refs: drop infrastructure to peel via iterators Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 10/13] object: add flag to `peel_object()` to verify object type Patrick Steinhardt
2025-10-08 11:04   ` Kristoffer Haugsbakk
2025-10-07 10:58 ` [PATCH 11/13] refs: don't store peeled object IDs for invalid tags Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 12/13] ref-filter: detect broken tags when dereferencing them Patrick Steinhardt
2025-10-07 10:58 ` [PATCH 13/13] ref-filter: parse objects on demand Patrick Steinhardt
2025-10-08 11:05   ` Kristoffer Haugsbakk
2025-10-08 13:45     ` Patrick Steinhardt
2025-10-07 21:00 ` [PATCH 00/13] refs: improvements and fixes for peeling tags Junio C Hamano
2025-10-07 21:49   ` Taylor Blau
2025-10-07 23:01 ` Junio C Hamano
2025-10-08 15:50 ` [PATCH v2 00/14] " Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 01/14] refs: introduce wrapper struct for `each_ref_fn` Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 02/14] refs: introduce `.ref` field for the base iterator Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 03/14] refs: fully reset `struct ref_iterator::ref` on iteration Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 04/14] refs: refactor reference status flags Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 05/14] refs: expose peeled object ID via the iterator Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 06/14] upload-pack: convert to use `reference_get_peeled_oid()` Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 07/14] ref-filter: propagate peeled object ID Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 08/14] builtin/show-ref: convert to use `reference_get_peeled_oid()` Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 09/14] refs: drop `current_ref_iter` hack Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 10/14] refs: drop infrastructure to peel via iterators Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 11/14] object: add flag to `peel_object()` to verify object type Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 12/14] refs: don't store peeled object IDs for invalid tags Patrick Steinhardt
2025-10-08 16:27     ` shejialuo
2025-10-09  5:22       ` Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 13/14] ref-filter: detect broken tags when dereferencing them Patrick Steinhardt
2025-10-08 15:50   ` [PATCH v2 14/14] ref-filter: parse objects on demand Patrick Steinhardt
2025-10-09  5:38   ` [PATCH v2 00/14] refs: improvements and fixes for peeling tags Jeff King
2025-10-09  6:09     ` Patrick Steinhardt
2025-10-09  6:39       ` Jeff King
2025-10-09  7:24         ` Patrick Steinhardt
2025-10-10  5:12           ` Jeff King
2025-10-10  5:22             ` Patrick Steinhardt
2025-10-10  6:26               ` Jeff King
2025-10-10 15:29               ` Junio C Hamano
2025-10-14  6:31                 ` Patrick Steinhardt
2025-10-14 16:52                   ` Junio C Hamano
2025-10-09 10:11         ` Toon Claes
2025-10-09 19:37     ` Junio C Hamano
2025-10-22  6:41 ` [PATCH v3 " Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 01/14] refs: introduce wrapper struct for `each_ref_fn` Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 02/14] refs: introduce `.ref` field for the base iterator Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 03/14] refs: fully reset `struct ref_iterator::ref` on iteration Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 04/14] refs: refactor reference status flags Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 05/14] refs: expose peeled object ID via the iterator Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 06/14] upload-pack: convert to use `reference_get_peeled_oid()` Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 07/14] ref-filter: propagate peeled object ID Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 08/14] builtin/show-ref: convert to use `reference_get_peeled_oid()` Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 09/14] refs: drop `current_ref_iter` hack Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 10/14] refs: drop infrastructure to peel via iterators Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 11/14] object: add flag to `peel_object()` to verify object type Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 12/14] refs: don't store peeled object IDs for invalid tags Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 13/14] ref-filter: detect broken tags when dereferencing them Patrick Steinhardt
2025-10-22  6:41   ` [PATCH v3 14/14] ref-filter: parse objects on demand Patrick Steinhardt
2025-10-22 15:27     ` Junio C Hamano
2025-10-23  6:00       ` Patrick Steinhardt
2025-10-22 10:57   ` [PATCH v3 00/14] refs: improvements and fixes for peeling tags Karthik Nayak
2025-10-22 14:47   ` Junio C Hamano
2025-10-23  5:52     ` Patrick Steinhardt
2025-10-23  7:16 ` [PATCH v4 " Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 01/14] refs: introduce wrapper struct for `each_ref_fn` Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 02/14] refs: introduce `.ref` field for the base iterator Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 03/14] refs: fully reset `struct ref_iterator::ref` on iteration Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 04/14] refs: refactor reference status flags Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 05/14] refs: expose peeled object ID via the iterator Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 06/14] upload-pack: convert to use `reference_get_peeled_oid()` Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 07/14] ref-filter: propagate peeled object ID Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 08/14] builtin/show-ref: convert to use `reference_get_peeled_oid()` Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 09/14] refs: drop `current_ref_iter` hack Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 10/14] refs: drop infrastructure to peel via iterators Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 11/14] object: add flag to `peel_object()` to verify object type Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 12/14] refs: don't store peeled object IDs for invalid tags Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 13/14] ref-filter: detect broken tags when dereferencing them Patrick Steinhardt
2025-10-23  7:16   ` [PATCH v4 14/14] ref-filter: parse objects on demand Patrick Steinhardt
2025-11-04 22:07     ` Jeff King
2025-11-04 23:40       ` Junio C Hamano
2025-11-04 23:54         ` Jeff King
2025-10-23 23:06   ` [PATCH v4 00/14] refs: improvements and fixes for peeling tags Junio C Hamano
2025-10-24  5:12     ` Patrick Steinhardt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).