Re: [PATCH v3] fetch: commit references fetched before backfilling tags
flat view
From: Justin Tobler <hidden>
Date: 2026-10-20 23:05:26
On 26/10/15 11:24PM, Karthik Nayak wrote:
In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the fetch code was modified to use batched updates to provide a good performance improvement. Batched updates are used both for the fetched references and to backfill tags. When using batched updates, the references aren't yet committed to disk when we start backfilling tags. This means in situations such as shallow fetching the negotiation during backfilling tags, the newly fetched refs are not available for advertisement in the 'haves' section. This can cause the server to send all objects the client already has.
Ok, without the earlier fetched references committed, the backfill fetch
may ultimately ask for objects it already got. Make sense.
The issue always existed when the '--atomic' flag was used, as both the regular ref updates and the tags need to be part of the same transaction.
So IIUC, you are saying that this problem already existed prior to 0e358de64a, but only for atomic fetches. Furthermore, this patch doesn't fix anything for atomic fetches as they can only use a single transaction.
Fix this by committing the previous batched update and initiating a new one for backfilling tags. Use a separate error variable for tracking the reference updates: One, it ensures that errors from the regular ref updates don't cause a total failure. Two, if starting the transaction for backfilling tags fails, only those updates are not made to disk. The initial error is now printed straight away to clear the buffer for further errors.
Naively, I think it would be nice if it were possible supplement the backfill fetch "haves" with the non-committed references in the pending transaction directly. That would avoid having to commit the references in the middle and probably also fix it for atomic fetches too, but I'm uncertain if that would be a simple or reasonable thing to do. Committing the intermediate state here does in part fix the issue, so it may be a good first step regardless.
quoted hunk ↗ jump to hunk
Add two tests, one which validates that the 'haves' are filled when backfilling tags. The other, to test that upstream is still set and tags are backfilled even if the main ref update is rejected. While this does make it a little slower than master, due to two transactions: Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = master) Time (mean ± σ): 82.1 ms ± 3.3 ms [User: 60.2 ms, System: 24.4 ms] Range (min … max): 77.4 ms … 89.4 ms 33 runs Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) Time (mean ± σ): 82.6 ms ± 2.4 ms [User: 59.3 ms, System: 25.2 ms] Range (min … max): 78.1 ms … 88.7 ms 31 runs Summary fetch: many refs (refformat = reftable, refcount = 10000, revision = master) ran 1.01 ± 0.05 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) It is still faster than not using batched updates: Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1) Time (mean ± σ): 1.468 s ± 0.041 s [User: 0.839 s, System: 0.587 s] Range (min … max): 1.427 s … 1.558 s 10 runs Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) Time (mean ± σ): 84.4 ms ± 1.7 ms [User: 60.9 ms, System: 25.8 ms] Range (min … max): 81.4 ms … 88.6 ms 29 runs Summary fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran 17.38 ± 0.61 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1) Reported-by: Mitja Bezenšek <redacted> Signed-off-by: Karthik Nayak <redacted> --- The issue was reported by Mitja Bezenšek on GitLab's Git repo [1]. [1]: https://gitlab.com/gitlab-org/git/-/work_items/799 --- builtin/fetch.c | 38 +++++++++++++++++++++++++++++++++++--- t/t5510-fetch.sh | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 3 deletions(-)diff --git a/builtin/fetch.c b/builtin/fetch.c index b2decc6cfd..34fc37b58a 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c@@ -1923,6 +1923,7 @@ static int do_fetch(struct transport *transport, struct strmap rejected_refs = STRMAP_INIT; int summary_width = 0; int follow_remote_head; + int commit_ref_ret = 0; if (tags == TAGS_DEFAULT) { if (transport->remote->fetch_tags == 2)@@ -2076,6 +2077,30 @@ static int do_fetch(struct transport *transport, struct ref *tags_ref_map = NULL, **tail = &tags_ref_map; find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail); + + /* + * As negotiation only reads references from disk, commit the references + * fetched so far, so that the backfill negotiation can advertise them as + * 'have's. Otherwise the server may re-send objects. + */ + if (tags_ref_map && !atomic_fetch) { + commit_ref_ret = commit_ref_transaction(&transaction, false, + transport->remote->name, + &rejected_refs, &err); + if (err.len) { + error("%s", err.buf); + strbuf_reset(&err); + }
It looks like we log the error and continue even if committing the
transaction failed. Is there a reason we would want to still try to
start the next transaction?
quoted hunk ↗ jump to hunk
+ + transaction = ref_store_transaction_begin(get_main_ref_store(the_repository), + REF_TRANSACTION_ALLOW_FAILURE, &err); + if (!transaction) { + free_refs(tags_ref_map); + tags_ref_map = NULL; + commit_ref_ret = -1; + } + } + if (tags_ref_map) { /* * If backfilling of tags fails then we want to tell@@ -2099,9 +2124,16 @@ static int do_fetch(struct transport *transport, if (verbosity >= 0) summary_width = transport_summary_width(ref_map); - retcode = commit_ref_transaction(&transaction, atomic_fetch, - transport->remote->name, - &rejected_refs, &err); + /* + * The transaction may have failed to initialize, meaning there is nothing + * to commit. Carry forward with other operations in such cases. + */ + if (transaction) + retcode = commit_ref_transaction(&transaction, atomic_fetch, + transport->remote->name, + &rejected_refs, &err); + retcode |= commit_ref_ret;
hmmm, the error paths here are confusing me a bit. If the transaction
failed to be created in the first place, wouldn't we want to stop
earlier on?
Thanks,
-Justin