[PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Jay Soffian]

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(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 2600847..deaf92b 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -20,6 +20,13 @@ static enum  {
 static enum  {
 	TYPE_TEXT, TYPE_OTHER,
 } message_type;
+/* RFC 3676 Text/Plain Format and DelSp Parameters */
+static enum {
+	FORMAT_NONE, FORMAT_FIXED, FORMAT_FLOWED,
+} tp_format;
+static enum {
+	DELSP_NONE, DELSP_YES, DELSP_NO,
+} tp_delsp;
 
 static char charset[256];
 static int patch_lines;
@@ -193,6 +200,18 @@ static int handle_content_type(char *line)
 
 	if (strcasestr(line, "text/") == NULL)
 		 message_type = TYPE_OTHER;
+	else if (strcasestr(line, "text/plain")) {
+		char attr[256];
+		if (slurp_attr(line, "format=", attr) && !strcasecmp(attr, "flowed")) {
+			tp_format = FORMAT_FLOWED;
+			if (slurp_attr(line, "delsp=", attr) && !strcasecmp(attr, "yes"))
+				tp_delsp = DELSP_YES;
+			else
+				tp_delsp = DELSP_NO;
+		}
+		else
+			tp_format = FORMAT_FIXED;
+	}
 	if (slurp_attr(line, "boundary=", boundary + 2)) {
 		memcpy(boundary, "--", 2);
 		if (content_top++ >= &content[MAX_BOUNDARIES]) {
@@ -681,6 +700,8 @@ again:
 	transfer_encoding = TE_DONTCARE;
 	charset[0] = 0;
 	message_type = TYPE_TEXT;
+	tp_format = FORMAT_NONE;
+	tp_delsp = DELSP_NONE;
 
 	/* slurp in this section's info */
 	while (read_one_header_line(line, sizeof(line), fin))
@@ -770,6 +791,24 @@ static int handle_filter(char *line, unsigned linesize)
 {
 	static int filter = 0;
 
+	if (tp_format == FORMAT_FLOWED && !!strcmp(line, "-- \n")) {
+		char *cp = line;
+		while (*cp == '>' && *cp != 0)
+			cp++;
+		if (*cp == ' ')
+			cp++;
+		line = cp;
+		if (!!strcmp(line, "-- \n")) {
+			while (*cp != '\n' && *cp !=0)
+				cp++;
+			if (cp > line && *cp == '\n' && *(cp-1) == ' ') {
+				if (tp_delsp == DELSP_YES)
+					*(cp-1) = '\0';
+				else
+					*cp = '\0';
+			}
+		}
+	}
 	/* filter tells us which part we left off on
 	 * a non-zero return indicates we hit a filter point
 	 */
@@ -818,6 +857,7 @@ static void handle_body(void)
 
 		switch (transfer_encoding) {
 		case TE_BASE64:
+		case TE_QP:
 		{
 			char *op = line;
 
-- 
1.5.4.1.1281.g75df

[PATCH 2/2] test mailinfo rfc3676 support

[Jay Soffian]

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

diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 9b1a745..d6c55c1 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \
 	'git mailsplit -o. ../t5100/sample.mbox >last &&
 	last=`cat last` &&
 	echo total is $last &&
-	test `cat last` = 8'
+	test `cat last` = 9'
 
 for mail in `echo 00*`
 do
diff --git a/t/t5100/info0009 b/t/t5100/info0009
new file mode 100644
index 0000000..63369be
--- /dev/null
+++ b/t/t5100/info0009
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: mailinfo: support rfc3676 (format=flowed) text/plain messages
+Date: Thu, 14 Feb 2008 20:56:34 -0500
+
diff --git a/t/t5100/msg0009 b/t/t5100/msg0009
new file mode 100644
index 0000000..eb9c7f8
--- /dev/null
+++ b/t/t5100/msg0009
@@ -0,0 +1,8 @@
+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: A U Thor <a.u.thor@example.com.com
diff --git a/t/t5100/patch0009 b/t/t5100/patch0009
new file mode 100644
index 0000000..a167e73
--- /dev/null
+++ b/t/t5100/patch0009
@@ -0,0 +1,98 @@
+---
+The next line will test space stuffing
+From A U Thor <a.u.thor@example.com.com>
+
+The next line will test space stuffing also, and force Mail.app to send this as quoted-printable. This line will be flowed.
+> Märchen
+
+A flowed quoted paragraph follows:
+Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
+
+And again with deeper quoting depth:
+Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
+
+
+builtin-mailinfo.c |   40 ++++++++++++++++++++++++++++++++++++++++
+1 files changed, 40 insertions(+), 0 deletions(-)
+
+diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
+index 2600847..deaf92b 100644
+--- a/builtin-mailinfo.c
++++ b/builtin-mailinfo.c
+@@ -20,6 +20,13 @@ static enum  {
+static enum  {
+	TYPE_TEXT, TYPE_OTHER,
+} message_type;
++/* RFC 3676 Text/Plain Format and DelSp Parameters */
++static enum {
++	FORMAT_NONE, FORMAT_FIXED, FORMAT_FLOWED,
++} tp_format;
++static enum {
++	DELSP_NONE, DELSP_YES, DELSP_NO,
++} tp_delsp;
+
+static char charset[256];
+static int patch_lines;
+@@ -193,6 +200,18 @@ static int handle_content_type(char *line)
+
+	if (strcasestr(line, "text/") == NULL)
+		 message_type = TYPE_OTHER;
++	else if (strcasestr(line, "text/plain")) {
++		char attr[256];
++		if (slurp_attr(line, "format=", attr) && !strcasecmp(attr, "flowed")) {
++			tp_format = FORMAT_FLOWED;
++			if (slurp_attr(line, "delsp=", attr) && !strcasecmp(attr, "yes"))
++				tp_delsp = DELSP_YES;
++			else
++				tp_delsp = DELSP_NO;
++		}
++		else
++			tp_format = FORMAT_FIXED;
++	}
+	if (slurp_attr(line, "boundary=", boundary + 2)) {
+		memcpy(boundary, "--", 2);
+		if (content_top++ >= &content[MAX_BOUNDARIES]) {
+@@ -681,6 +700,8 @@ again:
+	transfer_encoding = TE_DONTCARE;
+	charset[0] = 0;
+	message_type = TYPE_TEXT;
++	tp_format = FORMAT_NONE;
++	tp_delsp = DELSP_NONE;
+
+	/* slurp in this section's info */
+	while (read_one_header_line(line, sizeof(line), fin))
+@@ -770,6 +791,24 @@ static int handle_filter(char *line, unsigned linesize)
+{
+	static int filter = 0;
+
++	if (tp_format == FORMAT_FLOWED && !!strcmp(line, "-- \n")) {
++		char *cp = line;
++		while (*cp == '>' && *cp != 0)
++			cp++;
++		if (*cp == ' ')
++			cp++;
++		line = cp;
++		if (!!strcmp(line, "-- \n")) {
++			while (*cp != '\n' && *cp !=0)
++				cp++;
++			if (cp > line && *cp == '\n' && *(cp-1) == ' ') {
++				if (tp_delsp == DELSP_YES)
++					*(cp-1) = '\0';
++				else
++					*cp = '\0';
++			}
++		}
++	}
+	/* filter tells us which part we left off on
+	 * a non-zero return indicates we hit a filter point
+	 */
+@@ -818,6 +857,7 @@ static void handle_body(void)
+
+		switch (transfer_encoding) {
+		case TE_BASE64:
++		case TE_QP:
+		{
+			char *op = line;
+
+-- 
+1.5.4.1.1281.g75df
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 070c166..a1f9833 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -407,3 +407,141 @@ Subject: [PATCH] another patch
 
 Hey you forgot the patch!
 
+From nobody Thu Feb 14 21:02:08 2008
+Message-Id: <FCDFE42A-6A58-4F77-AEF2-E94C5373B14F@soffian.org>
+From: A U Thor <a.u.thor@example.com>
+To: A U Thor <a.u.thor@example.com>
+Content-Type: text/plain; charset=UTF-8; format=flowed; delsp=yes
+Content-Transfer-Encoding: quoted-printable
+Mime-Version: 1.0 (Apple Message framework v919.2)
+Subject: [PATCH] mailinfo: support rfc3676 (format=flowed) text/plain messages
+Date: Thu, 14 Feb 2008 20:56:34 -0500
+X-Mailer: Apple Mail (2.919.2)
+
+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: A U Thor <a.u.thor@example.com.com
+---
+The next line will test space stuffing
+ =46rom A U Thor <a.u.thor@example.com.com>
+
+The next line will test space stuffing also, and force Mail.app to =20
+send this as quoted-printable. This line will be flowed.
+ > M=C3=A4rchen
+
+A flowed quoted paragraph follows:
+> Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do =20
+> eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim =20=
+
+> ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut =20
+> aliquip ex ea commodo consequat. Duis aute irure dolor in =20
+> reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla =20=
+
+> pariatur. Excepteur sint occaecat cupidatat non proident, sunt in =20
+> culpa qui officia deserunt mollit anim id est laborum.
+
+And again with deeper quoting depth:
+>>> Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do =20
+>>> eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut =20
+>>> enim ad minim veniam, quis nostrud exercitation ullamco laboris =20
+>>> nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in =20=
+
+>>> reprehenderit in voluptate velit esse cillum dolore eu fugiat =20
+>>> nulla pariatur. Excepteur sint occaecat cupidatat non proident, =20
+>>> sunt in culpa qui officia deserunt mollit anim id est laborum.
+
+
+builtin-mailinfo.c |   40 ++++++++++++++++++++++++++++++++++++++++
+1 files changed, 40 insertions(+), 0 deletions(-)
+
+diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
+index 2600847..deaf92b 100644
+--- a/builtin-mailinfo.c
++++ b/builtin-mailinfo.c
+@@ -20,6 +20,13 @@ static enum  {
+static enum  {
+	TYPE_TEXT, TYPE_OTHER,
+} message_type;
++/* RFC 3676 Text/Plain Format and DelSp Parameters */
++static enum {
++	FORMAT_NONE, FORMAT_FIXED, FORMAT_FLOWED,
++} tp_format;
++static enum {
++	DELSP_NONE, DELSP_YES, DELSP_NO,
++} tp_delsp;
+
+static char charset[256];
+static int patch_lines;
+@@ -193,6 +200,18 @@ static int handle_content_type(char *line)
+
+	if (strcasestr(line, "text/") =3D=3D NULL)
+		 message_type =3D TYPE_OTHER;
++	else if (strcasestr(line, "text/plain")) {
++		char attr[256];
++		if (slurp_attr(line, "format=3D", attr) && =
+!strcasecmp(attr, =20
+"flowed")) {
++			tp_format =3D FORMAT_FLOWED;
++			if (slurp_attr(line, "delsp=3D", attr) && =
+!strcasecmp(attr, "yes"))
++				tp_delsp =3D DELSP_YES;
++			else
++				tp_delsp =3D DELSP_NO;
++		}
++		else
++			tp_format =3D FORMAT_FIXED;
++	}
+	if (slurp_attr(line, "boundary=3D", boundary + 2)) {
+		memcpy(boundary, "--", 2);
+		if (content_top++ >=3D &content[MAX_BOUNDARIES]) {
+@@ -681,6 +700,8 @@ again:
+	transfer_encoding =3D TE_DONTCARE;
+	charset[0] =3D 0;
+	message_type =3D TYPE_TEXT;
++	tp_format =3D FORMAT_NONE;
++	tp_delsp =3D DELSP_NONE;
+
+	/* slurp in this section's info */
+	while (read_one_header_line(line, sizeof(line), fin))
+@@ -770,6 +791,24 @@ static int handle_filter(char *line, unsigned =20
+linesize)
+{
+	static int filter =3D 0;
+
++	if (tp_format =3D=3D FORMAT_FLOWED && !!strcmp(line, "-- \n")) {
++		char *cp =3D line;
++		while (*cp =3D=3D '>' && *cp !=3D 0)
++			cp++;
++		if (*cp =3D=3D ' ')
++			cp++;
++		line =3D cp;
++		if (!!strcmp(line, "-- \n")) {
++			while (*cp !=3D '\n' && *cp !=3D0)
++				cp++;
++			if (cp > line && *cp =3D=3D '\n' && *(cp-1) =3D=3D=
+ ' ') {
++				if (tp_delsp =3D=3D DELSP_YES)
++					*(cp-1) =3D '\0';
++				else
++					*cp =3D '\0';
++			}
++		}
++	}
+	/* filter tells us which part we left off on
+	 * a non-zero return indicates we hit a filter point
+	 */
+@@ -818,6 +857,7 @@ static void handle_body(void)
+
+		switch (transfer_encoding) {
+		case TE_BASE64:
++		case TE_QP:
+		{
+			char *op =3D line;
+
+--=20
+1.5.4.1.1281.g75df
-- 
1.5.4.1.1281.g75df

Re: [PATCH 2/2] test mailinfo rfc3676 support

[Johannes Schindelin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 874 bytes --]

Hi,

On Thu, 14 Feb 2008, Jay Soffian wrote:

> +@@ -20,6 +20,13 @@ static enum  {
> +static enum  {
> +	TYPE_TEXT, TYPE_OTHER,
> +} message_type;
> ++/* RFC 3676 Text/Plain Format and DelSp Parameters */
> ++static enum {
> ++	FORMAT_NONE, FORMAT_FIXED, FORMAT_FLOWED,
> ++} tp_format;
> ++static enum {
> ++	DELSP_NONE, DELSP_YES, DELSP_NO,
> ++} tp_delsp;
> +
> +static char charset[256];
> +static int patch_lines;

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

Re: [PATCH 2/2] test mailinfo rfc3676 support

[Jay Soffian]

On Fri, Feb 15, 2008 at 6:01 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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)

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.

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Johannes Schindelin]

Hi,

On Thu, 14 Feb 2008, Jay Soffian wrote:

> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
> index 2600847..deaf92b 100644
> --- a/builtin-mailinfo.c
> +++ b/builtin-mailinfo.c
> @@ -20,6 +20,13 @@ static enum  {
>  static enum  {
>  	TYPE_TEXT, TYPE_OTHER,
>  } message_type;
> +/* RFC 3676 Text/Plain Format and DelSp Parameters */
> +static enum {
> +	FORMAT_NONE, FORMAT_FIXED, FORMAT_FLOWED,
> +} tp_format;

Why not call it "enum message_format"?

> +static enum {
> +	DELSP_NONE, DELSP_YES, DELSP_NO,
> +} tp_delsp;

Why not call it "enum message_is_delsp"?

> @@ -193,6 +200,18 @@ static int handle_content_type(char *line)
>  
>  	if (strcasestr(line, "text/") == NULL)
>  		 message_type = TYPE_OTHER;
> +	else if (strcasestr(line, "text/plain")) {
> +		char attr[256];
> +		if (slurp_attr(line, "format=", attr) && !strcasecmp(attr, "flowed")) {
> +			tp_format = FORMAT_FLOWED;
> +			if (slurp_attr(line, "delsp=", attr) && !strcasecmp(attr, "yes"))
> +				tp_delsp = DELSP_YES;
> +			else
> +				tp_delsp = DELSP_NO;
> +		}
> +		else
> +			tp_format = FORMAT_FIXED;

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

> @@ -681,6 +700,8 @@ again:
>  	transfer_encoding = TE_DONTCARE;
>  	charset[0] = 0;
>  	message_type = TYPE_TEXT;
> +	tp_format = FORMAT_NONE;
> +	tp_delsp = DELSP_NONE;
>  
>  	/* slurp in this section's info */
>  	while (read_one_header_line(line, sizeof(line), fin))
> @@ -770,6 +791,24 @@ static int handle_filter(char *line, unsigned linesize)
>  {
>  	static int filter = 0;
>  
> +	if (tp_format == FORMAT_FLOWED && !!strcmp(line, "-- \n")) {

The !! is unnecessary; please skip it.

> +		char *cp = line;
> +		while (*cp == '>' && *cp != 0)
> +			cp++;
> +		if (*cp == ' ')
> +			cp++;
> +		line = cp;

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.

> +		if (!!strcmp(line, "-- \n")) {

The !! is unnecessary; please skip it.

> +			while (*cp != '\n' && *cp !=0)
> +				cp++;

Again, this is the job for strchrnul().

> +			if (cp > line && *cp == '\n' && *(cp-1) == ' ') {
> +				if (tp_delsp == DELSP_YES)
> +					*(cp-1) = '\0';
> +				else
> +					*cp = '\0';
> +			}

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?

> @@ -818,6 +857,7 @@ static void handle_body(void)
>  
>  		switch (transfer_encoding) {
>  		case TE_BASE64:
> +		case TE_QP:
>  		{
>  			char *op = line;

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

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Jay Soffian]

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

>  Why not call it "enum message_format"?

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.

>  Why not call it "enum message_is_delsp"?

Sure, my reasoning is same as above.

>  > @@ -193,6 +200,18 @@ static int handle_content_type(char *line)
>  >
>  >       if (strcasestr(line, "text/") == NULL)
>  >                message_type = TYPE_OTHER;
>  > +     else if (strcasestr(line, "text/plain")) {
>  > +             char attr[256];
>  > +             if (slurp_attr(line, "format=", attr) && !strcasecmp(attr, "flowed")) {
>  > +                     tp_format = FORMAT_FLOWED;
>  > +                     if (slurp_attr(line, "delsp=", attr) && !strcasecmp(attr, "yes"))
>  > +                             tp_delsp = DELSP_YES;
>  > +                     else
>  > +                             tp_delsp = DELSP_NO;
>  > +             }
>  > +             else
>  > +                     tp_format = FORMAT_FIXED;
>
>  Does that mean that the format is only set if the content type is
>  "text/plain"?

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.

>  > +     if (tp_format == FORMAT_FLOWED && !!strcmp(line, "-- \n")) {
>
>  The !! is unnecessary; please skip it.

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.

>  How about using strchrnul()?

Cool. I'd never heard of it.

>  > +                     if (cp > line && *cp == '\n' && *(cp-1) == ' ') {
>  > +                             if (tp_delsp == DELSP_YES)
>  > +                                     *(cp-1) = '\0';
>  > +                             else
>  > +                                     *cp = '\0';
>  > +                     }
>
>  Or maybe
>                                 cp[0 - (tp_delsp == DELSP_YES)] = '\0';
>
>  But maybe that is too cute.

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

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

" \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.

>  > @@ -818,6 +857,7 @@ static void handle_body(void)
>  >
>  >               switch (transfer_encoding) {
>  >               case TE_BASE64:
>  > +             case TE_QP:
>  >               {
>  >                       char *op = line;
>
>  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?

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.

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Jay Soffian]

On Fri, Feb 15, 2008 at 5:41 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>  > +             char *cp = line;
>  > +             while (*cp == '>' && *cp != 0)
>  > +                     cp++;
>
>  How about using strchrnul()?

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.

>  > +                     while (*cp != '\n' && *cp !=0)
>  > +                             cp++;
>
>  Again, this is the job for strchrnul().

Yep, here strchrnul would be fine.

j.

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Johannes Schindelin]

Hi,

On Fri, 15 Feb 2008, Jay Soffian wrote:

> On Fri, Feb 15, 2008 at 5:41 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >  > +             char *cp = line;
> >  > +             while (*cp == '>' && *cp != 0)
> >  > +                     cp++;
> >
> >  How about using strchrnul()?
> 
> 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.

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

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Junio C Hamano]

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.

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Jay Soffian]

On Fri, Feb 15, 2008 at 12:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I really do not like this.

Ugh.

>  format=flowed instructs the receivers MUA that it is Ok to
>  reflow the text when the message is presented to the user.

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.

>  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.

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.

>  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.

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 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

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

>  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."

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

>  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.

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. :-)

>  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.

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.

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Junio C Hamano]

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

> On Fri, Feb 15, 2008 at 12:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
> 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.

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.

> Existing code generally shouldn't have trailing whitespace nor
> whitespace only lines. ...

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.

> ... 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.

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.

> 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.

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.

>>  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.
>
> 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 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.

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Jay Soffian]

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

> 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.

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.

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

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.

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Junio C Hamano]

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

> 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.

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.

>> RFC3676 may be a good text communication medium.  It is just not
>> suitable for patch transfer.  Just don't use it.
>
> 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?

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.

Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

[Derek Fawcus]

On Fri, Feb 15, 2008 at 09:10:19AM -0800, Junio C Hamano wrote:
> 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.

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