Re: [PATCH 1/1] Add preparing state to reference-transaction hook

[Junio C Hamano <gitster@pobox.com>]

eric.peijian@gmail.com writes:

> From: Eric Ju <eric.peijian@gmail.com>
>
> From: Eric Ju <eju@gitlab.com>

This is curious.  The former matches the sign-off, but I somehow
suspect that the @gitlab.com identity may be what you want to use
for both of them, if this is a company sponsored work by an
employee?  I dunno.

Also the commit title deviates from the established "<area>: <what
is done>" format.

    Subject: [PATCH] refs: add 'preparing" phase to the transaction hook

or something?

Other than that, both the cover letter and the proposed log message
very well explain the motivation behind the new feature.  I wish
everybody wrote their log messages as clearly as this one.

> The "reference-transaction" hook is invoked multiple times during a ref
> transaction. Each invocation corresponds to a different phase:
>
> - The "prepared" phase indicates that references have been locked.
> - The "commit" phase indicates that all updates have been written to disk.
> - The "abort" phase indicates that the transaction has been aborted and that
>   all changes have been rolled back.

"commit" -> "committed" and "abort" -> "aborted", if the existing
documentation is to be trusted.

> This hook can be used to learn about the updates that Git wants to perform.
> For example, forges use it to coordinate reference updates across multiple
> nodes.
>
> However, the phases are insufficient for some specific use cases. The earliest
> observable phase in the "reference-transaction" hook is "prepared", at which
> point Git has already taken exclusive locks on every affected reference. This
> makes it suitable for last-chance validation, but not for serialization. So by
> the time a hook sees the "prepared" phase, it has no way to defer locking, and
> thus it cannot rearrange multiple concurrent ref transactions relative to one
> another.

I cannot quite picture how "rearrangement" would happen, though.

Would the hook notice "ah there is a preparing hook invocation
incoming", stall the caller by not immediately returning and instead
wait for a different Git process to invoke the same ref-transaction
hook "preparing" invocation, and somehow decide to let the latter go
first before releasing the former?

> Introduce a new "preparing" phase that runs before the "prepared" phase, that
> is before Git acquires any reference lock on disk. This gives callers a
> well-defined window to perform validation, enable higher-level ordering of
> concurrent transactions, or reject the transaction entirely, all without
> interfering with the locking state.
>
> This change is strictly speaking not backwards compatible. Existing hook
> scripts that do not know to handle unknown phases handle the "preparing" state

"know to handle unknown phrases handle"?

> string will encounter an unknown phase, and that might cause them to return an
> error now. But the hook is considered to expose internal implementation details
> of how Git works, and as such we have been a bit more lenient with changing its
> exact semantics, like for example in a8ae923f85 (refs: support symrefs in
> 'reference-transaction' hook, 2024-05-07).
>
> An alternative would be to introduce a "reference-transaction-v2" hook that
> knows about the new phase. This feels like a rather heavy-weight option though,
> and was thus discarded.

And documenting the design alternatives and decision like these two
paragraphs is very much appreciated.

The insertion of a new hook invocation itself is at a very much
expected place in the code path.  Well written.

Will queue.  Thanks.