Re: [PATCH] fetch: fix non-conflicting tags not being committed
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Nov 3, 2025, 20:52 UTC
- Message-ID
- <i3wzd6r2iohohj36fbipc2owrxkqzjni6aqwyv2gw7hb5kdg6b@y6fsmfvphpom>
- In-Reply-To
- <20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com>
On 25/11/03 02:49PM, Karthik Nayak wrote:
Show 11 quoted lines
> 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.
Show 25 quoted lines
> Add two tests to check for this regression. While here, add a missing > cleanup from previous test. > > Reported-by: David Bohman <debohman@gmail.com> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > 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?
Show 40 quoted lines
> +{
> + 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?
Show 36 quoted lines
> 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