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

[Jeff King]

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

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

[Johannes Schindelin]

Hi Eric,

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, '>');

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 1206c48..8fa3982 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1606,4 +1606,18 @@ test_expect_success 'format-patch --pretty=mboxrd' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'format-patch From escaping' '
> + cat >msg <<-INPUT_END &&
> + somebody pasted format-patch output into a body
> +
> + From  Mon Sep 17 00:00:00 2001
> + INPUT_END
> +
> + C=$(git commit-tree HEAD^^{tree} -p HEAD  + git format-patch --stdout -1 $C~1..$C >patch &&

Either "-1 $C" or "$C~1..$C", not both...

> + git grep -h --no-index \
> + ">From  " \
> + patch
> +'
> +
>  test_done
> -- 
> EW

Heh, that's a nice Git version ;-)

Ciao,
Dscho
--
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