From: Justin Tobler Date: Mon, 03 Nov 2025 20:52:04 GMT Subject: Re: [PATCH] fetch: fix non-conflicting tags not being committed Message-ID: In-Reply-To: <20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com> On 25/11/03 02:49PM, Karthik Nayak wrote: > The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19) > updated the 'git-fetch(1)' command to use batched updates. This batches > updates to gain performance improvements. When fetching references, each > update is added to the transaction. Finally, when committing, individual > updates are allowed to fail with reason, while the transaction itself > succeeds. > > One scenario which was missed here, was fetching tags. When fetching > conflicting tags, the `fetch_and_consume_refs()` function returns '1', > which skipped committing the transaction and directly jumped to the > cleanup section. This mean that no updates were applied. Ok so when fetching tags, if there is a reference conflict, we are bailing out without committing the transaction. In such cases, we actually want to handle the rejected reference updates and continue with the transaction. > Fix this by committing the transaction even when we have an error code. > This ensures other references are applied. Do this by extracting out the > transaction commit code into a new `commit_ref_transaction()` function > and using that. Makes sense. > Add two tests to check for this regression. While here, add a missing > cleanup from previous test. > > Reported-by: David Bohman > Signed-off-by: Karthik Nayak > --- > This fixes the bug reported by David Bohman [1]. > > [1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com > --- > builtin/fetch.c | 65 +++++++++++++++++++++++++++++++++----------------------- > t/t5510-fetch.sh | 41 +++++++++++++++++++++++++++++++++++ > 2 files changed, 79 insertions(+), 27 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index c7ff3480fb..8dea08dc74 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1686,6 +1686,38 @@ static void ref_transaction_rejection_handler(const char *refname, > *data->retcode = 1; > } > > +static int commit_ref_transaction(struct ref_transaction **transaction, > + bool is_atomic, const char *remote_name, > + struct strbuf *err) nit: I think `commit_ref_transaction()` here can easily be confused with `ref_transaction_commit()` and it's not exactly clear how they differ from the names alone. Maybe we could explain the additional responsibilities in a comment? > +{ > + int retcode = ref_transaction_commit(*transaction, err); > + if (retcode) { > + /* > + * Explicitly handle transaction cleanup to avoid > + * aborting an already closed transaction. > + */ > + ref_transaction_free(*transaction); > + *transaction = NULL; > + } > + > + if (*transaction && !is_atomic) { > + struct ref_rejection_data data = { > + .conflict_msg_shown = 0, > + .remote_name = remote_name, > + .retcode = &retcode, > + }; > + > + ref_transaction_for_each_rejected_update(*transaction, > + ref_transaction_rejection_handler, > + &data); > + > + ref_transaction_free(*transaction); > + *transaction = NULL; > + } > + > + return retcode; > +} > + > static int do_fetch(struct transport *transport, > struct refspec *rs, > const struct fetch_config *config) > @@ -1826,6 +1858,10 @@ static int do_fetch(struct transport *transport, > > if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map, > &fetch_head, config)) { > + /* As we're using batched updates, commit any pending updates. */ > + if (!atomic_fetch) > + commit_ref_transaction(&transaction, false, > + transport->remote->name, &err); IIUC, when we encounter an error via `fetch_and_consume_refs()` we now explicitly commit the transaction early and handle the rejected references. At first I wondered why we wouldn't just skip the "goto cleanup" in such cases, but I assume this is in part because we are trying to match the pre-batched updates behavior. Naive question: I noticed that `backfill_tags()` also invokes `fetch_and_consume_refs()`. Do we also need to commit pending updates there in case of reference conflicts? > retcode = 1; > goto cleanup; > } > @@ -1858,33 +1894,8 @@ static int do_fetch(struct transport *transport, > if (retcode) > goto cleanup; > > - retcode = ref_transaction_commit(transaction, &err); > - if (retcode) { > - /* > - * Explicitly handle transaction cleanup to avoid > - * aborting an already closed transaction. > - */ > - ref_transaction_free(transaction); > - transaction = NULL; > - goto cleanup; > - } > - > - if (!atomic_fetch) { > - struct ref_rejection_data data = { > - .retcode = &retcode, > - .conflict_msg_shown = 0, > - .remote_name = transport->remote->name, > - }; > - > - ref_transaction_for_each_rejected_update(transaction, > - ref_transaction_rejection_handler, > - &data); > - if (retcode) { > - ref_transaction_free(transaction); > - transaction = NULL; > - goto cleanup; > - } > - } > + retcode = commit_ref_transaction(&transaction, atomic_fetch, > + transport->remote->name, &err); This is where we would normally commit the reference transaction and handle rejected reference updates. Now we just reuse `commit_ref_transaction()`. Do we need to check the return value and potentially "goto cleanup" before proceeding? -Justin