From: Junio C Hamano Date: Wed, 19 Nov 2025 23:08:17 GMT Subject: Re: [PATCH v7 3/3] fetch: fix failed batched updates skipping operations Message-ID: In-Reply-To: Eric Sunshine writes: > On Wed, Nov 19, 2025 at 4:47 PM 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: >> [...] >> Signed-off-by: Karthik Nayak >> --- >> 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. > >> + 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?