From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 22 May 2024 07:59:30 GMT
Subject: Re: [PATCH 6/6] update-ref: add support for 'symref-update' command
Message-ID: <CAOLa=ZRNNBOzsw+EzEJVnAU0ZsJ-bcTpciExdsQz2iDatYkqNQ@mail.gmail.com>
In-Reply-To: <CAOLa=ZQW-cCV5BP_fCvuZimfkjwAzjEiqXYRPft1Wf9kAX=_bw@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> On Tue, May 14, 2024 at 02:44:11PM +0200, Karthik Nayak wrote:
>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>
>>> Add 'symref-update' command to the '--stdin' mode of 'git-update-ref' to
>>> allow updates of symbolic refs. The 'symref-update' command takes in a
>>> <new-target>, which the <ref> will be updated to. If the <ref> doesn't
>>> exist it will be created.
>>>
>>> It also optionally takes either an `ref <old-target>` or `oid
>>> <old-oid>`. If the <old-target> is provided, it checks to see if the
>>> <ref> targets the <old-target> before the update. If <old-oid> is provided
>>> it checks <ref> to ensure that it is a regular ref and <old-oid> is the
>>> OID before the update. This by extension also means that this when a
>>> zero <old-oid> is provided, it ensures that the ref didn't exist before.
>>
>> It's somewhat unfortunate that the syntax diverges from the "update"
>> command. "update" also has essentially the same issue now, that we
>> cannot verify that its old value is a symref, right? Can we fix that in
>> a backwards compatible way?
>>
>
> I think Peff mentioned [1] of a way. So we convert the existing
>
>     update SP <ref> SP <newvalue> [SP <oldvalue>] LF
>     update SP <ref> NUL <newvalue> NUL [<oldvalue>] NUL // -z
>
> to
>
>     update SP <ref> SP <newvalue> [SP (<oldvalue> | ref <old_target>)] LF
>     update SP <ref> NUL <newvalue> NUL [(<oldvalue> | ref NUL <old_target>)] NUL // -z
>
> this should work, I think. I will play around this and add it in. Please
> let me know if you can think of a scenario where this breaks.

Thinking about this more, I'll actually do this in a follow up series. I
want to cleanup the existing tests already to move them to the loop that
I introduced in this series. That would make it easier to test and
ensure that we can port the existing 'update' command to also support
`old_target`.

[snip]