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