Re: [PATCH 5/6] fetch: make `--atomic` flag cover backfilling of tags

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 17, 2022, 11:37 UTC
Message-ID: <Yg4ze+TMUWEQmszJ@ncase>
In-Reply-To: <CAP8UFD1bdLESqzbZcYKYfib836vrDTfyCmYfT-9B-1ToJB0EWg@mail.gmail.com>

On Tue, Feb 15, 2022 at 09:11:55AM +0100, Christian Couder wrote:

Show 16 quoted lines

> On Mon, Feb 14, 2022 at 10:13 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > When fetching references from a remote we by default also fetch all tags
> > which point into the history we have fetched. This is a separate step
> > performed after updating local references because it requires us to walk
> > over the history on the client-side to determine whether the remote has
> > announced any tags which point to one of the fetched commits.
> >
> > This backfilling of tags isn't covered by the `--atomic` flag: right
> > now, it only applies to the step where we update our local references.
> > This is an oversight at the time the flag was introduced: its purpose is
> > to either update all references or none, but right now we happily update
> > local references even in the case where backfilling failed.
> 
> Also it looks like the backfilling of tags itself isn't atomic, right?
> Some tags could be backfilled while others aren't.

Right.

Show 7 quoted lines

> > Fix this by pulling up creation of the reference transaction such that
> > we can pass the same transaction to both the code which updates local
> > references and to the code which backfills tags. This allows us to only
> > commit the transaction in case both actions succeed.
> 
> Maybe this could be seen as a regression by users who are mostly
> interested in the local references though.

Even though the commit message discern "local references" and "backfilled tags", ultimately they're the same. Both are references that end up in your local refdb, so from the point of the user there is no real difference here. Documentation of the `--atomic` flag only says that "either all refs are updared, or on error, no refs are updated". I think that the current behaviour does not fit the description.

Show 21 quoted lines

> > Note that we also have to start passing the transaction into
> > `find_non_local_tags()`: this function is responsible for finding all
> > tags which we need to backfill. Right now, it will happily return tags
> > which we have already been updated with our local references. But when
> 
> s/we have/have/
> 
> > we use a single transaction for both local references and backfilling
> > then it may happen that we try to queue the same reference update twice
> > to the transaction, which consequentially triggers a bug. We thus have
> 
> s/consequentially/consequently/
> 
> > to skip over any tags which have already been queued. Unfortunately,
> > this requires us to reach into internals of the reference transaction to
> > access queued updates, but there is no non-internal interface right now
> > which would allow us to access this information.
> 
> This makes me wonder if such a non-internal interface should be
> implemented first. Or if some function to queue a reference update
> could check if the same reference update has already been queued.

Yeah. I noted that ommission in the cover letter already, but didn't yet want to fix that before getting some initial feedback. I'll add something like a `for_each_queued_reference_update()` in v2 of this series though.

Patrick