Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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) {
Attachment:
signature.asc
Description: PGP signature
