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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Nov 19, 2025, 08:59 UTC
Message-ID: <CAOLa=ZQW56JVxa+tahrhk00eOmY2d8b1Lch_XFk4tNkummVPeg@mail.gmail.com>
In-Reply-To: <xmqqv7j7dxqy.fsf@gitster.g>

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

Show 31 quoted lines

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

Oops, will change that.

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

I was surprised too, that we don't have a test covering that flag. You're right it doesn't. The test for the conflict does, but the happy path doesn't. Will change.

>
> Thanks.
>

Thanks for the review.