From: karthik nayak <karthik.188@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, sunshine@sunshineco.com
Subject: Re: [PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type
Date: Thu, 26 Mar 2015 01:50:21 +0530 [thread overview]
Message-ID: <55131885.5000706@gmail.com> (raw)
In-Reply-To: <xmqqzj7028mn.fsf@gitster.dls.corp.google.com>
On 03/26/2015 12:43 AM, Junio C Hamano wrote:
> 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.
next prev parent reply other threads:[~2015-03-25 20:20 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-25 7:19 [PATCH v5 0/2] cat-file: add a '--literally' option karthik nayak
2015-03-25 7:21 ` [PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type Karthik Nayak
2015-03-25 19:13 ` Junio C Hamano
2015-03-25 20:20 ` karthik nayak [this message]
2015-03-25 20:32 ` Junio C Hamano
2015-03-25 19:27 ` Junio C Hamano
2015-03-25 20:22 ` karthik nayak
2015-03-25 7:22 ` [PATCH v5 2/2] cat-file: teach cat-file a '--literally' option Karthik Nayak
2015-03-25 7:42 ` Eric Sunshine
2015-03-25 7:48 ` karthik nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55131885.5000706@gmail.com \
--to=karthik.188@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.