[PATCH] format-patch: Properly escape From_ lines when creating an mbox.

This uses the mboxrd style of quoting as documented here:
http://homepage.ntlworld.com/jonathan.deboynepollard/FGA/mail-mbox-formats.html

This matches the mboxrd-style un-escaping recently added to "git am".

Test 4152 is now extended to verify that format-patch does the proper
escaping.  It also now relies directly on the escaped output from
format-patch to verify that "git am" does the proper unescaping,
(where previously, it faked the escaped output with sed).

Signed-off-by: Carl Worth <redacted>
---

This patch is on top of the three patches I sent earlier. With this patch,
the series to make git use mbox in a robust fashion (and to avoid using mbox
where possible) is complete.

The entire test suite passes, and new tests are added for all new
functionality.

All of the features and caveats I mentioned earlier are taken care of. The
only potentially missing piece is that git-send-email doesn't have code
to un-escape From_ lines in an mbox. But this is irrelevant since send-
email doesn't even know how to handle an mbox anyway, (it will treat it
as one large email message instead).

Without this patch series, there's no documented way that an external
tool can use to reliably construct an mbox that will be correctly handled
by "git am". The best one could do is to peek inside the git implementation
and notice that it wants unescaped "From " lines, that it will ignore any
"From " line that doesn't end with something very much like asctime format,
and then somehow ensure that no messages in the mbox have lines that begin
with "From " and end with something like asctime format, (which won't be
possible in all cases without corrupting the message).

With this patch series, one can instead document that "git am" accepts an
mbox in "mboxrd" format as documented at the URL above, but with the caveat
that no additional characters are allowed after the asctime portion of the
"From " line. This requirement allows git to continue to accept mbox files
created by old versions of git, (with a very minor chance of corruption).

Mbox files create and consumed by versions of git after this patch series
should have no corruption by design.

 builtin/log.c                                      |    2 +-
 commit.h                                           |    5 ++-
 log-tree.c                                         |    1 +
 pretty.c                                           |   14 +++++++++++-
 ....sh => t4152-format-patch-am-From_-escaping.sh} |   21 ++++++++++++-------
 5 files changed, 30 insertions(+), 13 deletions(-)
 rename t/{t4152-am-From_.sh => t4152-format-patch-am-From_-escaping.sh} (78%)
 mode change 100755 => 100644

diff --git a/builtin/log.c b/builtin/log.c
index adbec9f..36b2f5a 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -763,7 +763,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 		     encoding);
 	pp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers,
 		      encoding, need_8bit_cte);
-	pp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0);
+	pp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0, 0);
 	printf("%s\n", sb.buf);
 
 	strbuf_release(&sb);

diff --git a/commit.h b/commit.h
index 6ef88dc..18e7197 100644
--- a/commit.h
+++ b/commit.h

@@ -70,6 +70,7 @@ struct pretty_print_context
 	const char *after_subject;
 	enum date_mode date_mode;
 	int need_8bit_cte;
+	int need_from_escaping;
 	int show_notes;
 	struct reflog_walk_info *reflog_info;
 };

@@ -103,8 +104,8 @@ void pp_title_line(enum cmit_fmt fmt,
 void pp_remainder(enum cmit_fmt fmt,
 		  const char **msg_p,
 		  struct strbuf *sb,
-		  int indent);
-
+		  int indent,
+		  int need_from_escaping);
 
 /** Removes the first commit from a list sorted by date, and adds all
  * of its parents.

diff --git a/log-tree.c b/log-tree.c
index 6aab273..1a179dc 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -415,6 +415,7 @@ void show_log(struct rev_info *opt)
 	ctx.abbrev = opt->diffopt.abbrev;
 	ctx.after_subject = extra_headers;
 	ctx.reflog_info = opt->reflog_info;
+	ctx.need_from_escaping = opt->format_mbox;
 	pretty_print_commit(opt->commit_format, commit, &msgbuf, &ctx);
 
 	if (opt->add_signoff)

diff --git a/pretty.c b/pretty.c
index 74cda1b..62b376b 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1011,7 +1011,8 @@ void pp_title_line(enum cmit_fmt fmt,
 void pp_remainder(enum cmit_fmt fmt,
 		  const char **msg_p,
 		  struct strbuf *sb,
-		  int indent)
+		  int indent,
+		  int need_from_escaping)
 {
 	int first = 1;
 	for (;;) {

@@ -1030,6 +1031,15 @@ void pp_remainder(enum cmit_fmt fmt,
 		}
 		first = 0;
 
+		if (need_from_escaping && (*line == '>' || *line == 'F'))
+		{
+			const char *s = line;
+			while (*s == '>')
+				s++;
+			if (strncmp (s, "From ", 5) == 0)
+				strbuf_addch(sb, '>');
+		}
+
 		strbuf_grow(sb, linelen + indent + 20);
 		if (indent) {
 			memset(sb->buf + sb->len, ' ', indent);

@@ -1117,7 +1127,7 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 
 	beginning_of_body = sb->len;
 	if (fmt != CMIT_FMT_ONELINE)
-		pp_remainder(fmt, &msg, sb, indent);
+		pp_remainder(fmt, &msg, sb, indent, context->need_from_escaping);
 	strbuf_rtrim(sb);
 
 	/* Make sure there is an EOLN for the non-oneline case */

diff --git a/t/t4152-am-From_.sh b/t/t4152-format-patch-am-From_-escaping.sh
old mode 100755
new mode 100644
similarity index 78%
rename from t/t4152-am-From_.sh
rename to t/t4152-format-patch-am-From_-escaping.sh
index 02821ee..dc013bb
--- a/t/t4152-am-From_.sh
+++ b/t/t4152-format-patch-am-From_-escaping.sh

@@ -34,13 +34,18 @@ test_expect_success setup '
 	test_tick &&
 	git commit -s -F msg &&
 	git tag second &&
-	git format-patch --stdout first | sed -e "1{p;d};s/^\(>*From \)/>\1/" > From_ &&
-	{
-		echo "X-Fake-Field: Line One" &&
-		echo "X-Fake-Field: Line Two" &&
-		echo "X-Fake-Field: Line Three" &&
-		git format-patch --stdout first | sed -e "1d"
-	} > From_.eml
+	git format-patch --stdout first > From_ &&
+	git format-patch first
+'
+
+test_expect_success 'format-patch escapes From_ lines in mbox' '
+	head -1 From_ | grep "^From " &&
+	test "$(grep "^From " From_ | wc -l)" = "1"
+'
+
+test_expect_success 'format-patch does not escapes From_ lines in email' '
+	head -1 0001-From_-lines.patch | grep -v "^From " >/dev/null &&
+	test "$(grep "^From " 0001-From_-lines.patch | wc -l)" = "1"
 '
 
 test_expect_success 'am unescapes From_ lines from mbox' '

@@ -54,7 +59,7 @@ test_expect_success 'am unescapes From_ lines from mbox' '
 
 test_expect_success 'am does not unescape From_ lines from email' '
 	git checkout first &&
-	git am From_.eml &&
+	git am 0001-From_-lines.patch &&
 	! test -d .git/rebase-apply &&
 	test -z "$(git diff second)" &&
 	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&

-- 
1.7.0.4

Carl Worth [off-list ref] writes:

Without this patch series, there's no documented way that an external
tool can use to reliably construct an mbox that will be correctly handled
by "git am". The best one could do is to peek inside the git implementation
and notice that it wants unescaped "From " lines, that it will ignore any
"From " line that doesn't end with something very much like asctime format,
and then somehow ensure that no messages in the mbox have lines that begin
with "From " and end with something like asctime format, (which won't be
possible in all cases without corrupting the message).

I have this small suspicion that mboxrd may be a suboptimal choice, when
you consider how robustly we can notice a failure (and to a lessor extent,
recover from it) when using output from "format-patch --stdout" to
sneakernet between existing and updated versions of git.  Especially
because your implementation quotes lines that begin with "From "
unconditionally (even when the tail end of the line would never be a
valid-looking timestamp).  Such an output will confuse existing mailsplit,
but the worst part of the story is that somebody who is applying a series
of patches will _not_ notice the breakage.  The payload of the second and
subsequent messages will likely be concatenated as if it were part of the
first message, ignoring cruft between patches, but the resulting tree
would likely to be the same as what the sending end intended.

Compared to that, I think a failure to split a message in the middle (iow,
commit message happened to have a line that begins with "From " and ends
with a timestamp-looking string) is much easier to notice (because the
first part of the message that was incorrectly split at such a line will
not have any patch, so "git am" will stop).  IOW, failure to split is
easier to notice than splitting too eagerly.

Perhaps perfect is an enemy of good?

On Tue, 08 Jun 2010 20:50:01 -0700, Junio C Hamano [off-list ref] wrote:

Carl Worth [off-list ref] writes:
Especially because your implementation quotes lines that begin with "From "
unconditionally (even when the tail end of the line would never be a
valid-looking timestamp).  Such an output will confuse existing mailsplit,
but the worst part of the story is that somebody who is applying a series
of patches will _not_ notice the breakage.  The payload of the second and
subsequent messages will likely be concatenated as if it were part of the
first message, ignoring cruft between patches, but the resulting tree
would likely to be the same as what the sending end intended.

I agree that anything that results in multiple patches being (silently!)
concatenated would be catastrophic and I do not recommend accepting any
patches that could result in failures like that.

Could you describe in more detail how the implementation could lead to a
case like that? I'm not seeing it myself. But if you can show me, I'll
be happy to attempt a fix.

In particular, I don't see how any of the new quoting will confuse
existing mailsplit. The splitting itself shouldn't be changed. And at
worst, using new "git format-patch" with old mailsplit could result in a
">From " getting into a commit message where a "From " should be.

We could reduce the occurrence of that problem by being less aggressive
with "From " quoting, (for example, examining whether the tail of the
line looks like a timestamp before quoting). The cost there would be
fairly minor. It would increase the occurrence of a failure to pass a
">From " correctly from a new "git am" to a new "git mailsplit". [*]

I don't see a way to eliminate both problems other than specifying that
git's mbox format is a non-standard mbox format that looks specifically
for From_ lines ending in timestamps and is not capable of containing an
arbitrary message, (namely messages with lines that begin with "From "
and end with timestamps).

That would be a particularly unsatisfying solution for me, since I'm
trying to implement an mbox-export option in a mail client as a general
feature (that happens to work with git) rather than implementing a
git-specific export option.

-Carl

[*] It would seem a strange strategy to make new git compatible with old
git while not being perfectly compatible with itself going forward, but
that is a possibility.

-- 
carl.d.worth@intel.com

On 06/08/2010 08:50 PM, Junio C Hamano wrote:

Carl Worth [off-list ref] writes:

quoted

Without this patch series, there's no documented way that an external
tool can use to reliably construct an mbox that will be correctly handled
by "git am". The best one could do is to peek inside the git implementation
and notice that it wants unescaped "From " lines, that it will ignore any
"From " line that doesn't end with something very much like asctime format,
and then somehow ensure that no messages in the mbox have lines that begin
with "From " and end with something like asctime format, (which won't be
possible in all cases without corrupting the message).

I have this small suspicion that mboxrd may be a suboptimal choice, when
you consider how robustly we can notice a failure (and to a lessor extent,
recover from it) when using output from "format-patch --stdout" to
sneakernet between existing and updated versions of git.  Especially
because your implementation quotes lines that begin with "From "
unconditionally (even when the tail end of the line would never be a
valid-looking timestamp).  Such an output will confuse existing mailsplit,
but the worst part of the story is that somebody who is applying a series
of patches will _not_ notice the breakage.  The payload of the second and
subsequent messages will likely be concatenated as if it were part of the
first message, ignoring cruft between patches, but the resulting tree
would likely to be the same as what the sending end intended.

Compared to that, I think a failure to split a message in the middle (iow,
commit message happened to have a line that begins with "From " and ends
with a timestamp-looking string) is much easier to notice (because the
first part of the message that was incorrectly split at such a line will
not have any patch, so "git am" will stop).  IOW, failure to split is
easier to notice than splitting too eagerly.

Perhaps perfect is an enemy of good?

For production perhaps we should do the MIME-escape thing?

For consumption, it's not so clear...

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

On Tue, 08 Jun 2010 22:48:44 -0700, "H. Peter Anvin" [off-list ref] wrote:

quoted

Perhaps perfect is an enemy of good?

For production perhaps we should do the MIME-escape thing?

For consumption, it's not so clear...

I suggest as a first step accepting the following:

	format-patch: Emit bare email rather than mbox for single messages.
	<id:1276040615-26008-1-git-send-email-cworth@cworth.org>

That patch should be entirely uncontroversial since it doesn't introduce
any new escaping, neither on the production nor on the consumption side.

It has the tremendous benefit of removing the mbox format entirely from
the "git send-email" workflow, (which will just use bare messages
instead).

With that patch in place, the only place that git will still generate
mbox files is "format-patch --stdout". And the most common use of that
is within git-rebase. For git-rebase, it doesn't matter what kind of
mbox is used as long as it's consistent, since it's practically
guaranteed that git-rebase will be using consistent versions of both
"git format-patch" and "git am".

At that point, I think discussion of confusion from new format-patch and
old am becomes almost meaningless as such interaction will most likely
be happening through bare messages rather than mbox files. When an mbox
file *is* involved I think it will be even more likely to happen through
some external program, (such as an MUA collecting a thread of
git-send-email messages and presenting them to "git am" as an mbox).

-Carl

-- 
carl.d.worth@intel.com

On Tue, 08 Jun 2010 22:14:23 -0700, Carl Worth [off-list ref] wrote:

On Tue, 08 Jun 2010 20:50:01 -0700, Junio C Hamano [off-list ref] wrote:

quoted

Carl Worth [off-list ref] writes:
Especially because your implementation quotes lines that begin with "From "
unconditionally (even when the tail end of the line would never be a
valid-looking timestamp).  Such an output will confuse existing mailsplit,
but the worst part of the story is that somebody who is applying a series
of patches will _not_ notice the breakage.  The payload of the second and
subsequent messages will likely be concatenated as if it were part of the
first message, ignoring cruft between patches, but the resulting tree
would likely to be the same as what the sending end intended.

...

Could you describe in more detail how the implementation could lead to a
case like that? I'm not seeing it myself. But if you can show me, I'll
be happy to attempt a fix.

Oh, perhaps I understand what you were getting at here.

If a commit is created (by whatever means) with a commit message that
has a line of the form:

	"From ... <timestamp>"

then with the existing code, there will be a failure if someone does a
format-patch and a git-am of that commit. And that might raise attention
that perhaps something went wrong.

But with my patch series, that commit will transfer through the
format-patch and git-am just fine.

I would contend that preserving this commit is the right (and "robust")
thing to do. For example, looking at the log recent of git.git master I
see 5 commits that have a "From ... <timestamp>" line in the commit
message. 

	34122b57eca747022336f5a3dc1aa80377d1ce56
	48027a918d89bad6735897a2c3da77c0451a038c
        19a8721ef8f82153fee93c62bd050659cf718d6d
	3dc1383290f9db3371a13ae8009ce4fcd5ffc93a
	1dfcfbce2d643b7c7b56dc828f36ced9de2bf9f2

They all look to me like mistakes, some worse than others. But now that
they are part of the history of the project, it would be better and more
robust of git to actually be able to replay these successfully.

Git has various tools for rewriting history, which are useful for
various reasons. But these tools will get tripped up on a commit like
one of the above. For example, taking the most recent commit from above,
"git rebase" is unable to replay it successfully:

	$ git checkout -b tmp 34122b57eca747022336f5a3dc1aa80377d1ce56
	Switched to a new branch 'tmp'
	$ git rebase --onto HEAD~2 HEAD~1
	First, rewinding head to replay your work on top of it...
	Patch is empty.  Was it split wrong?

After my patch series this rebase works:

	$ git checkout -b tmp 34122b57eca747022336f5a3dc1aa80377d1ce56
	Switched to a new branch 'tmp'
	0:~/src/git:(tmp)$ git rebase --onto HEAD~2 HEAD~1
	First, rewinding head to replay your work on top of it...
	Applying: gitweb: Always use three argument form of open

That's git being demonstrably more robust. And an operation like that
would make a good test for git's test suite.

Now, it's likely git could also use some help to avoid whatever mistakes
caused these commits to be created in the first place, but that's an
orthogonal issue.

Also, there is one commit that is more particularly broken than any of
the others. Even my patch series is not sufficient to successfully
replay the following commit:

	1dfcfbce2d643b7c7b56dc828f36ced9de2bf9f2

That's because in addition to the From_ line in the commit message, this
commit also has an entire additional patch within the commit
message. And git's "patch as email" format has an additional quoting
problem with the "---" delimiter to separate the commit message from the
patch. And again, that's orthogonal from the mbox quoting I'm currently
trying to solve.

-Carl

-- 
carl.d.worth@intel.com