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

Eric Sunshine [off-list ref] writes:

On Wed, Nov 19, 2025 at 4:47 PM Karthik Nayak [off-list ref] wrote:

quoted

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:
[...]
Signed-off-by: Karthik Nayak <redacted>
---

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh

@@ -1639,6 +1639,93 @@ test_expect_success "backfill tags when providing a refspec" '
+test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
+       test_when_finished rm -rf base repo &&
+ [...]
+       git init --bare repo &&
+       (
+               cd repo &&
+               ! test -f FETCH_HEAD &&

Is this supposed to be asserting that the file does not exist or that
the path is not a file? If the former, then test_path_is_missing()
would be a better choice.

Thanks for carefully reading.  Personally, I think this is not
needed, as we have just created a new repository.  It might be
even better to replace it with

		rm -f FETCH_HEAD &&

to clarify that we do want to see this _created_ with a failing "git
fetch", not merely left behind.

quoted

+               git remote add origin ../base &&
+               >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

More importantly, should we inspect the contents of this file to see
what gets recorded.  If we are fetching foo and bar, and we made foo
fail, do we expect foo and bar in the file?  Or do we expect only bar
in the file?  Something else?