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

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

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

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

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