patch, 2 partsmailinfo: Add support for keep_cr

From: Matthew Wilcox <mawilcox@microsoft.com>

If you have a base-64 encoded patch with CRLF endings (as produced by forwarding a patch from Outlook to a Linux machine, for example), the keep_cr setting is not honoured because keep_cr is only passed to mailsplit, which does not look through the encoding. The keep_cr logic needs to be applied after the base64 decode. I copied that logic to handle_filter(), and rather than add a new keep_cr parameter to handle_filter, I opted to add keep_cr to struct mailinfo; it seemed appropriate given use_scissors was already there.

Then I needed to initialise keep_cr in the struct mailinfo passed from git-am, and rather than thread a 'keep_cr' parameter all the way through to parse_mail(), I decided to add keep_cr to struct am_state, which let it be removed as a parameter from five other functions.

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 builtin/am.c | 49 ++++++++++++++++++++++++-------------------------
 mailinfo.c   |  6 ++++++
 mailinfo.h   |  1 +
 3 files changed, 31 insertions(+), 25 deletions(-)

From: Matthew Wilcox <mawilcox@microsoft.com>

Extend the --scissors mechanism to strip off the preamble created by forwarding a patch. There are a couple of extra headers ("Sent" and "To") added by forwarding, but other than that, the --scissors option will now remove patches forwarded from Microsoft Outlook to a Linux email account.

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 mailinfo.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

On 01/12/2017 01:20 AM, Matthew Wilcox wrote:

Also add a test showing the kind of message that the current code doesn't handle, and that this commit addresses.

Are these extra headers used in both the "real" e-mail headers and the in-body headers, or only one of them? (If the latter, they should probably be handled only in the relevant function - my previous patches to this file were in that direction too, if I remember correctly.) Also, I suspect that these will have to be handled differently to the other 3, but that will be clearer when you add the test with an example message.

1) You can use starts_with or skip_prefix.
2) This seems vulnerable to false positives. If "Original Message" 
always follows a certain kind of line, it might be better to check for 
that. (Again, it will be clearer when we have an example message.)

Why 15? Also, can skip_prefix avoid these magic numbers?

From: Jonathan Tan [mailto:jonathantanmy@google.com]

OK.  For the sake of discussion, here's what the base64-decoded message looks like:

--- 8< ---

-----Original Message-----
From: Rehas Sachdeva [mailto:aquannie@gmail.com]
Sent: Wednesday, January 4, 2017 11:55 AM
To: Matthew Wilcox <mawilcox@microsoft.com>; riel@surriel.com
Subject: [PATCH v3] radix tree test suite: Dial down verbosity with -v

Make the output of radix tree test suite less verbose by default and add -v and -vv command line options for increasing level of verbosity.

--- >8 ---

Without this part of the patch, using --scissors means it stops parsing headers when it hits the 'Sent' line.  So all I'm looking for is to have 'is_inbody_header()' return true.  If there's a better way to achieve that, I'm all for it.  Without this hunk of the patch, the commit looks like this:

Without any of this patch, the commit looks like this:

Author: Matthew Wilcox <mawilcox@microsoft.com>
Date:   Sat Jan 7 18:33:43 2017 +0000

    FW: [PATCH v3] radix tree test suite: Dial down verbosity with -v

    -----Original Message-----
    From: Rehas Sachdeva [mailto:aquannie@gmail.com]
    Sent: Wednesday, January 4, 2017 11:55 AM
    To: Matthew Wilcox <mawilcox@microsoft.com>; riel@surriel.com
    Subject: [PATCH v3] radix tree test suite: Dial down verbosity with -v

    Make the output of radix tree test suite less verbose by default and add
    -v and -vv command line options for increasing level of verbosity.

Without this hunk of the patch, the commit looks like this:

Author: Rehas Sachdeva <[mailto:aquannie@gmail.com]>
Date:   Sat Jan 7 18:33:43 2017 +0000

    FW: [PATCH v3] radix tree test suite: Dial down verbosity with -v

    Sent: Wednesday, January 4, 2017 11:55 AM
    To: Matthew Wilcox <mawilcox@microsoft.com>; riel@surriel.com
    Subject: [PATCH v3] radix tree test suite: Dial down verbosity with -v

    Make the output of radix tree test suite less verbose by default and add
    -v and -vv command line options for increasing level of verbosity.

And with this hunk, it turns into this:

Author: Rehas Sachdeva <[mailto:aquannie@gmail.com]>
Date:   Sat Jan 7 18:33:43 2017 +0000

    radix tree test suite: Dial down verbosity with -v

    Make the output of radix tree test suite less verbose by default and add
    -v and -vv command line options for increasing level of verbosity.

There's more work to do here, turning that silly <[mailto:]> into a proper email address, for example, and parsing Sent: like we parse Date:, but I thought it worth sending out patches 1 & 2 before writing patch 3.

I was following the existing logic in this function that looks for 8< or >8 or ...

I don't think it's terribly vulnerable to false positives.  The logic at the end of the function tries to make sure that the scissor marks make up a substantial component of the line.

We could do this differently; I'm not sure if there's a regex library built into git, but we could even custom-write a separate matcher that looks only for ^-{3,}Original Message-{3,}$

Or we could use a different option; eg --forwarded that will trim off the extra gunk associated with forwarding messages, instead of overloading --scissors.

Again, following the rest of the function.  c has already been advanced by 1, now it needs to be advanced to the end of the 16 character string "Original Message".

Matthew Wilcox <mawilcox@linuxonhyperv.com> writes:

To be quite honest, I am not enthused to even having to think about this kind of change. There seems to be no standard way MUAs produce "forwarded" messages, and this adds support for only one specific MUA, that happens to say "Original Message". Why should such a thing hardcoded in this codepath?

I think I am OK with a patch that lets users customize is_scissors_line(), perhaps accepting a regexp that ought to match a line to consider a scissors line. When such a regexp is given, check for a match before doing the "do we have a line filled with '-' with the scissors marker and is the mark long enough?" check.

Then you can do

	mailinfo --scissors='^-{5}Original Message-{5}$'

or something like that. Perhaps allow multiple regexps, or even allow them to come from a multi-valued configuration variable.

This array lists fields whose value we _care_ about. Do not put random garbage whose value we do not use in it.

Even though I am not enthused to see support for messages forwarded by Outlook bolted onto this codepath, I think it may make sense to allow random garbage that looks like an e-mail header to appear immediately after a scissors line (and ignore them). For that, perhaps you would instead want to extend is_inbody_header() so that after it decides that the given line is *NOT* one of the header field we care about, return a value that is not 0 or 1. Its caller currently expects to see 1 if it is a relevant in-body header line, or 0 if the line signals the end of the in-body header block. You'd be adding another class of lines that are not a header line with a meaning but do not terminate the in-body header block.

On 01/12/2017 01:20 AM, Matthew Wilcox wrote:

A test exercising the new functionality would be nice.

Also, maybe a more descriptive title like "mailinfo: also respect keep_cr after base64 decode" (50 characters) is better.

Maybe query the git config here (instead of later) so that we never have to worry about state->keep_cr being neither 0 nor 1? This function already queries the git config anyway.

I personally would write "unsigned keep_cr : 1" to further emphasize that this can only be 0 or 1, but I don't know if it's better to keep with the style existing in the file (that is, using int).

From: Jonathan Tan [mailto:jonathantanmy@google.com]

Roger.

No problem.

I wondered why the existing code didn't do that, but I wanted to make a minimal change rather than clean up an older mistake.  I'm happy to do it that way.

Probably best to stick to the existing file ... someone can always do a cleanup patch later, and having this match the others will make that easier, not harder.

Thanks for the review.