Karthik Nayak [off-list ref] writes:
+ if ((flags & LOOKUP_LITERALLY)) {
+ if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, &hdrbuf) < 0)
+ status = error("unable to unpack %s header with --literally",
+ sha1_to_hex(sha1));
+ else if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)
+ status = error("unable to parse %s header", sha1_to_hex(sha1));
+ } else {
+ if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
+ status = error("unable to unpack %s header",
+ sha1_to_hex(sha1));
+ else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)
+ status = error("unable to parse %s header", sha1_to_hex(sha1));
+ }
I wonder if you can further reduce an unnecessary duplication in the
two "else if" clauses in the above, and if the result becomes easier
to read and maintain. Perhaps
if (((flags & LOOKUP_LITERALLY)
? unpack_sha1_header_to_strbuf(...)
: unpack_sha1_header(...)) < 0)
status = error(...);
else if ((status = parse_sha1_header_extended(...)) < 0)
status = error(...);
or even
status = 0;
if (flags & LOOKUP_LITERALLY) {
if (unpack_sha1_header_to_strbuf(...) < 0)
status = error(...);
} else {
if (unpack_sha1_header(...) < 0)
status = error(...);
}
if (!status) {
if (status = parse(...)) < 0)
status = error(...);
}
although I think the latter might be a bit harder to read.
On 03/26/2015 12:43 AM, Junio C Hamano wrote:
Karthik Nayak [off-list ref] writes:
quoted
+ if ((flags & LOOKUP_LITERALLY)) {
+ if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, &hdrbuf) < 0)
+ status = error("unable to unpack %s header with --literally",
+ sha1_to_hex(sha1));
+ else if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)
+ status = error("unable to parse %s header", sha1_to_hex(sha1));
+ } else {
+ if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
+ status = error("unable to unpack %s header",
+ sha1_to_hex(sha1));
+ else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)
+ status = error("unable to parse %s header", sha1_to_hex(sha1));
+ }
I wonder if you can further reduce an unnecessary duplication in the
two "else if" clauses in the above, and if the result becomes easier
to read and maintain. Perhaps
if (((flags & LOOKUP_LITERALLY)
? unpack_sha1_header_to_strbuf(...)
: unpack_sha1_header(...)) < 0)
status = error(...);
else if ((status = parse_sha1_header_extended(...)) < 0)
status = error(...);
or even
status = 0;
if (flags & LOOKUP_LITERALLY) {
if (unpack_sha1_header_to_strbuf(...) < 0)
status = error(...);
} else {
if (unpack_sha1_header(...) < 0)
status = error(...);
}
if (!status) {
if (status = parse(...)) < 0)
status = error(...);
}
although I think the latter might be a bit harder to read.
I hope you meant the former. The latter to me seems simpler as its a
simple if else statement whereas the former has a ternary operator with
function calls. I did think about this when writing the code, the
problem is when flag == LOOKUP_LITERALLY, parse_sha1_header_extended()
takes 'hdrbuf.buf' as first argument where as when flag !=
LOOKUP_LITERALLY, it takes 'hdr' as an argument. We could do this
status = 0;
char * hdrp;
if (flags & LOOKUP_LITERALLY) {
if (unpack_sha1_header_to_strbuf(...) < 0)
status = error(...);
hdrp = hdrbuf.buf;
} else {
if (unpack_sha1_header(...) < 0)
status = error(...);
hdrp = hdr;
}
if (!status)
if (status = parse(hdrp, ...)) < 0)
status = error(...);
}
But I think it just introduces another variable to keep track of, which
I rather not have.