Re: [PATCH v5 4/7] refs: add support for transactional symref updates

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3298 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, May 02, 2024 at 05:50:47AM +0000, Karthik Nayak wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> > Karthik Nayak <karthik.188@gmail.com> writes:
>> >> From: Karthik Nayak <karthik.188@gmail.com>
>> >> We do not add reflog for dangling symref updates, because currently
>> >> 'git-symbolic-ref' doesn't add reflog for dangling symref updates and it
>> >> would be best to keep this behavior consistent as we would move it to
>> >> start using transaction based updates in the following commit.
>> >
>> > If we are not changing the behaviour, does it deserve a four-line
>> > paragraph?  It is not like we describe every no changes (i.e. "we
>> > could break the behaviour by introducing this and that bugs, but we
>> > did not" is not something we usually say in proposed log messages).
>> >
>> > At most, if you want to highlight that behaviour, I would expect a
>> > brief mention like:
>> >
>> >     Note that a dangling symref update does not record a new reflog
>> >     entry, which is unchanged before and after this commit.
>> >
>> > As a reflog entry records name of the object that is pointed by the
>> > ref (either directly or indirectly) before and after an operation,
>> > an operation that involve a dangling reflog that does not point at
>> > any object cannot be expressed in a reflog, no?  It is way too late
>> > to change this, but it would have been interesting if the design of
>> > reflog had a room to log the change of symbolic ref target as well
>> > as object names.  It would have allowed us to say "HEAD at time T
>> > pointed at refs/heads/main (which did not exist)", "HEAD at time T+1
>> > directly pointed at commit X (detached)", "HEAD at time T+2 pointed
>> > at refs/heads/next", etc. and allowed us to much more cleanly
>> > support "previous branch".
>> >
>>
>> While I agree that four lines may seem excessive, I think it is indeed
>> an important point to note. Mostly because this shows that when doing
>> dangling symref updates, there is no record of this update. The best
>> situation would be like you mentioned, to record the symref target
>> changes. But even with the current design, it would have been nice to at
>> least acknowledge that there was some update done to the symref. By
>> having zero-oid for the new and old value in the reflog. But seems like
>> we can't do that either.
>
> I wouldn't say we can't do that. We already do log when symrefs become
> dangling when updating references via HEAD by logging a zero OID as new
> OID. That is, if we have "HEAD -> refs/heads/foo" and you delete the
> latter, then we create a new reflog message for "HEAD" with zero OID as
> new OID.
>
> I would claim that the current behaviour where we don't create a reflog
> entry when updating a ref to become dangling is a mere bug. I think it's
> fair to declare this a #leftoverbit and handle it in a follow-up patch
> series. But it would be nice to say so in an in-code comment.
>

I think _can't_ wasn't the best terminology. My previous series actually
added a reflog, but I noticed a bunch of tests were failing and I think
it made sense to keep the existing behaviour.

But addressing it as a bug would definitely be a good way to go and fix
this, I'll add a comment in the code for now.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]