Re: [PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2015, 19:13 UTC
Message-ID: <xmqqzj7028mn.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1427268105-16901-1-git-send-email-karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 13 quoted lines

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