Re: [PATCH 2/2] format-patch: escape "From " lines recognized by mailsplit

From: Jeff King <peff@peff.net>
Date: Jul 24, 2016, 15:30 UTC
Message-ID: <20160724153035.GA32304@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1607240926500.14111@virtualbox>

On Sun, Jul 24, 2016 at 09:37:57AM +0200, Johannes Schindelin wrote:

Show 32 quoted lines

> 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