Re: [PATCH RESEND] refs: Always pass old object name to reftx hook

From: Junio C Hamano <gitster@pobox.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, peff@peff.net, me@ttaylorr.com
Subject: Re: [PATCH RESEND] refs: Always pass old object name to reftx hook
Date: Fri, 22 Jan 2021 10:33:49 -0800	[thread overview]
Message-ID: <xmqqwnw495yq.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <YAp0Y3rHty7itayo@ncase> (Patrick Steinhardt's message of "Fri, 22 Jan 2021 07:44:51 +0100")

Patrick Steinhardt <ps@pks.im> writes:

>> Yes, it can mean both, but when you pretend to be that hook,
>> wouldn't you check if the ref exists?  If not, the user is trying to
>> create it, and otherwise, the user does not know or care what the
>> original value is, no?
>
> As long as you're aware as the script author, yes.

As you said downbelow, I agree that clear documentation may be
necessary.

> There is one gotcha though: you can verify the state when the
> reference-transaction hook gets invoked in the "prepared" state, as it
> means that all references have been locked and thus cannot be changed by
> any other well-behaved process accessing the git repository. In
> "committed" or "aborted" that's not true anymore, given that the state
> has changed already, so any locks have been released and it's impossible
> to find out what happened now.

True, but isn't the situation the same if we replaced the 0{40} old
side with (one version of) original value of the ref?

> different from the user-provided change. E.g.
>
>     0{40} <new> <ref>
>     ^<old>
>
> or
>
>     0{40}^<old> <new> <ref>
>
> That can be considered as backwards-incompatible though.

Yes, it is an incompatible change.  I thought of somehow annotating
the old side, e.g. "<old> <new> <ref>" vs "<OLD> <new> <ref>", to
show the distinction between "this is the original value of ref the
user wanted to see when updating <ref>" and "the user does not care
what value the <ref> gets updated from, but by the way, here is the
original value of the ref as Git sees it" [*], but I cannot think of
a way to do so without breaking existing readers.

    Side note: here, I am exploring the approach to replace 0{40}
    that is given when "do not care" into an actual original object
    name taken from the current state, like your patch did, but
    trying to find a way to make non-NULL object name distinguishable
    between the two cases (i.e. user-supplied vs system-filled).

That raises another question.  How much trust should the hook place
on the value of the <old> given to it?  When a non-NULL <old> value
is given by the end-user, does the hook get the value as-is, or do
we read the current value of the ref and send that as <old>?  Does
the transaction get rejected if the two are different and such a
record is not even given to the hook?

> Yup. Whatever we agree on, what is clear is that the documentation needs
> to be more specific here.

Yes, agreed.

Thanks.