Junio C Hamano [off-list ref] writes:

Karthik Nayak [off-list ref] writes:

quoted

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]