Subject: Re: [PATCH v5 3/7] refs: support symrefs in 'reference-transaction' hook

From: Junio C Hamano <[email-masked]>
Date: Wed, 01 May 2024 16:05:30 -0700

Karthik Nayak <[email-masked]> writes:

> +		if (update->flags & REF_HAVE_OLD && update->old_target)

Although the precedence rule does not require it,

		if ((update->flags & REF_HAVE_OLD) && update_old_target)

is probably easier to read.

> +			strbuf_addf(&buf, "ref:%s ", update->old_target);
> +		else
> +			strbuf_addf(&buf, "%s ", oid_to_hex(&update->old_oid));

So the promise this code assumes is that .old_target member is
non-NULL if and only if the ref originally is a symbolic ref?

And if the "we do not care what the original value is, whether it is
a normal ref or a symbolic one" case, .old_oid would be all '\0' and
REF_HAVE_OLD bit is not set?

If we can write it like so:

	if (!(update->flags & REF_HAVE_OLD))
		strbuf_addf(&buf, "%s ", oid_to_hex(null_oid()));
	else if (update->old_target)
		strbuf_addf(&buf, "ref:%s ", update->old_target);
	else
		strbuf_addf(&buf, "ref:%s ", oid_to_hex(update->old_oid));

it may make the intent of the code a lot more clear.  If we are
operating in "!HAVE_OLD" mode, we show 0{40}.  Otherwise, old_target
is non-NULL when the thing is symbolic, and if old_target is NULL,
it is not symbolic and has its own value.

The same comment applies to the other side.

> +		if (update->flags & REF_HAVE_NEW && update->new_target)
> +			strbuf_addf(&buf, "ref:%s ", update->new_target);
> +		else
> +			strbuf_addf(&buf, "%s ", oid_to_hex(&update->new_oid));

> +		strbuf_addf(&buf, "%s\n", update->refname);
>  
>  		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
>  			if (errno != EPIPE) {