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

From: Karthik Nayak <karthik.188@gmail.com>
Date: May 2, 2024, 05:32 UTC
Message-ID: <CAOLa=ZQrjYRq14KYsGASD+ua5fNNQCjnO0JbL4W7RTbXc1veJg@mail.gmail.com>
In-Reply-To: <xmqqy18tgmkl.fsf@gitster.g>

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

Show 10 quoted lines

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

Will add.

Show 7 quoted lines

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

Yes, for old_target this is correct. new_target could be set for a ref to convert it to 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?
>

Yup that's accurate.

Show 16 quoted lines

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

I see how it makes it clearer, but I think the intent with the existing code was clear too. I'll add this change to my local for the next version.

Show 10 quoted lines

>> +		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) {