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

[Junio C Hamano <gitster@pobox.com>]

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.

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

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

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

I'll stop here for now.

Thanks.