Re: [PATCH v3 6/6] refs: skip hooks when deleting uncovered packed refs

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 13, 2022, 13:04 UTC
Message-ID: <220113.868rvjvkyn.gmgdl@evledraar.gmail.com>
In-Reply-To: <279eadc41cda9ceea4c5317d6a4c358c18d50ce9.1642054003.git.ps@pks.im>

On Thu, Jan 13 2022, Patrick Steinhardt wrote:

Show 8 quoted lines

> [[PGP Signed Part:Undecided]]
> When deleting refs from the loose-files refs backend, then we need to be
> careful to also delete the same ref from the packed refs backend, if it
> exists. If we don't, then deleting the loose ref would "uncover" the
> packed ref. We thus always have to queue up deletions of refs for both
> the loose and the packed refs backend. This is done in two separate
> transactions, where the end result is that the reference-transaction
> hook is executed twice for the deleted refs.

But do we (which would be an issue before this series) delete the loose and and then the packed one, thus racily exposing the stale ref to any concurrent repository reader, or do we first update the packed ref to the valu of the now-locked loose ref to avoid such a race?

Show 9 quoted lines

> [...]
> Fix this behaviour and don't execute the reference-transaction hook at
> all when refs in the packed-refs backend if it's driven by the files
> backend. This works as expected even in case the refs to be deleted only
> exist in the packed-refs backend because the loose-backend always queues
> refs in its own transaction even if they don't exist such that they can
> be locked for concurrent creation. And it also does the right thing in
> case neither of the backends has the ref because that would cause the
> transaction to fail completely.

I do wonder if the fundimental approach here is the right one. I.e. changing the hook to only expose "real" updates, as opposed to leaving it as a lower-level facility to listed in on any sort of ref updates.

In such a scenario we could imagine adding a third parameter or otherwise flag the update as "real" to the hook, so a dumber hook consumer could ignore the more verbose inter-transactional chatter.

I say that because this change does the right thing for the use-case you have in mind, but if you e.g. imagine a more gentle background-friendly "gc" such a thing could be implemented by backing off as soon as it sees an ongoing transaction being started.

With my ae35e16cd43 (reflog expire: don't lock reflogs using previously seen OID, 2021-08-23) not getting that more chatty data should be be OK for such a hypothetical hook.

But we might have more avoidable tripping over locks as the gc and ref transaction race one another to lock various things in the repository.

Or maybe nobody cares in practice, just food for thought.