From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 01 Dec 2025 12:58:19 GMT
Subject: Re: [PATCH v8 3/3] fetch: fix failed batched updates skipping operations
Message-ID: <aS2Q6y_hnwBxycGk@pks.im>
In-Reply-To: <20251121-fix-tags-not-fetching-v8-3-23b53a8a8334@gmail.com>

On Fri, Nov 21, 2025 at 12:13:47PM +0100, Karthik Nayak wrote:
> Fix a regression introduced with batched updates in 0e358de64a (fetch:
> use batched reference updates, 2025-05-19) when fetching references. In
> the `do_fetch()` function, we jump to cleanup if committing the
> transaction fails, regardless of whether using batched or atomic
> updates. This skips three subsequent operations:
> 
>   - Update 'FETCH_HEAD' as part of `commit_fetch_head()`.
> 
>   - Add upstream tracking information via `set_upstream()`.
> 
>   - Setting remote 'HEAD' values when `do_set_head` is true.
> 
> For atomic updates, this is expected behavior. For batched updates,
> we want to continue with these operations even if some refs fail to
> update.
> 
> Skipping `commit_fetch_head()` isn't actually a regression because
> 'FETCH_HEAD' is already updated via `append_fetch_head()` when not
> using '--atomic'. However, we add a test to validate this behavior.

This raises the question what happens when this function _does_ get
executed again. But we're guarding us:

    static void commit_fetch_head(struct fetch_head *fetch_head)
    {
        if (!fetch_head->fp || !atomic_fetch)
            return;
        strbuf_write(&fetch_head->buf, fetch_head->fp);
    }

And as we only `goto cleanup` in case `retcode && atomic_fetch` we know
that the above function will exit early. So this is a no-op change
indeed.

> Skipping the other two operations (upstream tracking and remote HEAD)
> is a regression. Fix this by only jumping to cleanup when using
> '--atomic', allowing batched updates to continue with post-fetch
> operations. Add tests to prevent future regressions.

Makes sense.

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 4b113d7c27..a1ca4e1ac7 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1639,6 +1639,94 @@ test_expect_success "backfill tags when providing a refspec" '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
> +	test_when_finished rm -rf base repo &&
> +
> +	git init base &&
> +	(
> +		cd base &&
> +		test_commit "updated" &&
> +
> +		git update-ref refs/heads/foo @ &&
> +		git update-ref refs/heads/branch @
> +	) &&
> +
> +	git init --bare repo &&
> +	(
> +		cd repo &&
> +		rm -f FETCH_HEAD &&
> +		git remote add origin ../base &&
> +		>refs/heads/foo.lock &&

Hm. Is this compatible with all supported systems? We typically write
this as:

    : >refs/heads/foo.lock

But I have to acknowledge that I only do this because some people that
are more knowledgeable than I am know that we need this.

Other than that I'm happy with the current state of this patch series.
If the above turns out to be a non-issue I think it should be ready for
'next'.

Thanks!

Patrick