From: Karthik Nayak Date: Wed, 19 Nov 2025 08:59:34 GMT Subject: Re: [PATCH v6 3/3] fetch: fix failed batched updates skipping operations Message-ID: In-Reply-To: Junio C Hamano writes: > Karthik Nayak 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.