Re: [PATCH 7/8] refs: add 'update-symref' command to 'update-ref'

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> So perhaps we can say "update with a concrete <old-oid> will ensure
>>> that the <ref> poitns at <old-oid> before proceeding, but update
>>> with 0{40} as <old-oid> to ensure creation is deprecated.  update
>>> with 0{40} as <new-oid> as deletion is also deprecated.  Use create
>>> and delete for these two deprecated operation modes".
>>>
>>> This assumes that create and delete currently ensures that what is
>>> asked to be created does not exist, and what is asked to be deleted
>>> does exist, before the operation.  If we are loosely doing these two
>>> operations, then we cannot easily deprecate the checking-update,
>>> without breaking existing users.
>
> Note that I did not (and do not) know if "create" and "delete" have
> such checks; I expected somebody (other than me) to check before
> going forward.
>

>> But this still means we need to think of the best output for the
>> reference transaction hook (following commit).
>>
>> My current implementation of:
>>    <symref-target> SP <ref-name> LF
>> Should be changed to:
>>    <old-ref> SP <new-ref> LF
>>
>> But this means, for creation of symrefs <old-ref> needs to be "zero"
>> value. Also there is no way for clients to differentiate between regular
>> refs and symrefs here. I wonder if it makes sense to do something like:
>>
>>    symref SP <old-ref> SP <new-ref> LF
>
> What do clients see for a regular ref?  "<old-oid> SP <new-oid> LF"?
> That would be OK, as "symref" cannot be an object name, I guess?
>

>> Where symref is a fixed string at the start, used as a differentiator.
>> This however still would leave us to deal with "zero" values for
>> creation and deletion.
>
> Are these two <old-ref> and <new-ref> values optional in the context
> of your discussion?  The review comment was on input from the end-user
> that made it optional to validate the precondition, but this is what
> you produce as a result---if these are not optional, then an empty
> string can be a reasonable "zero" value.  I am confused...
>> Perhaps the best way here to actually be a lot more verbose and have the
>> hook output the following:
>>
>>    symref-create SP <new-ref> LF
>>    symref-delete SP <old-ref> LF
>>    symref-update SP <old-ref> SP <new-ref> LF
>>    symref-update-forced <new-ref> LF
>
> It is unfortunate that the input to the hook for a normal reference
> update uses syntax different from the "--stdin" input format, i.e.,
>
>     <old-oid> SP <new-oid> SP <ref> LF
>
> but it is way too late to change it now.  So to be consistent,
>
>     symref-create SP <new-ref> SP <ref> LF
>     symref-delete SP <old-ref> SP <ref> LF
>     symref-update SP <old-ref> SP <new-ref> SP <ref> LF
>
> would be the three operations.
>
> But this is not an end-user input that tells Git "I do not care
> about precondition, I did not even bother to learn the current state
> to give you as <old-something>, just force it".  The input to hook
> is what we tell the hook what we are planning to do (so that it can
> decline), and we do not need the ability to say "I do not know what
> the current state is".  So I do not think you need any "zero" value
> in the input to the reference-transaction hook.  And I do not see a
> need for the "symref-update-forced" variant, either.

>> But this is not an end-user input that tells Git "I do not care
>> about precondition, I did not even bother to learn the current state
>> to give you as <old-something>, just force it".  The input to hook
>> is what we tell the hook what we are planning to do (so that it can
>> decline), and we do not need the ability to say "I do not know what
>> the current state is".  So I do not think you need any "zero" value
>> in the input to the reference-transaction hook.  And I do not see a
>> need for the "symref-update-forced" variant, either.
>
> I misspoke here.  We do need "zero" value to indicate that "this
> update is a creation event" and "this update is a deletion event".
> What I meant to say is that there is no need to make the "zero"
> value distinguishable from a "missing optional" value, which was a
> problem on the "--stdin" side with "-z" format, where each command
> is in a format with fixed number of parameters, unlike the textual
> format, where a missing optional argument can be expressed by
> omitting SP before the value and the value itself and it can be
> differentiated from an empty string as an optional value that is not
> missing.
>
> Thanks.

> What I suspect that needs more thought is what should happen when
> you request via "--stdin" to create, update, or delete a symref,
> but <ref> is a regular ref, e.g., "symref-delete <ref>".  For
> "symref-create <ref> <new-ref>", we would fail if <ref> exists,
> whether it is a symref or a normal ref, so that is OK.  For
> "symref-delete <ref> <old-ref>", we would fail if <ref> is *not*
> a symref to <old-ref>, so the case where <ref> is a normal ref
> is already covered.
>
> Should we support "symref-update <ref> <new-ref> <old-oid>" that
> makes <ref> a symref that points at <new-ref>, but ensures that
> <ref> before the operation is a normal ref that points at <old-oid>?
>
> Or should "symref-update" work only on <ref> that is an existing
> symref?
>
> I think I am OK if the answer was "You can only give a precondition
> in the form of <old-ref>, which means you can only turn an existing
> symref to point at a different ref with precondition protection. If
> you want to turn a normal ref into a symref, you have to force it by
> not having a precondition, or delete the ref and then (re)create it
> as a symref".  But we need to decide the semantics and document it.

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> But this still means we need to think of the best output for the
>>> reference transaction hook (following commit).
>>> ...
>
> One thing I missed.  We are not currently reporting symref updates
> to these hooks.  Are they prepared to take such extra input?  If not,
> are they going to barf when they see "symref-update" while expecting
> to see <old-oid>?
>
> We may need to make it possible for Git to tell which variant of the
> hook script it was given somehow (the easiest cop-out is to introduce
> ref-transaction-hook-v2 as a separate hook, and use it if exists, or
> fall back to the reference-transaction hook, and report symref updates
> only when we are using v2, but there may be better approaches).
>