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

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2024, 23:05 UTC
Message-ID: <xmqqy18tgmkl.fsf@gitster.g>
In-Reply-To: <20240501202229.2695774-4-knayak@gitlab.com>

Karthik Nayak <karthik.188@gmail.com> 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) {