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

[Carl Worth <cworth@cworth.org>]

[-- Attachment #1: Type: text/plain, Size: 3866 bytes --]

On Tue, 08 Jun 2010 22:14:23 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Tue, 08 Jun 2010 20:50:01 -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > Carl Worth <cworth@cworth.org> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]