patch, 2 partstest mailinfo rfc3676 support

RFC 3676 establishes two parameters (Format and DelSP) to be used with the Text/Plain media type. In the presence of these parameters, trailing whitespace is used to indicate flowed lines and a canonical quote indicator is used to indicate quoted lines.

mailinfo now unfolds, unquotes, and un-space-stuffs such messages.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
It's been a while since I hacked C, so mucho scrutiny appreciated. The
mailinfo testsuite still passes, and this patch is followed by one which
adds a new test for this code, which also passes.

This is based off next, but mailinfo hasn't changed in a while, so it should apply cleanly to master (didn't test that though).

 builtin-mailinfo.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

Adds a format=flowed message to the sample.mbox in order to test mailinfo's rfc3676 support.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 t/t5100-mailinfo.sh |    2 +-
 t/t5100/info0009    |    5 ++
 t/t5100/msg0009     |    8 +++
 t/t5100/patch0009   |   98 ++++++++++++++++++++++++++++++++++++
 t/t5100/sample.mbox |  138 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 250 insertions(+), 1 deletions(-)
 create mode 100644 t/t5100/info0009
 create mode 100644 t/t5100/msg0009
 create mode 100644 t/t5100/patch0009

Hi,

On Thu, 14 Feb 2008, Jay Soffian wrote:

Hmm. Such a corrupt patch (lacking spaces at the beginning of the line) would not be accepted by git-apply. I briefly thought about teaching git-apply to grok that, with a flag. But now I think that mailsplit should handle that, no?

Question is: can you "de-corruptify" such a patch? (Note: it would probably need a validating step, too, i.e. count the lines it added a space to, and match that up with the numbers in the @@ lines)

Ciao, Dscho

On Fri, Feb 15, 2008 at 6:01 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Heh, I see this was a confusing patch. It is a lot clearer to read if you apply it and then take a look at what's added to sample.mbox, as well as look at the new files in t5100.

What I did to create this test was add the format=flowed code to mailinfo (my previous patch email), extract it with format-patch, then cut/paste it into my MUA and mail it to myself so I could have a sample format=flowed message. I then added that to the sample.mbox to see if the code I'd just added to mailinfo was working properly. Yes, it "de-corrupted" it just fine by removing the flowing.

j.

Hi,

On Thu, 14 Feb 2008, Jay Soffian wrote:

Why not call it "enum message_format"?

Why not call it "enum message_is_delsp"?

Does that mean that the format is only set if the content type is "text/plain"?

The !! is unnecessary; please skip it.

How about using strchrnul()?

Note: I do not know enough about format=flawed to know why you should skip 
to "> ".  I would have expected "\n", though.

The !! is unnecessary; please skip it.

Again, this is the job for strchrnul().

Or maybe
				cp[0 - (tp_delsp == DELSP_YES)] = '\0';

But maybe that is too cute.

But another thing struck me here: why setting *cp = '\0'; only if *(cp-1) == ' ', even if tp_delsp != DELSP_YES?

Did that just slip in, or was this intended. If the latter, is this related to format=flawed, or is it a bug fix in its own right?

Ciao, Dscho

On Fri, Feb 15, 2008 at 5:41 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

It's only applicable to text/plain messages. Then again, this will be set to FORMAT_NONE for non-text/plain messages so I guess that's just as good a name.

BTW, since all I care about is format=flowed, I could just as easily use an int and call it something like message_is_flowed. This would actually simplify the code a bit but I was trying to follow the existing code which uses an enum for message_type that could similarly have been an int called message_type_is_text.

Sure, my reasoning is same as above.

tp_format is initially set to FORMAT_NONE. Only for text/plain is it them set to FORMAT_FIXED or FORMAT_FLOWED. Again, though, all I care about is format=flowed so I could be using an int called message_is_flowed but was following the enum message_type example.

Heh. I'd never seen that before I started perusing the git code, I was just following along. But looking more carefully now I see that the other code only does that if it's assigning to an int. I see that it's not needed in a boolean context.

Cool. I'd never heard of it.

Heh, I think that's too clever by half.

" \n$" indicates the line is flowed, meaning the MUA added the '\n' to the line and we need to remove it. The next question is whether the space before the '\n' was also inserted by the MUA. If DELSP_YES, it was, so we want to remove it, else it was an existing space that we want to keep.

It was intended. The patch doesn't have enough context to have included this comment inside the TE_BASE64 case:

	/* this is a decoded line that may contain
	 * multiple new lines.  Pass only one chunk
	 * at a time to handle_filter()
	 */

It turns out that decoding QP can also cause a decoded line to contain extra newlines. So it's a bug fix, but I'm not sure it mattered before. I'll break it out into a separate patch though to make that clear. And I know I should add a test first for that fix, but ugh, figuring out the test case for a one-line code change is painful.

Thanks for the comments, I'll follow up with a revised patch.

j.

On Fri, Feb 15, 2008 at 5:41 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Actually strchrnul() here isn't correct. I want to strip leading '>' only. strchrnul() will search for the first occurrence (skipping over non-'>' to do so), which is not what I want.

Yep, here strchrnul would be fine.

j.

Hi,

On Fri, 15 Feb 2008, Jay Soffian wrote:

Oh, okay, I read again.

You just wanted to strip the leading ">". So I know even less what you tried to do. But then, I do not know anything about format=flawed either.

Ciao, Dscho

I really do not like this.

format=flowed instructs the receivers MUA that it is Ok to reflow the text when the message is presented to the user. That is exactly what we do _NOT_ want to happen to patches. Your implementation may cleanly salvage what the sender intended to send, but salvaging when applying the patch is too late.

You review the patch, decide to apply or reject, and then finally apply. Unmangling of corrupt contents should be done before you review, not before you apply.

We have similar hacks to clean-up MIME attachments and CTE. They are useful when your mailpath is not clean and can corrupt contents even if you try to send it as text/plain in-line patch as a fallback measure to ensure the contents not to get corrupted. format=flowed is completely opposite --- you are giving your and recipients MUA freedom to reflow the text, but there is no valid reason to allow that when sending patches.

We do not even encourage MIME attachments and ask senders to try sending uncorrupt patch in-line, _even though_ MIME attachments is a way to try harder not to corrupt the payload. Why should we encourage format=flowed which is meant to do the opposite (i.e. we do not care about the exact content, we care more about how better the paragraph looks and easier to read on screens with different width)?

The format is not meant for the exact transmission of text (like "patch") but more for paragraphed prose that can be re-fit on narrower display like phones. Section 5. even goes to say "Hand-aligned text, such as ASCII tables or art, source code, etc., SHOULD be sent as fixed, not flowed lines."

Side note. I did not look at the patch very carefully, but can you salvage a deleted text in the patch that removes a line that consists of "- " (minus followed by a single space and then end-of-line), or any deleted or added text that ends with a SP without making them misinterpreted as "flowed" line for that matter?

I even suspect that the sending MUA client may misbehave for such a patch line. In fact, doesn't section 4.2 say "a generating agant should trim spaces before user-inserted hard line breaks."? It implies to me that you cannot have a fixed line that ends with SP.

So just reject the patch when somebody sends you format=flowed and ask them to re-send without =flowed, and the world will be a much better place.

On Fri, Feb 15, 2008 at 12:10 PM, Junio C Hamano <gitster@pobox.com> wrote:

Ugh.

No, it indicates that the MUA sender mangled the message, but did so in a way that it can be unmanged* as long as the receiving MUA implements RFC 3676.

* Trailing whitespace removed by the MUA before sending is the one
  exception that cannot be unmangled; I'll address this further below.

As long as you're reviewing the patch in an MUA which implements RFC 3676, you'll be reviewing the unmangled patch. By your reasoning, mailinfo shouldn't decode QP or BASE64 either. If you want to make 100% sure you're reviewing *exactly* what you plan to apply, then you need to run it through git-am first and then review it via git-diff, no?

If you're reviewing in your MUA, your MUA may already be taking care of QP or BASE64 for you. If the MUA is RFC 3676 aware, it's un-flowing format=flowed too. mailinfo should do the same.

Not all MUA's can be configured to disable format=flowed. Sure, it's best if folks send 100% plain/text w/o any mangling, but their MUA may already be doing B64 or QP out of their control.

We're not trying to encourage it, just accommodate it where the sender can't disable it.

Again, some MUA's aren't configurable. They apply format=flowed to any text/plain message.

Yes, the sending MUA likely striped trailing whitespace and this cannot be recovered. Let's look at this in practice to see where it can be a problem.

Existing code generally shouldn't have trailing whitespace nor whitespace only lines. However, let's say that it does and that the patch refers to one of these lines (either as context or a subtraction). In this case that hunk will fail to apply, unless we teach git-apply to be lenient if the only difference between the line in the patch and the existing code is trailing whitespace.

In the case of an addition, the patch *shouldn't* contain trailing whitespace anyway. If it did, git-apply would in its default configuration flag it as a whitespace error. So arguably, the MUA is doing you a favor by stripping whitespace on such lines. :-)

I don't get it. Why not accommodate fascist RFC 3676 MUAs if we can for the same reasons we accommodate QP, BASE64, and attachments instead of inline? I guess I can only think of two reasons:

1) We need to accommodate QP, BASE64, etc since they may be due to the
   MTA. By contrast, format=flowed is always an MUA issue.

2) The other transformations are 100% safe in getting back exactly the
   patch the user intended. format=flowed isn't. But per above, I'm not
   sure the trailing-whitespace loss is a problem in practice.

j.

"Jay Soffian" <jaysoffian@gmail.com> writes:

There is no way for you to address this other than brushing it aside, saying "it does not matter". The information lost is information lost.

Says who?

We and the kernel folks are probably one of the more whitespace strict projects, but I am sure both of us get our fair share of patches that fix whitespace breakages while updating the surrounding area. There are existing breakages, and also there are lines that have deliberate trailing whitespaces.

Also, remember that this is not May 2005. git is not about the kernel and git itself. We passed that point long time ago. Other communities have different whitespace policies.

Failing to apply is not something I am worried about.

Silently applying bogosity is much more problematic, because it would be _silent_, and "git am" is often used to apply hundreds of patches in one go. We need to be able to trust the tool and the tool should error out when there is any ambiguity.

It just "warns". It does not strip by default. It should be a conscious user decision (done with "am --whitespace=fix") and the project policy.

I need to ask people listening from the sidelines on the list here. Was my explanation why MIME attachments and QP/BASE64 are _fundamentally different_ from format=flowed insufficient?

The point is, format=flowed is _not_ meant for precise transfer of the content matter. The design objective of format=flowed lies elsewhere. It achieves nicer looking line-wrapping by sacrificing the precise transfer of the content. It probably does a very good job, allowing you to communicate with pals on text-email enabled phones, although I do not have one so I cannot judge ;-).

Attachments, QP and BASE64 are all about getting the contents as intact as possible. Because they tend to make reviews harder, git and the kernel community frown upon them. But our tools tolerate them, because attachment is a fine or even preferred way to transfer the patches in other circles.

But format=flowed is different. It loses information. Not just one space at the end of the original line, but if you have very long line that has more than one spaces at an unfortunate place, the sending MUA can cut the line there and leave a single trailing space for the receiving end to reflow.

RFC3676 may be a good text communication medium. It is just not suitable for patch transfer. Just don't use it.

On Feb 16, 2008 1:57 AM, Junio C Hamano <gitster@pobox.com> wrote:

Well, according to the RFC, that shouldn't be the case. The only lost information should be trailing whitespace. Embedded whitespace should not be altered.

Would you consider making this configurable. Something like:

applymail.allowed_flowed = true/false/warn

If you're completely opposed though, we should modify git-am (and/or mailinfo) to reject format=flowed messages entirely, no?

j.

"Jay Soffian" <jaysoffian@gmail.com> writes:

Unfortunately, actual implementations and people's use matter more. I usually do not use graphical MUAs but when I tried to paste long lines to Thunderbird (or was it Evolution, I do not recall), it behaved as if I was typing the words, folded them at convenient inter word spaces. I would not blindly trust what RFC says.

There also seem to be a strong correlation between people who allow their MUA to send format=flowed when sending patches and people who cut and paste their patches to their MUA, corrupting whitespaces.

I would not even consider applying flowed message these days (my workflow is to review in MUA, save perhaps worthy ones to a separate mailbox and after re-reviewing apply), so they will not hit "git am" and it would not personally affect me. Honestly, I do not care very much either way.

But for some projects (perhaps the ones that do not value their source as much as we do ;-)) accepting a subset of flowed contents might be reasonable and even useful. Maybe something like this would be reasonably safe and still useful?

 - A format=flowed message that actually has a flowed line is
   always rejected;

 - If there is no flowed line (i.e. lines that end with SP) in
   the message, we unstuff the initial space to produce the
   result (there won't be anything else that is funny, will
   there?  In a patch we do not care much about the quotation of
   discussions):
 
   - If apply.whitespace is set to nowarn, we do not warn even
     though we might have lost the trailing whitespaces.

   - If apply.whitespace is set to warn, we warn about the
     flowed message.

   - If apply.whitespace is set to error or fix, we error out,
     but still leaving the result for manual inspection.

I dunno.

By the way, I do not think a solution that only uses configuration is usable.

When you have to apply many patches, you set your configuration to the most strict (i.e. apply.whitespace=error), and when the processing errors out, you then inspect the situation and manually override with the command line --whitespace=fix (or "warn") to process that one message. You need both.

Since mailinfo now has only a very few users (quiltimport and "am"), we probably could add --whitespace option to mailinfo, and teach "am" to pass --whitespace command line option it was given (otherwise the value from apply.whitespace configuration) when running mailinfo, if we were to do a "safer subset" as outlined above.

On Fri, Feb 15, 2008 at 09:10:19AM -0800, Junio C Hamano wrote:

I have to agree that this is a bad idea - for the reasons stated, and one additional:

There are MUA's which send 'format=fixed' email marked as 'format=flowed'.

At which point the receiving MUA performing the deflowing _will_ get a corrupt patch.

I saw this recently wrt to internal company code reviews. We have a variety of different MUAs in use on a variety of different OSs.

Some 'properly' implement format=flowed, some do not. Some always send format=flowed for text, some can have it disabled to send format=fixed. Despite what the RFCs say, some of them also QP or base64 encode such flowed pieces of text. It is a nightmare.

However it seems thay all use fixed format for attachments, unfortunately some them always tag those as application/octet-stream, so one cannot automatically process them (in the absense of other clues).

I am able to work around some of these issues by configuing mutt to do some guesses when it sees application/x-patch, or application/octet-stream and a file with say as '.diff' extension.

But ultimately for some of these one is forced to contact the original author and get access to the actual patch file on the filesystem. While this can be done for a 'small' community, it is not something that would work in the wider world.

So - format=flowed is simply unsuitable for fixed format text and should not be used.

[Actually it rather turns out format=flowed is a bad idea all together,
 you should see what it does to log files...]

DF