On Sun, Jul 24, 2016 at 09:37:57AM +0200, Johannes Schindelin wrote:
> On Sun, 24 Jul 2016, Eric Wong wrote:
>
> > @@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,
> > strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
> > line, linelen);
> > else {
> > - if (pp->fmt == CMIT_FMT_MBOXRD &&
> > - is_mboxrd_from(line, linelen))
> > - strbuf_addch(sb, '>');
> > + switch (pp->fmt) {
> > + case CMIT_FMT_EMAIL:
> > + if (is_from_line(line, linelen))
> > + strbuf_addch(sb, '>');
> > + break;
> > + case CMIT_FMT_MBOXRD:
> > + if (is_mboxrd_from(line, linelen))
> > + strbuf_addch(sb, '>');
> > + break;
> > + default:
> > + break;
> > + }
>
> Sorry to be nitpicking once again; I think this would be conciser (and
> easier to read at least for me) as:
>
> - if (pp->fmt == CMIT_FMT_MBOXRD &&
> - is_mboxrd_from(line, linelen))
> + if ((pp->fmt == CMIT_FMT_MBOXRD &&
> + is_mboxrd_from(line, linelen)) ||
> + (pp->fmt == CMIT_FMT_EMAIL &&
> + is_from_line(line, linelen)))
> strbuf_addch(sb, '>');
Since we are nitpicking, I think:
static int needs_from_quoting(enum cmit_fmt fmt,
const char *line, size_t len)
{
if (fmt == CMIT_FMT_MBOXRD && is_mboxrd_from(line, linelen))
return 1;
if (fmt == CMIT_FMT_EMAIL && is_from_line(line, linelen))
return 1;
return 0;
}
...
if (needs_from_quoting(pp->fmt, line, linelen))
strbuf_addch(sb, '>');
is even nicer. There are lots of alternatives to write the helper
function, and I do not care much which one is chosen. But splitting it
out into a concise "do we need to do this?" query function makes the
flow in pp_remainder much easier to follow.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
