From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 10 Nov 2025 07:34:15 GMT
Subject: Re: [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed
Message-ID: <aRGVd7L2DV4DNM-h@pks.im>
In-Reply-To: <20251108-fix-tags-not-fetching-v3-2-a12ab6c4daef@gmail.com>

On Sat, Nov 08, 2025 at 10:34:44PM +0100, Karthik Nayak wrote:
> 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