From: Karthik Nayak Date: Sat, 15 Nov 2025 22:16:28 GMT Subject: Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed Message-ID: In-Reply-To: Junio C Hamano writes: > Karthik Nayak writes: > >> The cleanup section is reached with `retcode` set in several scenarios: >> >> - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set >> `retcode` before the transaction is created, so no commit is >> attempted. >> >> - `fetch_and_consume_refs()` and `backfill_tags()` are the primary >> cases this fix targets, both setting a positive `retcode` to >> trigger the committing of the transaction. >> >> This simplifies error handling and ensures future modifications to >> `do_fetch()` don't need special handling for batched updates. > > This may be sufficient to clean out the hanging transaction that the > original code forgot to commit, but in scenarios where this change > makes a difference, i.e., where the code does "goto cleanup" before > it calls commit_ref_transaction() in the main flow of the code, > there are things that are not performed that we may still want to > perform. Namely, we do not > > - call commit_fetch_head() > > - run set_upstream processing > > - honor do_set_head flag that was left for remote that does not > have followremotehead=never > > but don't we want to do some of them at least? > Thanks for bringing this up. I would think we should do all of these, but not if the '--atomic' flag is used. If the '--atomic' flag is used, we shouldn't do anything else and simply skip to the end. > If it turns out that we want to do all of them, I also wonder if the > resulting code would become easier to follow if we lose the call to > commit_ref_transaction() in the main code flow, do the above three > points before committing the ref transaction, and then after the > cleanup label, make a call to commit_ref_transaction() if we have an > open transaction and we are not atomic (regardless of the value of > retcode at that point). That call may yield another retcode that > the existing error reporting at the end of this function may have to > react to. > The issue is with '--atomic' again. I could think of this small change over this topic, which passes the fetch tests. diff --git a/builtin/fetch.c b/builtin/fetch.c index b19fa8e966..df11f59f56 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1890,7 +1890,7 @@ static int do_fetch(struct transport *transport, retcode = commit_ref_transaction(&transaction, atomic_fetch, transport->remote->name, &err); - if (retcode) + if (retcode && atomic_fetch) goto cleanup; commit_fetch_head(&fetch_head); I do wonder if we can cleanup this code, it is a bit messy right now. That said, we could either append this change as a new commit with some additional tests and re-roll the series or send it as a separate commit based on this series. I'd prefer the latter so that we have the fix for fetching tags merged sooner, but happy to do either. [snip]