Re: [PATCH v6 3/3] fetch: fix failed batched updates skipping operations

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> 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.
>
> 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.

Other than the usual "unless you care about timestamps, do not use
'touch' only to create a file" applies, but other than that the
added tests look quite sensible.

About the second new test piece, it is a bit surprising that we
didn't have test for --set-upstream on successful fetch.  It does
not need REFFILES prerequisite, does it?

Thanks.

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 4b113d7c27..f5c87d81fe 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1639,6 +1639,93 @@ 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 &&
> +		! test -f FETCH_HEAD &&
> +		git remote add origin ../base &&
> +		touch refs/heads/foo.lock &&
> +		test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
> +		test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err &&
> +		test -f FETCH_HEAD
> +	)
> +'
> +
> +test_expect_success REFFILES "upstream tracking info is added with --set-upstream" '
> +	test_when_finished rm -rf base repo &&
> +
> +	git init --initial-branch=main base &&
> +	test_commit -C base "updated" &&
> +
> +	git init --bare --initial-branch=main repo &&
> +	(
> +		cd repo &&
> +		git remote add origin ../base &&
> +		git fetch origin --set-upstream main &&
> +		git config get branch.main.remote >actual &&
> +		echo "origin" >expect &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success REFFILES "upstream tracking info is added even with conflicts" '
> +	test_when_finished rm -rf base repo &&
> +
> +	git init --initial-branch=main base &&
> +	test_commit -C base "updated" &&
> +
> +	git init --bare --initial-branch=main repo &&
> +	(
> +		cd repo &&
> +		git remote add origin ../base &&
> +		test_must_fail git config get branch.main.remote &&
> +
> +		mkdir -p refs/remotes/origin &&
> +		touch refs/remotes/origin/main.lock &&
> +		test_must_fail git fetch origin --set-upstream main &&
> +		git config get branch.main.remote >actual &&
> +		echo "origin" >expect &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success REFFILES "HEAD is updated even with conflicts" '
> +	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 &&
> +		git remote add origin ../base &&
> +
> +		! test -f refs/remotes/origin/HEAD &&
> +		mkdir -p refs/remotes/origin &&
> +		touch refs/remotes/origin/branch.lock &&
> +		test_must_fail git fetch origin &&
> +		test -f refs/remotes/origin/HEAD
> +	)
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd