Patrick Steinhardt [off-list ref] writes:

On Sat, Nov 08, 2025 at 10:34:44PM +0100, Karthik Nayak wrote:

quoted

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 49e195199e..337ca2b0af 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -1963,6 +1963,14 @@ 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 > 0 && !atomic_fetch && transaction)
+		commit_ref_transaction(&transaction, false,
+				       transport->remote->name, &err);

I think this needs some explanation why this condition is safe. There's
quite a bunch of function calls and conditions that assign to it:

  - `truncate_fetch_head()` only ever assigns negative. This will be
    ignored as expected.

  - `open_fetch_head()` behaves likewise.

Also the transaction isn't even defined until this stage.

  - `prune_refs()` returns negative, but we then turn the return code
    into `1`. So we'd end up calling `commit_ref_transaction()` in this
    case, but we didn't in the previous iteration of this patch series.
    Was this intentional?

Its basically the same, before batched updates, we would return the
return code of `refs_delete_refs()` from within `prune_refs()`.

The fn `refs_delete_refs()` creates a transaction within to delete all
refs, this is done because we delete refs without old OID and hence they
wouldn't ever fail.

So now, when we pass our transaction to `prune_refs()`, it is also safe
to commit it.

One scenario I didn't think of earlier was that we would now enable
partial pruning with this change. But I would argue that this is
desirable since that is how we deal with other ref updates during fetch.

  - `fetch_and_consume_refs()` is one of the intended cases, and it sets
    up a positive retcode indeed.

  - `backfill_tags()` behaves likewise, and was intended.

So this looks good to me, with the only questionable one being
`prune_refs()`.

Patrick

Either ways, I would think that we should elaborate a little here.

Thanks,
Karthik