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

From: karthik nayak <karthik.188@gmail.com>
Date: Mar 25, 2015, 20:20 UTC
Message-ID: <55131885.5000706@gmail.com>
In-Reply-To: <xmqqzj7028mn.fsf@gitster.dls.corp.google.com>

On 03/26/2015 12:43 AM, Junio C Hamano wrote:

Show 44 quoted lines

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

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.