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

[Christian Couder <christian.couder@gmail.com>]

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.

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

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