Re: [PATCH 1/3] pretty: support "mboxrd" output format

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, May 30, 2016 at 7:21 PM, Eric Wong <e@80x24.org> wrote:
> 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, '>');

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);
    }

    ...
    if (is_mboxrd_from(line, linelen)
        strbuf_addch(sb, '>');

or something.

> +
>                         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 &&

Maybe use <<-\INPUT_END to indicate that no variable interpolation is
expected. Ditto below.

> +       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 &&

Hmm, -A is not POSIX and is otherwise not used in Git tests. Perhaps
you could use 'git grep --no-index -A' instead or something?

> +       test_cmp expect actual
> +'
> +
>  test_done