From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 25 Mar 2015 19:13:20 GMT
Subject: Re: [PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type
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:

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