From: Junio C Hamano Date: Thu, 13 Nov 2025 21:23:28 GMT Subject: Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed Message-ID: In-Reply-To: <20251113-fix-tags-not-fetching-v5-2-371ea7ec638d@gmail.com> 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? 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. > cleanup: > + /* > + * When using batched updates, we want to commit the non-rejected > + * updates and also handle the rejections. > + */ > + if (retcode && !atomic_fetch && transaction) > + commit_ref_transaction(&transaction, false, > + transport->remote->name, &err); > > if (retcode) { > if (err.len) { > error("%s", err.buf); IOW, something like this on top of this patch (not even compile tested). builtin/fetch.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git c/builtin/fetch.c w/builtin/fetch.c index b19fa8e966..d3eb65dac6 100644 --- c/builtin/fetch.c +++ w/builtin/fetch.c @@ -1888,11 +1888,6 @@ static int do_fetch(struct transport *transport, if (retcode) goto cleanup; - retcode = commit_ref_transaction(&transaction, atomic_fetch, - transport->remote->name, &err); - if (retcode) - goto cleanup; - commit_fetch_head(&fetch_head); if (set_upstream) { @@ -1957,14 +1952,9 @@ static int do_fetch(struct transport *transport, } cleanup: - /* - * When using batched updates, we want to commit the non-rejected - * updates and also handle the rejections. - */ - if (retcode && !atomic_fetch && transaction) - commit_ref_transaction(&transaction, false, - transport->remote->name, &err); - + if (!atomic_fetch && transaction) + retcode = commit_ref_transaction(&transaction, false, + transport->remote->name, &err); if (retcode) { if (err.len) { error("%s", err.buf);