From: Karthik Nayak <karthik.188@gmail.com>
Date: Fri, 21 Nov 2025 11:00:58 GMT
Subject: Re: [PATCH v7 3/3] fetch: fix failed batched updates skipping operations
Message-ID: <CAOLa=ZSQZhXEVGXzwg1bWd7En+vz8dzYHZTM+8AvW8UnDk-Fag@mail.gmail.com>
In-Reply-To: <xmqqo6oxaae6.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> 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.
>

That's fair.

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

I would say we should. Let me send in a version with these changes.

Thanks,
Karthik