Re: [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed
From: Patrick Steinhardt <hidden>
Date: 2025-11-10 07:34:27
On Sat, Nov 08, 2025 at 10:34:44PM +0100, Karthik Nayak wrote:
quoted hunk ↗ jump to hunk
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.
- `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?
- `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