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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 19, 2025, 23:08 UTC
Message-ID: <xmqqo6oxaae6.fsf@gitster.g>
In-Reply-To: <CAPig+cRjN85S3oCvazAvUD_V0EwkzdvKAm+DC66+uVijF5=HQA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 22 quoted lines

> On Wed, Nov 19, 2025 at 4:47 PM Karthik Nayak <karthik.188@gmail.com> 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 <karthik.188@gmail.com>
>> ---
>> 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.

Show 6 quoted lines

>
>> +               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?