Re: [PATCH 1/3] pretty: support "mboxrd" output format
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 31, 2016, 03:40 UTC
- Message-ID
- <CAPig+cQrSJe03_RtSyf5KO2vE3Rri7t70-he8SXA9Y4oBYY_Ww@mail.gmail.com>
- In-Reply-To
- <20160530232142.21098-2-e@80x24.org>
On Mon, May 30, 2016 at 7:21 PM, Eric Wong <e@80x24.org> wrote:
Show 39 quoted lines
> 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.
Show 10 quoted lines
> + > 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.
Show 22 quoted lines
> + 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