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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2021, 18:33 UTC
Message-ID: <xmqqwnw495yq.fsf@gitster.c.googlers.com>
In-Reply-To: <YAp0Y3rHty7itayo@ncase>

Patrick Steinhardt <ps@pks.im> writes:

Show 6 quoted lines

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

Show 7 quoted lines

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

Show 10 quoted lines

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