RFC patch, 3 partssupport mboxrd format

> This output format prevents format-patch output from breaking
> readers if somebody copy+pasted an mbox into a commit message.
>
> Unlike the traditional "mboxo" format, "mboxrd" is designed to
> be fully-reversible.  "mboxrd" also gracefully degrades to
> showing extra ">" in existing "mboxo" readers.
> [...]
> Signed-off-by: Eric Wong <e@80x24.org>
> ---
> diff --git a/pretty.c b/pretty.c
> @@ -1697,12 +1699,34 @@ static void pp_handle_indent(struct pretty_print_context *pp,
> +static regex_t *mboxrd_prepare(void)
> +{
> +       static regex_t preg;
> +       const char re[] = "^>*From ";
> +       int err = regcomp(&preg, re, REG_NOSUB | REG_EXTENDED);
> +[...]
> +       return &preg;
> +}
> +
>  void pp_remainder(struct pretty_print_context *pp,
>                   const char **msg_p,
>                   struct strbuf *sb,
>                   int indent)
>  {
> +       static regex_t *mboxrd_from;
> +
> +       if (pp->fmt == CMIT_FMT_MBOXRD && !mboxrd_from)
> +               mboxrd_from = mboxrd_prepare();
> +
> @@ -1725,8 +1749,13 @@ void pp_remainder(struct pretty_print_context *pp,
>                 else if (pp->expand_tabs_in_log)
>                         strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
>                                              line, linelen);
> -               else
> +               else {
> +                       if (pp->fmt == CMIT_FMT_MBOXRD &&
> +                                       !regexec(mboxrd_from, line, 0, 0, 0))
> +                               strbuf_addch(sb, '>');

> +
>                         strbuf_add(sb, line, linelen);
> +               }
>                 strbuf_addch(sb, '\n');
>         }
>  }
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> @@ -1565,4 +1565,31 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
> +test_expect_success 'format-patch --pretty=mboxrd' '
> +       cat >msg <<-INPUT_END &&

> +       mboxrd should escape the body
> +
> +       From could trip up a loose mbox parser
> +       >From extra escape for reversibility
> +       >>From extra escape for reversibility 2
> +       from lower case not escaped
> +       Fromm bad speling not escaped
> +        From with leading space not escaped
> +       INPUT_END
> +
> +       cat >expect <<-INPUT_END &&
> +       >From could trip up a loose mbox parser
> +       >>From extra escape for reversibility
> +       >>>From extra escape for reversibility 2
> +       from lower case not escaped
> +       Fromm bad speling not escaped
> +        From with leading space not escaped
> +       INPUT_END
> +
> +       C=$(git commit-tree HEAD^^{tree} -p HEAD <msg) &&
> +       git format-patch --pretty=mboxrd --stdout -1 $C~1..$C >patch &&
> +       grep -A5 "^>From could trip up a loose mbox parser" patch >actual &&

> On Mon, May 30, 2016 at 7:21 PM, Eric Wong <e@80x24.org> wrote:
> > diff --git a/pretty.c b/pretty.c
> > @@ -1697,12 +1699,34 @@ static void pp_handle_indent(struct pretty_print_context *pp,
> > +static regex_t *mboxrd_prepare(void)
> > +{
> > +       static regex_t preg;
> > +       const char re[] = "^>*From ";
> > +       int err = regcomp(&preg, re, REG_NOSUB | REG_EXTENDED);
> > +[...]
> > +       return &preg;
> > +}
> > +
> >  void pp_remainder(struct pretty_print_context *pp,
> >                   const char **msg_p,
> >                   struct strbuf *sb,
> >                   int indent)
> >  {
> > +       static regex_t *mboxrd_from;
> > +
> > +       if (pp->fmt == CMIT_FMT_MBOXRD && !mboxrd_from)
> > +               mboxrd_from = mboxrd_prepare();
> > +
> > @@ -1725,8 +1749,13 @@ void pp_remainder(struct pretty_print_context *pp,
> >                 else if (pp->expand_tabs_in_log)
> >                         strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
> >                                              line, linelen);
> > -               else
> > +               else {
> > +                       if (pp->fmt == CMIT_FMT_MBOXRD &&
> > +                                       !regexec(mboxrd_from, line, 0, 0, 0))
> > +                               strbuf_addch(sb, '>');
> 
> At first glance, this seems dangerous since it's handing 'line' to
> regexec() without paying attention to 'linelen'. For an arbitrary
> regex, this could result in erroneous matches on subsequent "lines",
> however, since this expression is anchored with '^', it's not a
> problem. But, it is a bit subtle.

> I wonder if hand-coding, rather than using a regex, could be an improvement:
> 
>     static int is_mboxrd_from(const char *s, size_t n)
>     {
>         size_t f = strlen("From ");
>         const char *t = s + n;
> 
>         while (s < t && *s == '>')
>             s++;
>         return t - s >= f && !memcmp(s, "From ", f);
>     }
> 
> or something.

> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > @@ -1565,4 +1565,31 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
> > +test_expect_success 'format-patch --pretty=mboxrd' '
> > +       cat >msg <<-INPUT_END &&
> 
> Maybe use <<-\INPUT_END to indicate that no variable interpolation is
> expected. Ditto below.

> Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, May 30, 2016 at 7:21 PM, Eric Wong <e@80x24.org> wrote:
>> > +                       if (pp->fmt == CMIT_FMT_MBOXRD &&
>> > +                                       !regexec(mboxrd_from, line, 0, 0, 0))
>> > +                               strbuf_addch(sb, '>');
>>
>> At first glance, this seems dangerous since it's handing 'line' to
>> regexec() without paying attention to 'linelen'. For an arbitrary
>> regex, this could result in erroneous matches on subsequent "lines",
>> however, since this expression is anchored with '^', it's not a
>> problem. But, it is a bit subtle.
>
> Maybe having more context of the pp_remainder function and
> seeing the get_one_line call would've helped in the diff;
> I didn't think of this issue once I figured out where to
> place the change.

>> I wonder if hand-coding, rather than using a regex, could be an improvement:
>>
>>     static int is_mboxrd_from(const char *s, size_t n)
>>     {
>>         size_t f = strlen("From ");
>>         const char *t = s + n;
>>
>>         while (s < t && *s == '>')
>>             s++;
>>         return t - s >= f && !memcmp(s, "From ", f);
>>     }
>>
>> or something.
>
> Yikes.  I mostly work in high-level languages and do my best to
> avoid string parsing in C; so that scares me.  A lot.

> >> I wonder if hand-coding, rather than using a regex, could be an improvement:
> >>
> >>     static int is_mboxrd_from(const char *s, size_t n)
> >>     {
> >>         size_t f = strlen("From ");
> >>         const char *t = s + n;
> >>
> >>         while (s < t && *s == '>')
> >>             s++;
> >>         return t - s >= f && !memcmp(s, "From ", f);
> >>     }
> >>
> >> or something.
> >
> > Yikes.  I mostly work in high-level languages and do my best to
> > avoid string parsing in C; so that scares me.  A lot.
> 
> The hand-coded is_mboxrd_from() above is pretty much idiomatic C and
> (I think) typical of how such a function would be coded in Git itself,
> so it looks normal and easy to grok to me (but, of course, I'm
> probably biased since I wrote it).
> 
> > I admit a regex isn't necessary, but I'm wondering if the above
> > could be made less frightening to someone like me.
> 
> Perhaps, but it's difficult to say without knowing how it frightens you.

> > Maybe extra test cases + valgrind can quell my fears :)
> 
> I can envision tests such as:
> 
>     ""
>     "F"
>     "From"
>     "From "
>     "From     "
>     "From foobar"
> 
> and so on, if that's what you mean.

> Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, May 31, 2016 at 3:45 AM, Eric Wong <e@80x24.org> wrote:
>> > Eric Sunshine <sunshine@sunshineco.com> wrote:
> <snip>  Ah thanks, I can see your point-of-view, now.
> I always had the '^' in my mind since I've written the same
> thing in Perl and Ruby.

> On reflection, even with the '^' anchor, it isn't safe the way it's
> coded since '^' will match following a newline, won't it? Therefore,
> because 'line' isn't necessarily NUL-terminated, the pattern could
> match on some line beyond what get_one_line() considers the "current
> line".

> Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Tue, May 31, 2016 at 3:45 AM, Eric Wong <e@80x24.org> wrote:
> > > Eric Sunshine <sunshine@sunshineco.com> wrote:
> > >> I wonder if hand-coding, rather than using a regex, could be an improvement:
> > >>
> > >>     static int is_mboxrd_from(const char *s, size_t n)
> > >>     {
> > >>         size_t f = strlen("From ");
> > >>         const char *t = s + n;
> > >>
> > >>         while (s < t && *s == '>')
> > >>             s++;
> > >>         return t - s >= f && !memcmp(s, "From ", f);
> > >>     }
> > >>
> > >> or something.
> > >
> > > Yikes.  I mostly work in high-level languages and do my best to
> > > avoid string parsing in C; so that scares me.  A lot.
> > 
> > The hand-coded is_mboxrd_from() above is pretty much idiomatic C and
> > (I think) typical of how such a function would be coded in Git itself,
> > so it looks normal and easy to grok to me (but, of course, I'm
> > probably biased since I wrote it).

> Eric Wong <e@80x24.org> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > On Tue, May 31, 2016 at 3:45 AM, Eric Wong <e@80x24.org> wrote:
>> > > Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > >> I wonder if hand-coding, rather than using a regex, could be an improvement:
>> > >>
>> > >>     static int is_mboxrd_from(const char *s, size_t n)
>> > >>     {
>> > >>         size_t f = strlen("From ");
>> > >>         const char *t = s + n;
>> > >>
>> > >>         while (s < t && *s == '>')
>> > >>             s++;
>> > >>         return t - s >= f && !memcmp(s, "From ", f);
>> > >>     }
>> > >>
>> > >> or something.
>> > >
>> > > Yikes.  I mostly work in high-level languages and do my best to
>> > > avoid string parsing in C; so that scares me.  A lot.
>> >
>> > The hand-coded is_mboxrd_from() above is pretty much idiomatic C and
>> > (I think) typical of how such a function would be coded in Git itself,
>> > so it looks normal and easy to grok to me (but, of course, I'm
>> > probably biased since I wrote it).
>
> For reference, here is the gfrom function from qmail (gfrom.c,
> source package netqmail-1.06 in Debian, reformatted git style)
>
> int gfrom(char *s, int len)
> {
>         while ((len > 0) && (*s == '>')) {
>                 ++s;
>                 --len;
>         }
>
>         return (len >= 5) && !str_diffn(s, "From ", 5);
> }

> Similar to yours, but a several small things improves
> readability for me:
>
> * the avoidance of subtraction from the "return" conditional
> * s/n/len/ variable name

> str_diffn is memcmp-like, I assume.  My eyes glazed over
> when I saw that function implemented in str_diffn.c, too.
>
> Just thinking out loud, with sufficient tests I could go with
> either.  Will reroll when/if I get the chance tomorrow.

> On Thu, Jun 2, 2016 at 3:51 AM, Eric Wong <e@80x24.org> wrote:
>> Eric Wong <e@80x24.org> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> > On Tue, May 31, 2016 at 3:45 AM, Eric Wong <e@80x24.org> wrote:
>>> > > Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> > >> I wonder if hand-coding, rather than using a regex, could be an improvement:
>>> > >>
>>> > >>     static int is_mboxrd_from(const char *s, size_t n)
>>> > >>     {
>>> > >>         size_t f = strlen("From ");
>>> > >>         const char *t = s + n;
>>> > >>
>>> > >>         while (s < t && *s == '>')
>>> > >>             s++;
>>> > >>         return t - s >= f && !memcmp(s, "From ", f);
>>> > >>     }
>>> > >>
>>> > >> or something.
>>> > >
>>> > > Yikes.  I mostly work in high-level languages and do my best to
>>> > > avoid string parsing in C; so that scares me.  A lot.
>
> As mentioned above, it's all subjective and, of course, I have a bias
> toward the example I provided, but don't otherwise feel strongly about
> it. I do, however, like the idea of using a simple hand-coded matching
> function over the regex (but no so much that I would complain about
> it). Use whatever you and Junio feel is appropriate.

> Eric Sunshine <sunshine@sunshineco.com> writes:
>> On Thu, Jun 2, 2016 at 3:51 AM, Eric Wong <e@80x24.org> wrote:
>>> Eric Wong <e@80x24.org> wrote:
>>>> Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> > On Tue, May 31, 2016 at 3:45 AM, Eric Wong <e@80x24.org> wrote:
>>>> > > Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> > >> I wonder if hand-coding, rather than using a regex, could be an improvement:
>>>> > >>
>>>> > >>     static int is_mboxrd_from(const char *s, size_t n)
>>>> > >>     {
>>>> > >>         size_t f = strlen("From ");
>>>> > >>         const char *t = s + n;
>>>> > >>
>>>> > >>         while (s < t && *s == '>')
>>>> > >>             s++;
>>>> > >>         return t - s >= f && !memcmp(s, "From ", f);
>>>> > >>     }
>>>> > >>
>>>> > >> or something.
>>>> > >
>>>> > > Yikes.  I mostly work in high-level languages and do my best to
>>>> > > avoid string parsing in C; so that scares me.  A lot.
>>
>> As mentioned above, it's all subjective and, of course, I have a bias
>> toward the example I provided, but don't otherwise feel strongly about
>> it. I do, however, like the idea of using a simple hand-coded matching
>> function over the regex (but no so much that I would complain about
>> it). Use whatever you and Junio feel is appropriate.
>
> This is meant to be a replacement for the original that uses
> regexec(), which in turn means the string we are checking is
> guaranteed to be NUL terminated, right?

>         static int is_mboxrd_from(const char *line) {
>                 return starts_with(line + strspn(line, ">"), "From ");
>         }
>
> is sufficiently high-level that no longer is scary, hopefully?

>>         static int is_mboxrd_from(const char *line) {
>>                 return starts_with(line + strspn(line, ">"), "From ");
>>         }
>>
>> is sufficiently high-level that no longer is scary, hopefully?
>
> That's nice and concise but unfortunately not useful for this case
> where we must respect the logical end-of-line (within the larger
> buffer), represented by line+linelen.

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>>         static int is_mboxrd_from(const char *line) {
>>>                 return starts_with(line + strspn(line, ">"), "From ");
>>>         }
>>>
>>> is sufficiently high-level that no longer is scary, hopefully?
>>
>> That's nice and concise but unfortunately not useful for this case
>> where we must respect the logical end-of-line (within the larger
>> buffer), represented by line+linelen.
>
> Hmph, none of ">", "F", "r", "o", "m", or " " is "\n"; would eol
> still be an issue?

> On Fri, Jun 3, 2016 at 7:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>>>         static int is_mboxrd_from(const char *line) {
>>>>                 return starts_with(line + strspn(line, ">"), "From ");
>>>>         }
>>>>
>>>> is sufficiently high-level that no longer is scary, hopefully?
>>>
>>> That's nice and concise but unfortunately not useful for this case
>>> where we must respect the logical end-of-line (within the larger
>>> buffer), represented by line+linelen.
>>
>> Hmph, none of ">", "F", "r", "o", "m", or " " is "\n"; would eol
>> still be an issue?
>
> Ah right. Sorry for being slow.
>
> Your concise version of is_mboxrd_from() is a nice alternative, as well.

>
> My only minor reservation is that it your concise version is still
> subtle with regard to not taking 'linelen' into account. At first
> glance, it looks like a bug that it doesn't consider the logical
> end-of-line, and someone reading the code has to put in extra effort
> to convince himself that the code works as intended.
>
> For that reason, I have a bit of a preference for a version of
> is_mboxrd_from() which does take 'linelen' into account explicitly.