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

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

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> The reference backends currently support transactional reference
>> updates. While this is exposed to users via 'git-update-ref' and its
>> '--stdin' mode, it is also used internally within various commands.
>>
>> However, we never supported transactional updates of symrefs. Let's add
>> support for symrefs in both the 'files' and the 'reftable' backend.
>>
>> Here, we add and use `ref_update_is_null_new_value()`, a helper function
>> which is used to check if there is a new_value in a reference update.
>
> I know you want to express a condition where you answer yes to "Is
> the new value specified in this ref update NULL?", but "is" at that
> position in the name somehow sounds awkward.  Any of
>
> 	ref_update_has_null_new_value
>         ref_update_with_no_new_value
>         ref_update_without_new_value
>
> might be nicer to ears.
>

Yes, thanks with this, I do agree that `ref_update_has_null_new_value`
sounds better.

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

>> @@ -1247,10 +1249,15 @@ struct ref_update *ref_transaction_add_update(
>>
>>  	update->flags = flags;
>>
>> -	if (flags & REF_HAVE_NEW)
>> +	if (new_target)
>> +		update->new_target = xstrdup(new_target);
>> +	if (old_target)
>> +		update->old_target = xstrdup(old_target);
>
> "Is the assumption that *update is 0-initialized?" was the first
> question that came to my mind.
>
> Doing an unconditional
>
> 	update->new_target = xstrdup_or_null(new_target);
> 	update->old_target = xstrdup_or_null(old_target);
>
> would convey the intention much more clearly without having readers
> guess the answer to the above question.
>

Right, I didn't catch the nuance last time, thanks for the explanation.

>> +	if (new_oid && flags & REF_HAVE_NEW)
>
> Even though syntactically not required,
>
> 	if (new_oid && (flags & REF_HAVE_NEW))
>
> or better yet
>
> 	if ((flags & REF_HAVE_NEW) && new_oid)
>
> would be easier to see.
>
>>  		oidcpy(&update->new_oid, new_oid);
>
> Again is the expectation that update->new_oid is initialized to
> all-0?  I am wondering if we want an else clause, i.e.
>
> 	if (!(flags & REF_HAVE_NEW))
> 		oidcpy(&update->new_oid, null_oid());
> 	else
> 		oidcpy(&update->new_oid, new_oid ? new_oid : null_oid());
>
> to clarify the intention of the code, since the way you wrote the
> consumer of thes two members and REF_HAVE_NEW bit in the previous
> step implied that the new_oid member gets used even when REF_HAVE_*
> bit is off, only for its null_oid() value.
>

Yes I understand what you're saying, but since we're doing a
`FLEX_ALLOC_MEM` right above this code, I thought the fact that the
'update' struct is 0 initialized is known. With this, and the fact that
here `update->new_oid` is a static variable, while `new_oid` is a
pointer. I think being too verbose is not required.

> I'll stop here for now.
>
> Thanks.
>

Thanks Junio for taking the time to review.

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