On Mon, Feb 14, 2022 at 10:13 PM Patrick Steinhardt <ps@xxxxxx> 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.
