Re: [PATCH v2 0/7] update-ref: add symref oriented commands

From: Karthik Nayak <karthik.188@gmail.com>
Date: Apr 21, 2024, 19:11 UTC
Message-ID: <CAOLa=ZRsNHgWriZhiCxBfbqS2D6gSytnG9Hy3y2BMK2vV+x0Lw@mail.gmail.com>
In-Reply-To: <ZiNdvEhuhtH87Mw6@framework>

Patrick Steinhardt <ps@pks.im> writes:

Show 28 quoted lines

> On Fri, Apr 12, 2024 at 11:59:01AM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> The 'git-update-ref(1)' command allows transactional reference updates.
>> But currently only supports regular reference updates. Meaning, if one
>> wants to update HEAD (symbolic ref) in a transaction, there is no tool
>> to do so.
>>
>> One option to obtain transactional updates for the HEAD ref is to
>> manually create the HEAD.lock file and commit. This is intrusive, where
>> the user needs to mimic internal git behavior. Also, this only works
>> when using the files backend.
>>
>> At GitLab, we've been using the manual process till date, to allow users
>> to set and change their default branch. But with the introduction of
>> reftables as a reference backend, this becomes a necessity to be solved
>> within git.
>>
>> This patch series goes about introducing a set of commands
>> symref-{create,verify,delete,update} to work with symrefs complimenting
>> the existing commands for the regular refs within 'git-update-ref(1)'.
>
> One more thought crossed my mind this night: is it even necessary to
> introduce new commands for this? As far as I can see, it should be
> feasible to introduce symref support for all existing commands without
> breaking backwards compatibility. This can be achieved by using a prefix
> that cannot ever be an object ID, like for example "ref:".
>

I couldn't think of any reason that we can't do this. I think it would work. I'll try to patch something with my existing tests and see if breaks something.

Show 19 quoted lines

> Thus, all of the following should work (with 1234 being an OID and 0000
> being the null OID):
>
>     update HEAD ref:refs/heads/main ref:refs/heads/master
>     update HEAD ref:refs/heads/main 1234
>     update HEAD ref:refs/heads/main 0000
>     update HEAD 1234 ref:refs/heads/main
>     update HEAD 0000 ref:refs/heads/main
>     create HEAD ref:refs/heads/main
>     delete HEAD ref:refs/heads/main
>     verify HEAD ref:refs/heads/mains
>
> Parsing is unambiguous because we can use `starts_with("ref:")` for an
> otherwise invalid object ID. Furthermore, because refs cannot have
> spaces, we also don't have an issue with the SP separator.
>
> I have a hunch that this variant might lead to less code duplication,
> lead to less confusing behaviour and also makes for an easier user
> interface.

I'm certain that the duplication in update-ref.c will go away with this. Regarding other code changes, I think they'll stay the same.

But I do agree, this seems much nicer in terms of UX and also allows us to introduce `symref => regular ref` conversion in this series itself.