reftable: fix quadratic behavior when re-creating deleted refs #2166

gitgitgadget · 2026-07-07T16:06:08Z

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Mon, Jul 06, 2026 at 01:35:56PM +0000, Kristofer Karlsson via GitGitGadget wrote: > From: Kristofer Karlsson <krka@spotify.com> > > When many refs are deleted and then re-created, update-ref exhibits > quadratic behavior. With 8000 refs deleted and re-created, the > runtime is ~15s, quadrupling for each doubling of input size. > > The root cause is the merged iterator's suppress_deletions flag. > When set, merged_iter_next_void() silently consumes tombstone records > in a tight internal loop before returning to the caller. This > prevents higher-level code from checking iteration bounds (such as > prefix or refname comparisons) until after all tombstones have been > scanned. > > This affects two code paths during ref creation: > > - refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to > check for D/F conflicts and must scan through all subsequent > tombstones before the caller can see that they are past the prefix > of interest. > > - reftable_backend_read_ref() seeks to a specific refname and must > scan through all subsequent tombstones before returning "not > found", because the merged iterator skips the matching tombstone > and searches for the next live record. It probably not only impacts reference creation, but also every reader that wants to search for a specific reference that doesn't exist. > Fix this by removing suppress_deletions from the merged iterator and > instead handling deletion records at each call site in the reftable > backend, where prefix and refname bounds are available. Tombstones > are now returned to callers, which skip them after their existing > bounds checks. This allows iteration to terminate as soon as a > tombstone past the relevant bound is encountered. This option is still used by downstream users of the reftable library, like libgit2. So we shouldn't just delete it outright. > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > index 4ae22922de..8c4f119ff1 100644 > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -633,6 +633,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator) > break; > } > > + if (iter->ref.value_type == REFTABLE_REF_DELETION) > + continue; > + > if (iter->exclude_patterns && should_exclude_current_ref(iter)) > continue; > Okay. I was first wondering whether we should move this call earlier. But we actually don't want to, as this is the code that precedes the above: if (iter->prefix_len && strncmp(iter->prefix, iter->ref.refname, iter->prefix_len)) { iter->err = 1; break; } So this allows us to not only skip the current iteration, but completely abort iteration by observing tombstones that sort after our prefix. In any case, as far as I can see all sites where we iterate through either ref or log records have been adapted to handle deletions. Thanks! Patrick

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Tue, 7 Jul 2026 at 17:24, Patrick Steinhardt <ps@pks.im> wrote: > > > This affects two code paths during ref creation: > > > > - refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to > > check for D/F conflicts and must scan through all subsequent > > tombstones before the caller can see that they are past the prefix > > of interest. > > > > - reftable_backend_read_ref() seeks to a specific refname and must > > scan through all subsequent tombstones before returning "not > > found", because the merged iterator skips the matching tombstone > > and searches for the next live record. > > It probably not only impacts reference creation, but also every reader > that wants to search for a specific reference that doesn't exist. Hm good point, I will try to rephrase this better. > > Fix this by removing suppress_deletions from the merged iterator and > > instead handling deletion records at each call site in the reftable > > backend, where prefix and refname bounds are available. Tombstones > > are now returned to callers, which skip them after their existing > > bounds checks. This allows iteration to terminate as soon as a > > tombstone past the relevant bound is encountered. > > This option is still used by downstream users of the reftable library, > like libgit2. So we shouldn't just delete it outright. Good catch! I can keep suppress_deletions as-is and just stop setting it from stack.c. That way libgit2 is unchanged, while we still optimize it at the other call sites. The reftable library diff then shrinks to a single removed line. > > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > > index 4ae22922de..8c4f119ff1 100644 > > --- a/refs/reftable-backend.c > > +++ b/refs/reftable-backend.c > > @@ -633,6 +633,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator) > > break; > > } > > > > + if (iter->ref.value_type == REFTABLE_REF_DELETION) > > + continue; > > + > > if (iter->exclude_patterns && should_exclude_current_ref(iter)) > > continue; > > > > Okay. I was first wondering whether we should move this call earlier. > But we actually don't want to, as this is the code that precedes the > above: > > if (iter->prefix_len && > strncmp(iter->prefix, iter->ref.refname, iter->prefix_len)) { > iter->err = 1; > break; > } > > So this allows us to not only skip the current iteration, but completely > abort iteration by observing tombstones that sort after our prefix. Indeed, this is the primary win. > In any case, as far as I can see all sites where we iterate through > either ref or log records have been adapted to handle deletions. Thanks! Appreciate the review (and spotting the libgit breakage!) Kristofer

gitgitgadget · 2026-07-09T14:08:53Z

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Thu, Jul 09, 2026 at 12:08:31PM +0000, Kristofer Karlsson via GitGitGadget wrote: > diff --git a/reftable/stack.c b/reftable/stack.c > index ab12926708..fd7d8f3f1e 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -337,7 +337,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, > /* Update the stack to point to the new tables. */ > if (st->merged) > reftable_merged_table_free(st->merged); > - new_merged->suppress_deletions = 1; > st->merged = new_merged; > > if (st->tables) Okay, we still retain the field after this patch. But the question is: how would libgit2 now set it? I think we should rather extend the `struct reftable_stack_options` so that the caller can control whether or not to suppress deletions at stack creation time. Thanks! Patrick

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Thu, 9 Jul 2026 at 15:53, Patrick Steinhardt <ps@pks.im> wrote: > > On Thu, Jul 09, 2026 at 12:08:31PM +0000, Kristofer Karlsson via GitGitGadget wrote: > > diff --git a/reftable/stack.c b/reftable/stack.c > > index ab12926708..fd7d8f3f1e 100644 > > --- a/reftable/stack.c > > +++ b/reftable/stack.c > > @@ -337,7 +337,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, > > /* Update the stack to point to the new tables. */ > > if (st->merged) > > reftable_merged_table_free(st->merged); > > - new_merged->suppress_deletions = 1; > > st->merged = new_merged; > > > > if (st->tables) > > Okay, we still retain the field after this patch. But the question is: > how would libgit2 now set it? I think we should rather extend the > `struct reftable_stack_options` so that the caller can control whether > or not to suppress deletions at stack creation time. You are right, I (still) missed the compatibility problem here. I started thinking about a way to make it fully backwards compatible, but then I looked at the libgit2 repo and realized it will need updating anyway since it predates the reftable_stack_options split. I will add suppress_deletions to reftable_stack_options as you suggested. Thanks, Kristofer

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Thu, Jul 09, 2026 at 04:48:43PM +0200, Kristofer Karlsson wrote: > On Thu, 9 Jul 2026 at 15:53, Patrick Steinhardt <ps@pks.im> wrote: > > > > On Thu, Jul 09, 2026 at 12:08:31PM +0000, Kristofer Karlsson via GitGitGadget wrote: > > > diff --git a/reftable/stack.c b/reftable/stack.c > > > index ab12926708..fd7d8f3f1e 100644 > > > --- a/reftable/stack.c > > > +++ b/reftable/stack.c > > > @@ -337,7 +337,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, > > > /* Update the stack to point to the new tables. */ > > > if (st->merged) > > > reftable_merged_table_free(st->merged); > > > - new_merged->suppress_deletions = 1; > > > st->merged = new_merged; > > > > > > if (st->tables) > > > > Okay, we still retain the field after this patch. But the question is: > > how would libgit2 now set it? I think we should rather extend the > > `struct reftable_stack_options` so that the caller can control whether > > or not to suppress deletions at stack creation time. > > You are right, I (still) missed the compatibility problem here. > > I started thinking about a way to make it fully backwards compatible, > but then I looked at the libgit2 repo and realized it will need > updating anyway since it predates the reftable_stack_options split. Yeah, that's something I'll handle soon(ish). > I will add suppress_deletions to reftable_stack_options as you > suggested. Thanks! Patrick

gitgitgadget · 2026-07-10T15:53:18Z

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Fri, Jul 10, 2026 at 10:36:07AM +0000, Kristofer Karlsson via GitGitGadget wrote: > From: Kristofer Karlsson <krka@spotify.com> > > When many tombstones are present in a reftable, operations that need > to look up or iterate over refs exhibit quadratic behavior. With > 8000 refs deleted and re-created, update-ref takes ~15s, quadrupling > for each doubling of input size. > > The root cause is the merged iterator's suppress_deletions flag. > When set, merged_iter_next_void() silently consumes tombstone records > in a tight internal loop before returning to the caller. This > prevents higher-level code from checking iteration bounds (such as > prefix or refname comparisons) until after all tombstones have been > scanned. > > This affects any code path that seeks into a range containing > tombstones, including: > > - refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to > check for D/F conflicts and must scan through all subsequent > tombstones before the caller can see that they are past the prefix > of interest. > > - reftable_backend_read_ref() seeks to a specific refname and must > scan through all subsequent tombstones before returning "not > found", because the merged iterator skips the matching tombstone > and searches for the next live record. > > Fix this by making suppress_deletions configurable via > reftable_stack_options instead of unconditionally enabling it. Git > no longer sets the flag, so tombstones are now returned to callers in > the reftable backend, which skip them after their existing bounds > checks. This allows iteration to terminate as soon as a tombstone > past the relevant bound is encountered. > > Downstream users of the reftable library (e.g. libgit2) can still > enable suppress_deletions through the stack options to retain the > previous behavior. > > This also requires adding deletion checks to the log iteration paths, > since suppress_deletions applied to both ref and log iterators. Nit: s/applied/applies/ > diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h > index 11f9963f4f..5d22d84e80 100644 > --- a/reftable/reftable-stack.h > +++ b/reftable/reftable-stack.h > @@ -42,6 +42,8 @@ struct reftable_stack_options { > */ > void (*on_reload)(void *payload); > void *on_reload_payload; > + > + int suppress_deletions; > }; A comment would've been nice, but I don't think this warrants a reroll. > diff --git a/reftable/stack.c b/reftable/stack.c > index ab12926708..caaedf24d6 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -337,7 +337,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, > /* Update the stack to point to the new tables. */ > if (st->merged) > reftable_merged_table_free(st->merged); > - new_merged->suppress_deletions = 1; > + new_merged->suppress_deletions = st->opts.suppress_deletions; > st->merged = new_merged; Yup, this looks good to me. Thanks! Patrick

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Fri, 10 Jul 2026 at 16:32, Patrick Steinhardt <ps@pks.im> wrote: > > > This also requires adding deletion checks to the log iteration paths, > > since suppress_deletions applied to both ref and log iterators. > > Nit: s/applied/applies/ Language and using correct tense is always the tricky part -- will fix if a reroll is needed for other reasons. > > + int suppress_deletions; > > A comment would've been nice, but I don't think this warrants a reroll. Agreed, the field name felt self-documenting to me, but I will add a short comment if there is a reroll. Something like this? "boolean: filters out tombstoned/deleted refs early if true" > > - new_merged->suppress_deletions = 1; > > + new_merged->suppress_deletions = st->opts.suppress_deletions; > > Yup, this looks good to me. Thanks for the quick review. Another thing I have been thinking about: should we consider suppress_deletions a temporary stopgap, with the goal of eventually removing it? I took a look at libgit2's refdb_reftable.c to see what it would look like. It doesn't seem _too_ complicated (but I have been wrong about complexity before): reftable_stack_read_ref() and reftable_stack_read_log() already check is_deletion() after the seek+next, so the call sites that use those would work correctly without suppress_deletions too. (I think?) The other call sites that iterate would need the same type of filter as we have in this patch series. So the total cost for libgit2 to stop relying on suppress_deletions would be fairly small and it would maybe also got a nice performance boost for the edge cases, though I have not attempted to verify that. That said, it does not affect this patch - regardless of the future we will need this flag now. Thanks, Kristofer

gitgitgadget · 2026-07-07T16:05:41Z

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Mon, Jul 06, 2026 at 01:35:55PM +0000, Kristofer Karlsson via GitGitGadget wrote: > diff --git a/t/perf/p1401-ref-store-tombstones.sh b/t/perf/p1401-ref-store-tombstones.sh > new file mode 100755 > index 0000000000..e40a6dcbf4 > --- /dev/null > +++ b/t/perf/p1401-ref-store-tombstones.sh > @@ -0,0 +1,44 @@ > +#!/bin/sh > + > +test_description="Tests performance of ref operations with many tombstones" > + > +. ./perf-lib.sh > + > +test_expect_success "setup" ' > + git init --ref-format=reftable repo && > + blob=$(echo foo | git -C repo hash-object -w --stdin) && > + for i in $(test_seq 8000) > + do > + printf "create refs/tags/tag-%d %s\n" "$i" "$blob" || > + return 1 > + done >repo/input && > + git -C repo update-ref --stdin <repo/input && > + git -C repo for-each-ref --format="delete %(refname)" | > + git -C repo update-ref --stdin > +' > + > +test_perf "recreate refs after mass delete" ' > + git -C repo update-ref --stdin <repo/input && > + git -C repo for-each-ref --format="delete %(refname)" | > + git -C repo update-ref --stdin > +' You're not only benchmarking the reference recreation, but also their deletion. If I'm not misreading things, then you can queue cleanups via `test_when_finished`, and these calls will not be measured. > +test_expect_success "setup asymmetric" ' > + for i in $(test_seq 8000) > + do > + printf "create refs/tags/old-%d %s\n" "$i" "$blob" || > + return 1 > + done >repo/input-old && > + sed "s/old-/new-/" <repo/input-old >repo/input-new && > + git -C repo update-ref --stdin <repo/input-old && > + git -C repo for-each-ref --format="delete %(refname)" | > + git -C repo update-ref --stdin > +' Would it make sense to use separate repositories? Otherwise, state from the preceding benchmark(s) will impact subsequent ones. > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > index e19e036898..4b7cfe38e4 100755 > --- a/t/t0610-reftable-basics.sh > +++ b/t/t0610-reftable-basics.sh > @@ -1163,4 +1163,26 @@ test_expect_success 'writes do not persist peeled value for invalid tags' ' > ) > ' > > +test_expect_success 'delete and re-create refs with tombstones' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + test_commit -C repo A && > + A=$(git -C repo rev-parse HEAD) && > + cat >input <<-EOF && > + create refs/tags/a $A > + create refs/tags/b $A > + create refs/tags/c $A > + EOF > + git -C repo update-ref --stdin <input && > + > + # delete all tags, leaving tombstones > + git -C repo for-each-ref --format="delete %(refname)" refs/tags/ | > + git -C repo update-ref --stdin && > + > + # re-create the same refs and verify they are visible > + git -C repo update-ref --stdin <input && > + git -C repo tag -l >actual && > + test_line_count = 3 actual > +' I wonder whether this test really adds any value. We probably have lots of tests already that test creation/deletion of references. Patrick

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Tue, 7 Jul 2026 at 17:24, Patrick Steinhardt <ps@pks.im> wrote: > > On Mon, Jul 06, 2026 at 01:35:55PM +0000, Kristofer Karlsson via GitGitGadget wrote: > > diff --git a/t/perf/p1401-ref-store-tombstones.sh b/t/perf/p1401-ref-store-tombstones.sh > > new file mode 100755 > > index 0000000000..e40a6dcbf4 > > --- /dev/null > > +++ b/t/perf/p1401-ref-store-tombstones.sh > > @@ -0,0 +1,44 @@ > > +#!/bin/sh > > + > > +test_description="Tests performance of ref operations with many tombstones" > > + > > +. ./perf-lib.sh > > + > > +test_expect_success "setup" ' > > + git init --ref-format=reftable repo && > > + blob=$(echo foo | git -C repo hash-object -w --stdin) && > > + for i in $(test_seq 8000) > > + do > > + printf "create refs/tags/tag-%d %s\n" "$i" "$blob" || > > + return 1 > > + done >repo/input && > > + git -C repo update-ref --stdin <repo/input && > > + git -C repo for-each-ref --format="delete %(refname)" | > > + git -C repo update-ref --stdin > > +' > > + > > +test_perf "recreate refs after mass delete" ' > > + git -C repo update-ref --stdin <repo/input && > > + git -C repo for-each-ref --format="delete %(refname)" | > > + git -C repo update-ref --stdin > > +' > > You're not only benchmarking the reference recreation, but also their > deletion. If I'm not misreading things, then you can queue cleanups via > `test_when_finished`, and these calls will not be measured. I don't think measuring the full create+delete cycle is wrong per se, but you are right that if we can benchmark something more isolated is even more useful. I will try to split this up better. > > +test_expect_success "setup asymmetric" ' > > + for i in $(test_seq 8000) > > + do > > + printf "create refs/tags/old-%d %s\n" "$i" "$blob" || > > + return 1 > > + done >repo/input-old && > > + sed "s/old-/new-/" <repo/input-old >repo/input-new && > > + git -C repo update-ref --stdin <repo/input-old && > > + git -C repo for-each-ref --format="delete %(refname)" | > > + git -C repo update-ref --stdin > > +' > > Would it make sense to use separate repositories? Otherwise, state from > the preceding benchmark(s) will impact subsequent ones. Agreed, I can use a fresh repo for each scenario. > > > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > > +test_expect_success 'delete and re-create refs with tombstones' ' > > I wonder whether this test really adds any value. We probably have lots > of tests already that test creation/deletion of references. I could not find an existing test that covers the delete-then-recreate flow (where tombstones are present when the new refs are created). The existing tests cover creation and deletion separately but not the interaction with tombstones. (But perhaps such a test exists and I just can't find it.) Thanks, Kristofer

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Tue, Jul 07, 2026 at 06:12:31PM +0200, Kristofer Karlsson wrote: > On Tue, 7 Jul 2026 at 17:24, Patrick Steinhardt <ps@pks.im> wrote: > > On Mon, Jul 06, 2026 at 01:35:55PM +0000, Kristofer Karlsson via GitGitGadget wrote: > > > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > > > +test_expect_success 'delete and re-create refs with tombstones' ' > > > > I wonder whether this test really adds any value. We probably have lots > > of tests already that test creation/deletion of references. > > I could not find an existing test that covers the delete-then-recreate > flow (where tombstones are present when the new refs are created). > The existing tests cover creation and deletion separately but not the > interaction with tombstones. > (But perhaps such a test exists and I just can't find it.) In t1400 we definitely have some tests where we exercise this implicitly. In any case, if we want to retain this test I'd rather add it to t1400 itself, as the functionality that we're testing is itself not specific to the backend. Patrick

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Wed, 8 Jul 2026 at 08:00, Patrick Steinhardt <ps@pks.im> wrote: > > > > > I could not find an existing test that covers the delete-then-recreate > > flow (where tombstones are present when the new refs are created). > > The existing tests cover creation and deletion separately but not the > > interaction with tombstones. > > (But perhaps such a test exists and I just can't find it.) > > In t1400 we definitely have some tests where we exercise this > implicitly. In any case, if we want to retain this test I'd rather add > it to t1400 itself, as the functionality that we're testing is itself > not specific to the backend. Thanks, you are right about the placement -- the contract is valid regardless of backend. You are also right about it already being tested this is implicitly tested between multiple test runs since they have shared state (the repo). Multiple tests delete the ref as clean up and multiple tests also create a ref and verifies it. So it is technically covered but it depends on multiple tests being executed. I think this is simply exposing my personal preference to have more self-contained and explicit tests, but I am happy to drop the added tests -- it is perhaps more important to avoid bloating the test code. I will drop the added correctness test for the next iteration. Thanks, Kristofer

-Original file line number
+Diff line change
@@ Expand Up @@
     	if (ret)
     		goto done;
-    	if (strcmp(ref.refname, refname)) {
+    	if (strcmp(ref.refname, refname) ||
+    	    reftable_ref_record_is_deletion(&ref)) {
     		ret = 1;
     		goto done;
     	}
@@ Expand All @@
     		oidread(oid, reftable_ref_record_val1(&ref),
     			&hash_algos[hash_id]);
     	} else {
-    		/* We got a tombstone, which should not happen. */
     		BUG("unhandled reference value type %d", ref.value_type);
     	}
@@ Expand Down Expand Up @@
     			break;
     		}
+    		if (iter->ref.value_type == REFTABLE_REF_DELETION)
+    			continue;
     		if (iter->exclude_patterns && should_exclude_current_ref(iter))
     			continue;
@@ Expand Down Expand Up @@
     					ret = 0;
     					break;
     				}
+    				if (reftable_log_record_is_deletion(&log))
+    					continue;
     				ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
     				tombstone = &logs[logs_nr++];
@@ Expand Down Expand Up @@
     			ret = 0;
     			break;
     		}
+    		if (reftable_log_record_is_deletion(&old_log))
+    			continue;
     		free(old_log.refname);
@@ Expand Down Expand Up @@
     		if (iter->err)
     			break;
+    		if (reftable_log_record_is_deletion(&iter->log))
+    			continue;
     		/*
     		 * We want the refnames that we have reflogs for, so we skip if
     		 * we've already produced this name. This could be faster by
@@ Expand Down Expand Up @@
     			ret = 0;
     			break;
     		}
+    		if (reftable_log_record_is_deletion(&log))
+    			continue;
     		ret = yield_log_record(refs, &log, fn, cb_data);
     		if (ret)
@@ Expand Down Expand Up @@
     			ret = 0;
     			break;
     		}
+    		if (reftable_log_record_is_deletion(&log)) {
+    			reftable_log_record_release(&log);
+    			continue;
+    		}
     		ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
     		logs[logs_nr++] = log;
@@ Expand Down Expand Up @@
     		goto done;
     	/*
-    	 * Check whether we get at least one log record for the given ref name.
-    	 * If so, the reflog exists, otherwise it doesn't.
+    	 * Check whether we get at least one non-deleted log record for the
+    	 * given ref name.  If so, the reflog exists, otherwise it doesn't.
     	 */
-    	ret = reftable_iterator_next_log(&it, &log);
-    	if (ret < 0)
-    		goto done;
-    	if (ret > 0) {
-    		ret = 0;
-    		goto done;
+    	while (1) {
+    		ret = reftable_iterator_next_log(&it, &log);
+    		if (ret < 0)
+    			goto done;
+    		if (ret > 0) {
+    			ret = 0;
+    			goto done;
+    		}
+    		if (strcmp(log.refname, refname)) {
+    			ret = 0;
+    			goto done;
+    		}
+    		if (!reftable_log_record_is_deletion(&log))
+    			break;
     	}
-    	ret = strcmp(log.refname, refname) == 0;
+    	ret = 1;
     done:
     	reftable_iterator_destroy(&it);
@@ Expand Down Expand Up @@
     			ret = 0;
     			break;
     		}
+    		if (reftable_log_record_is_deletion(&log))
+    			continue;
     		tombstone.refname = (char *)arg->refname;
     		tombstone.value_type = REFTABLE_LOG_DELETION;
@@ Expand Down Expand Up @@
     			reftable_log_record_release(&log);
     			break;
     		}
+    		if (reftable_log_record_is_deletion(&log)) {
+    			reftable_log_record_release(&log);
+    			continue;
+    		}
     		oidread(&old_oid, log.value.update.old_hash,
     			ref_store->repo->hash_algo);
@@ Expand Down Expand Up @@
     		report.path = refname.buf;
     		switch (ref.value_type) {
+    		case REFTABLE_REF_DELETION:
+    			continue;
     		case REFTABLE_REF_VAL1:
     		case REFTABLE_REF_VAL2: {
     			struct object_id oid;
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

reftable: fix quadratic behavior when re-creating deleted refs #2166

Diff view

Diff view

There are no files selected for viewing

gitgitgadget Bot Jul 7, 2026

Uh oh!

gitgitgadget Bot Jul 7, 2026

Uh oh!

gitgitgadget Bot Jul 9, 2026

Uh oh!

gitgitgadget Bot Jul 9, 2026

Uh oh!

gitgitgadget Bot Jul 9, 2026

Uh oh!

gitgitgadget Bot Jul 10, 2026

Uh oh!

gitgitgadget Bot Jul 10, 2026

Uh oh!

gitgitgadget Bot Jul 7, 2026

Uh oh!

gitgitgadget Bot Jul 7, 2026

Uh oh!

gitgitgadget Bot Jul 8, 2026

Uh oh!

gitgitgadget Bot Jul 8, 2026

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up / @@ -42,6 +42,8 @@ struct reftable_stack_options { @@
     	 */
     	void (*on_reload)(void *payload);
     	void *on_reload_payload;
+    	int suppress_deletions;
     };
     /* open a new reftable stack. The tables along with the table list will be
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     	/* Update the stack to point to the new tables. */
     	if (st->merged)
     		reftable_merged_table_free(st->merged);
-    	new_merged->suppress_deletions = 1;
+    	new_merged->suppress_deletions = st->opts.suppress_deletions;
     	st->merged = new_merged;
     	if (st->tables)
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,46 @@
+    #!/bin/sh
+    test_description="Tests performance of ref operations with many tombstones"
+    . ./perf-lib.sh
+    test_expect_success "setup" '
+    	git init --ref-format=reftable repo &&
+    	blob=$(echo foo | git -C repo hash-object -w --stdin) &&
+    	for i in $(test_seq 8000)
+    	do
+    		printf "create refs/tags/tag-%d %s\n" "$i" "$blob" ||
+    		return 1
+    	done >repo/input &&
+    	git -C repo update-ref --stdin <repo/input &&
+    	git -C repo for-each-ref --format="delete %(refname)" |
+    	git -C repo update-ref --stdin
+    '
+    test_perf "recreate refs after mass delete" '
+    	git -C repo update-ref --stdin <repo/input &&
+    	git -C repo for-each-ref --format="delete %(refname)" |
+    	git -C repo update-ref --stdin
+    '
+    test_expect_success "setup asymmetric" '
+    	git init --ref-format=reftable repo2 &&
+    	blob=$(echo foo | git -C repo2 hash-object -w --stdin) &&
+    	for i in $(test_seq 8000)
+    	do
+    		printf "create refs/tags/old-%d %s\n" "$i" "$blob" ||
+    		return 1
+    	done >repo2/input-old &&
+    	sed "s/old-/new-/" <repo2/input-old >repo2/input-new &&
+    	git -C repo2 update-ref --stdin <repo2/input-old &&
+    	git -C repo2 for-each-ref --format="delete %(refname)" |
+    	git -C repo2 update-ref --stdin
+    '
+    test_perf "create new refs after deleting differently-named refs" '
+    	git -C repo2 update-ref --stdin <repo2/input-new &&
+    	git -C repo2 for-each-ref --format="delete %(refname)" refs/tags/ |
+    	git -C repo2 update-ref --stdin
+    '
+    test_done

Uh oh!

reftable: fix quadratic behavior when re-creating deleted refs #2166

Are you sure you want to change the base?

reftable: fix quadratic behavior when re-creating deleted refs #2166

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

gitgitgadget Bot Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!