[PATCH] format-patch: RFC 2047 says multi-octet character may not be split

[PATCH] format-patch: RFC 2047 says multi-octet character may not be split

[Kirill Smelkov]

Intro
-----

In 'Subject:' characters are encoded in Q encoding, as per RFC 2047, e.g.

    föö

becomes

    =?UTF-8?q?f=C3=B6=C3=B6?=

. Long encoded lines must be wrapped to be no longer than 76 bytes.

Also RFC 2047, section 5 (3) says:

    Each 'encoded-word' MUST represent an integral number of
    characters.  A multi-octet character may not be split across
    adjacent 'encoded- word's.

that means that for

    Subject: .... föö bar

encoding

    Subject: =?UTF-8?q?....=20f=C3=B6=C3=B6?=
     =?UTF-8?q?=20bar?=

is correct, and

    Subject: =?UTF-8?q?....=20f=C3=B6=C3?=      <-- NOTE ö is broken here
     =?UTF-8?q?=B6=20bar?=

is not, because "ö" character UTF-8 encoding C3 B6 is split here across
adjacent encoded words.

~~~~

As it is now, format-patch does not respect "multi-octet charactes may
not be split" rule, and so sending patches with non-english subject has
issues:

    The problematic case shows in mail readers as ".... fö?? bar".

Solution
--------

I introduce mbs_chrlen() function to compute character length in bytes
for multi-byte text according to encoding, and use it appropriately in
add_rfc2047() in pretty.

So far it works correctly only for UTF-8 encoding, because we have
infrastructure for it in place already, but other encoding could be
supported too in the future with the help of iconv. For now they all, except
UTF-8, are treated as being one-byte encodings, which was format-patch
current behaviour, with appropriate TODO put in mbs_chrlen().

Not sure whether mbs_chrlen() is a good name, but otherwise my
understanding is that the patch is ok to go in.

Thanks.

Cc: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 pretty.c                | 27 +++++++++++++++++++--------
 t/t4014-format-patch.sh | 27 ++++++++++++++-------------
 utf8.c                  | 39 +++++++++++++++++++++++++++++++++++++++
 utf8.h                  |  2 ++
 4 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/pretty.c b/pretty.c
index b57adef..c9c7ff5 100644
--- a/pretty.c
+++ b/pretty.c
@@ -345,7 +345,7 @@ static int needs_rfc2047_encoding(const char *line, int len,
 	return 0;
 }
 
-static void add_rfc2047(struct strbuf *sb, const char *line, int len,
+static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,
 		       const char *encoding, enum rfc2047_type type)
 {
 	static const int max_encoded_length = 76; /* per rfc2047 */
@@ -355,9 +355,18 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 	strbuf_grow(sb, len * 3 + strlen(encoding) + 100);
 	strbuf_addf(sb, "=?%s?q?", encoding);
 	line_len += strlen(encoding) + 5; /* 5 for =??q? */
-	for (i = 0; i < len; i++) {
-		unsigned ch = line[i] & 0xFF;
-		int is_special = is_rfc2047_special(ch, type);
+
+	while (len) {
+		/*
+		 * RFC 2047, section 5 (3):
+		 *
+		 * Each 'encoded-word' MUST represent an integral number of
+		 * characters.  A multi-octet character may not be split across
+		 * adjacent 'encoded- word's.
+		 */
+		const unsigned char *p = (const unsigned char *)line;
+		int chrlen = mbs_chrlen(&line, &len, encoding);
+		int is_special = (chrlen > 1) || is_rfc2047_special(*p, type);
 
 		/*
 		 * According to RFC 2047, we could encode the special character
@@ -367,16 +376,18 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		 * causes ' ' to be encoded as '=20', avoiding this problem.
 		 */
 
-		if (line_len + 2 + (is_special ? 3 : 1) > max_encoded_length) {
+		if (line_len + 2 + (is_special ? 3*chrlen : 1) > max_encoded_length) {
 			strbuf_addf(sb, "?=\n =?%s?q?", encoding);
 			line_len = strlen(encoding) + 5 + 1; /* =??q? plus SP */
 		}
 
 		if (is_special) {
-			strbuf_addf(sb, "=%02X", ch);
-			line_len += 3;
+			for (i = 0; i < chrlen; i++) {
+				strbuf_addf(sb, "=%02X", p[i]);
+				line_len += 3;
+			}
 		} else {
-			strbuf_addch(sb, ch);
+			strbuf_addch(sb, *p);
 			line_len++;
 		}
 	}
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 78633cb..b993dae 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -837,25 +837,26 @@ Subject: [PATCH] =?UTF-8?q?f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
- =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar?=
 EOF
 test_expect_success 'format-patch wraps extremely long subject (rfc2047)' '
 	rm -rf patches/ &&
diff --git a/utf8.c b/utf8.c
index 8f6e84b..7911b58 100644
--- a/utf8.c
+++ b/utf8.c
@@ -531,3 +531,42 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 	return out;
 }
 #endif
+
+/*
+ * Returns first character length in bytes for multi-byte `text` according to
+ * `encoding`.
+ *
+ * - The `text` pointer is updated to point at the next character.
+ * - When `remainder_p` is not NULL, on entry `*remainder_p` is how much bytes
+ *   we can consume from text, and on exit `*remainder_p` is reduced by returned
+ *   character length. Otherwise `text` is treated as limited by NUL.
+ */
+int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding)
+{
+	int chrlen;
+	const char *p = *text;
+	size_t r = (remainder_p ? *remainder_p : INT_MAX);
+
+	if (r < 1)
+		return 0;
+
+	if (is_encoding_utf8(encoding)) {
+		pick_one_utf8_char(&p, &r);
+
+		chrlen = p ? (p - *text)
+			   : 1 /* not valid UTF-8 -> raw byte sequence */;
+	}
+	else {
+		/* TODO use iconv to decode one char and obtain its chrlen
+		 *
+		 * for now, let's treat encodings != UTF-8 as one-byte
+		 */
+		chrlen = 1;
+	}
+
+	*text += chrlen;
+	if (remainder_p)
+		*remainder_p -= chrlen;
+
+	return chrlen;
+}
diff --git a/utf8.h b/utf8.h
index 501b2bd..1f8ecad 100644
--- a/utf8.h
+++ b/utf8.h
@@ -22,4 +22,6 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 #define reencode_string(a,b,c) NULL
 #endif
 
+int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);
+
 #endif
-- 
1.8.2.rc2.353.gd2380b4

Re: [PATCH] format-patch: RFC 2047 says multi-octet character may not be split

[Kirill Smelkov]

Junio,

On Wed, Mar 06, 2013 at 09:47:53AM -0800, Junio C Hamano wrote:
> Kirill Smelkov <kirr@mns.spb.ru> writes:
> 
> > Intro
> > -----
> 
> Drop this.  We know the beginning part is "intro" already ;-)

:)


> >     Subject: .... föö bar
> >
> > encoding
> >
> >     Subject: =?UTF-8?q?....=20f=C3=B6=C3=B6?=
> >      =?UTF-8?q?=20bar?=
> >
> > is correct, and
> >
> >     Subject: =?UTF-8?q?....=20f=C3=B6=C3?=      <-- NOTE ö is broken here
> >      =?UTF-8?q?=B6=20bar?=
> >
> > is not, because "ö" character UTF-8 encoding C3 B6 is split here across
> > adjacent encoded words.
> 
> The above is an important part to keep in the log message.
> Everything above that I snipped can be left out for brevity.
> 
> > As it is now, format-patch does not respect "multi-octet charactes may
> > not be split" rule, and so sending patches with non-english subject has
> > issues:
> >
> >     The problematic case shows in mail readers as ".... fö?? bar".
> 
> But the log message lacks crucial bits of information before you
> start talking about your solution.  Where does it go wrong?  What
> did the earlier attempt bafc478..41dd00bad miss?  This can be fixed
> trivially by replacing the above (and the "solution" section),
> perhaps like this:
> 
>     Even though an earlier attempt (bafc478..41dd00bad) cleaned
>     up RFC 2047 encoding, pretty.c::add_rfc2047() still decides
>     where to split the output line by going through the input
>     one byte at a time, and potentially splits a character in
>     the middle.  A subject line may end up showing like this:
> 
>          The problematic case shows in mail readers as ".... fö?? bar".
> 
>     Instead, make the loop grab one _character_ at a time and
>     determine its output length to see where to break the output
>     line.  Note that this version only knows about UTF-8, but the
>     logic to grab one character is abstracted out in mbs_chrlen()
>     function to make it possible to extend it to other encodings.

I agree my description was messy and thanks for reworking and clarifying
it - your version is much better.

I'll use its slight variation for the updated patch.


> > +	while (len) {
> > +		/*
> > +		 * RFC 2047, section 5 (3):
> > +		 *
> > +		 * Each 'encoded-word' MUST represent an integral number of
> > +		 * characters.  A multi-octet character may not be split across
> > +		 * adjacent 'encoded- word's.
> > +		 */
> > +		const unsigned char *p = (const unsigned char *)line;
> > +		int chrlen = mbs_chrlen(&line, &len, encoding);
> > +		int is_special = (chrlen > 1) || is_rfc2047_special(*p, type);
> >  
> >  		/*
> >  		 * According to RFC 2047, we could encode the special character
> > @@ -367,16 +376,18 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
> >  		 * causes ' ' to be encoded as '=20', avoiding this problem.
> >  		 */
> >  
> > +		if (line_len + 2 + (is_special ? 3*chrlen : 1) > max_encoded_length) {
> 
> Always have SP around binary operators such as '*' (multiplication).

ok, but note that's just a matter of style, and if one is used to code
formulas, _not_ having SP is more convenient sometimes.


> I would actually suggest adding an extra variable "encoded_len" and
> do something like this:
> 
> 	/* "=%02X" times num_char, or the byte itself */
> 	encoded_len = is_special ? 3 * num_char : 1;
>         if (max_encoded_length < line_len + 2 + encoded_len) {
>         	/* It will not fit---break the line */
> 		...

Right. Actually if we add encoded_len, adding encoded_fmt is tempting

    const char *encoded_fmt = is_special ? "=%02X"    : "%c";

and then encoding part simplifies to just unconditional

    for (i = 0; i < chrlen; i++)
            strbuf_addf(sb, encoded_fmt, p[i]);
    line_len += encoded_len;


> You may also want to say what the hardcoded "2" is about in the
> comment there.

ok.


> > diff --git a/utf8.c b/utf8.c
> > index 8f6e84b..7911b58 100644
> > --- a/utf8.c
> > +++ b/utf8.c
> > @@ -531,3 +531,42 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
> >  	return out;
> >  }
> >  #endif
> > +
> > +/*
> > + * Returns first character length in bytes for multi-byte `text` according to
> > + * `encoding`.
> > + *
> > + * - The `text` pointer is updated to point at the next character.
> > + * - When `remainder_p` is not NULL, on entry `*remainder_p` is how much bytes
> > + *   we can consume from text, and on exit `*remainder_p` is reduced by returned
> > + *   character length. Otherwise `text` is treated as limited by NUL.
> > + */
> > +int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding)
> > +{
> > +	int chrlen;
> > +	const char *p = *text;
> > +	size_t r = (remainder_p ? *remainder_p : INT_MAX);
> 
> Ugly, and more importantly I suspect this is wrong because size_t is
> not signed and INT_MAX is.

Why is it ugly? There is similiar snippet in pick_one_utf8_char():

        /*
         * A caller that assumes NUL terminated text can choose
         * not to bother with the remainder length.  We will
         * stop at the first NUL.
         */ 
        remainder = (remainder_p ? *remainder_p : 999);

only ad-hoc 999 is used there.

I agree about INT_MAX being signed - my mistake - better change it to
SIZE_MAX or ((size_t)-1) for portability, but otherwise the construct is
imho ok. I'll change to SIZE_MAX since it is alredy used in Git.

Computing r in the beginning simplifies following code.

> > +	if (r < 1)
> > +		return 0;
> > +
> > +	if (is_encoding_utf8(encoding)) {
> > +		pick_one_utf8_char(&p, &r);
> > +
> > +		chrlen = p ? (p - *text)
> > +			   : 1 /* not valid UTF-8 -> raw byte sequence */;
> > +	}
> > +	else {
> > +		/* TODO use iconv to decode one char and obtain its chrlen
> > +		 *
> > +		 * for now, let's treat encodings != UTF-8 as one-byte
> > +		 */
> > +		chrlen = 1;
> 
> 	/*
>          * We format our multi-line
>          * comments like this
>          */

ok, I agree.


> Thanks.

Thanks too,
Kirill


Interdiff and updated patch follows:

diff -u b/pretty.c b/pretty.c
--- b/pretty.c
+++ b/pretty.c
@@ -369,4 +369,8 @@
 		int is_special = (chrlen > 1) || is_rfc2047_special(*p, type);
 
+		/* "=%02X" * chrlen, or the byte itself */
+		const char *encoded_fmt = is_special ? "=%02X"    : "%c";
+		int	    encoded_len = is_special ? 3 * chrlen : 1;
+
 		/*
 		 * According to RFC 2047, we could encode the special character
@@ -376,20 +380,15 @@
 		 * causes ' ' to be encoded as '=20', avoiding this problem.
 		 */
 
-		if (line_len + 2 + (is_special ? 3*chrlen : 1) > max_encoded_length) {
+		if (line_len + encoded_len + /* ?= */2 > max_encoded_length) {
+			/* It will not fit---break the line */
 			strbuf_addf(sb, "?=\n =?%s?q?", encoding);
 			line_len = strlen(encoding) + 5 + 1; /* =??q? plus SP */
 		}
 
-		if (is_special) {
-			for (i = 0; i < chrlen; i++) {
-				strbuf_addf(sb, "=%02X", p[i]);
-				line_len += 3;
-			}
-		} else {
-			strbuf_addch(sb, *p);
-			line_len++;
-		}
+		for (i = 0; i < chrlen; i++)
+			strbuf_addf(sb, encoded_fmt, p[i]);
+		line_len += encoded_len;
 	}
 	strbuf_addstr(sb, "?=");
 }
diff -u b/utf8.c b/utf8.c
--- b/utf8.c
+++ b/utf8.c
@@ -545,7 +545,7 @@
 {
 	int chrlen;
 	const char *p = *text;
-	size_t r = (remainder_p ? *remainder_p : INT_MAX);
+	size_t r = (remainder_p ? *remainder_p : SIZE_MAX);
 
 	if (r < 1)
 		return 0;
@@ -557,8 +557,8 @@
 			   : 1 /* not valid UTF-8 -> raw byte sequence */;
 	}
 	else {
-		/* TODO use iconv to decode one char and obtain its chrlen
-		 *
+		/*
+		 * TODO use iconv to decode one char and obtain its chrlen
 		 * for now, let's treat encodings != UTF-8 as one-byte
 		 */
 		chrlen = 1;

---- 8< ----
From 46b9cddc63c07cb5513cfbf6d20aaaa98c66bcdf Mon Sep 17 00:00:00 2001
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Wed, 6 Mar 2013 14:28:46 +0400
Subject: [PATCH v2] format-patch: RFC 2047 says multi-octet character may not be split

Even though an earlier attempt (bafc478..41dd00bad) cleaned
up RFC 2047 encoding, pretty.c::add_rfc2047() still decides
where to split the output line by going through the input
one byte at a time, and potentially splits a character in
the middle.  A subject line may end up showing like this:

     ".... fö?? bar".   (instead of  ".... föö bar".)

if split incorrectly.

RFC 2047, section 5 (3) explicitly forbids such beaviour

    Each 'encoded-word' MUST represent an integral number of
    characters.  A multi-octet character may not be split across
    adjacent 'encoded- word's.

that means that e.g. for

    Subject: .... föö bar

encoding

    Subject: =?UTF-8?q?....=20f=C3=B6=C3=B6?=
     =?UTF-8?q?=20bar?=

is correct, and

    Subject: =?UTF-8?q?....=20f=C3=B6=C3?=      <-- NOTE ö is broken here
     =?UTF-8?q?=B6=20bar?=

is not, because "ö" character UTF-8 encoding C3 B6 is split here across
adjacent encoded words.

To fix the problem, make the loop grab one _character_ at a time and
determine its output length to see where to break the output line.  Note
that this version only knows about UTF-8, but the logic to grab one
character is abstracted out in mbs_chrlen() function to make it possible
to extend it to other encodings with the help of iconv in the future.

(With help from Junio C Hamano <gitster@pobox.com>)
Cc: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 pretty.c                | 34 ++++++++++++++++++++++------------
 t/t4014-format-patch.sh | 27 ++++++++++++++-------------
 utf8.c                  | 39 +++++++++++++++++++++++++++++++++++++++
 utf8.h                  |  2 ++
 4 files changed, 77 insertions(+), 25 deletions(-)

diff --git a/pretty.c b/pretty.c
index b57adef..c5fae69 100644
--- a/pretty.c
+++ b/pretty.c
@@ -345,7 +345,7 @@ static int needs_rfc2047_encoding(const char *line, int len,
 	return 0;
 }
 
-static void add_rfc2047(struct strbuf *sb, const char *line, int len,
+static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,
 		       const char *encoding, enum rfc2047_type type)
 {
 	static const int max_encoded_length = 76; /* per rfc2047 */
@@ -355,9 +355,22 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 	strbuf_grow(sb, len * 3 + strlen(encoding) + 100);
 	strbuf_addf(sb, "=?%s?q?", encoding);
 	line_len += strlen(encoding) + 5; /* 5 for =??q? */
-	for (i = 0; i < len; i++) {
-		unsigned ch = line[i] & 0xFF;
-		int is_special = is_rfc2047_special(ch, type);
+
+	while (len) {
+		/*
+		 * RFC 2047, section 5 (3):
+		 *
+		 * Each 'encoded-word' MUST represent an integral number of
+		 * characters.  A multi-octet character may not be split across
+		 * adjacent 'encoded- word's.
+		 */
+		const unsigned char *p = (const unsigned char *)line;
+		int chrlen = mbs_chrlen(&line, &len, encoding);
+		int is_special = (chrlen > 1) || is_rfc2047_special(*p, type);
+
+		/* "=%02X" * chrlen, or the byte itself */
+		const char *encoded_fmt = is_special ? "=%02X"    : "%c";
+		int	    encoded_len = is_special ? 3 * chrlen : 1;
 
 		/*
 		 * According to RFC 2047, we could encode the special character
@@ -367,18 +380,15 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		 * causes ' ' to be encoded as '=20', avoiding this problem.
 		 */
 
-		if (line_len + 2 + (is_special ? 3 : 1) > max_encoded_length) {
+		if (line_len + encoded_len + /* ?= */2 > max_encoded_length) {
+			/* It will not fit---break the line */
 			strbuf_addf(sb, "?=\n =?%s?q?", encoding);
 			line_len = strlen(encoding) + 5 + 1; /* =??q? plus SP */
 		}
 
-		if (is_special) {
-			strbuf_addf(sb, "=%02X", ch);
-			line_len += 3;
-		} else {
-			strbuf_addch(sb, ch);
-			line_len++;
-		}
+		for (i = 0; i < chrlen; i++)
+			strbuf_addf(sb, encoded_fmt, p[i]);
+		line_len += encoded_len;
 	}
 	strbuf_addstr(sb, "?=");
 }
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 78633cb..b993dae 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -837,25 +837,26 @@ Subject: [PATCH] =?UTF-8?q?f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
- =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar?=
 EOF
 test_expect_success 'format-patch wraps extremely long subject (rfc2047)' '
 	rm -rf patches/ &&
diff --git a/utf8.c b/utf8.c
index 8f6e84b..7f64857 100644
--- a/utf8.c
+++ b/utf8.c
@@ -531,3 +531,42 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 	return out;
 }
 #endif
+
+/*
+ * Returns first character length in bytes for multi-byte `text` according to
+ * `encoding`.
+ *
+ * - The `text` pointer is updated to point at the next character.
+ * - When `remainder_p` is not NULL, on entry `*remainder_p` is how much bytes
+ *   we can consume from text, and on exit `*remainder_p` is reduced by returned
+ *   character length. Otherwise `text` is treated as limited by NUL.
+ */
+int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding)
+{
+	int chrlen;
+	const char *p = *text;
+	size_t r = (remainder_p ? *remainder_p : SIZE_MAX);
+
+	if (r < 1)
+		return 0;
+
+	if (is_encoding_utf8(encoding)) {
+		pick_one_utf8_char(&p, &r);
+
+		chrlen = p ? (p - *text)
+			   : 1 /* not valid UTF-8 -> raw byte sequence */;
+	}
+	else {
+		/*
+		 * TODO use iconv to decode one char and obtain its chrlen
+		 * for now, let's treat encodings != UTF-8 as one-byte
+		 */
+		chrlen = 1;
+	}
+
+	*text += chrlen;
+	if (remainder_p)
+		*remainder_p -= chrlen;
+
+	return chrlen;
+}
diff --git a/utf8.h b/utf8.h
index 501b2bd..1f8ecad 100644
--- a/utf8.h
+++ b/utf8.h
@@ -22,4 +22,6 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 #define reencode_string(a,b,c) NULL
 #endif
 
+int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);
+
 #endif
-- 
1.8.2.rc2.353.gd2380b4

Re: [PATCH] format-patch: RFC 2047 says multi-octet character may not be split

[Kirill Smelkov]

On Thu, Mar 07, 2013 at 10:05:30AM -0800, Junio C Hamano wrote:
> Kirill Smelkov <kirr@mns.spb.ru> writes:
> 
> >> > @@ -367,16 +376,18 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
> >> >  		 * causes ' ' to be encoded as '=20', avoiding this problem.
> >> >  		 */
> >> >  
> >> > +		if (line_len + 2 + (is_special ? 3*chrlen : 1) > max_encoded_length) {
> >> 
> >> Always have SP around binary operators such as '*' (multiplication).
> >
> > ok, but note that's just a matter of style, and if one is used to code
> > formulas,...
> 
> Well, when working on a project with others, what _you_ are used to
> does not matter.  Also please never call coding style "just a matter
> of".  Keeping things consistent with the style around the area is a
> prerequisite.
> 
>    When you have time:
>    Cf. https://www.youtube.com/watch?feature=player_embedded&v=fMeH7wqOwXA

Junio, what Greg says here is all known and good and respected. I agree
coding style is not a "just a matter of" and is important to follow for
project to stay consisting. My note here was just a sentiment about
spaces around operators, which I didn't know was in the coding style
because it is not in Documentation/CodingGuidelines, and especially if
the project sometimes uses my style

    *offset = 60*off;                                       date.c      5e2a78a4
    diff += 7*n;                                            date.c      6b7b0427
    sub_size < 2*window && i+1 < delta_search_threads       pack_objects.c  bf874896


But anyway, I'm ok with any style the project chooses - it's not so important
for me to insist here, so let it be "3 * chrlen" and lets forget about it.


> > Actually if we add encoded_len, adding encoded_fmt is tempting
> >
> >     const char *encoded_fmt = is_special ? "=%02X"    : "%c";
> >
> > and then encoding part simplifies to just unconditional
> >
> >     for (i = 0; i < chrlen; i++)
> >             strbuf_addf(sb, encoded_fmt, p[i]);
> >     line_len += encoded_len;
> 
> Sounds very sensible ;-)

Thanks.

> >  		 * for now, let's treat encodings != UTF-8 as one-byte
> >  		 */
> >  		chrlen = 1;
> >
> > ---- 8< ----
> > From 46b9cddc63c07cb5513cfbf6d20aaaa98c66bcdf Mon Sep 17 00:00:00 2001
> > From: Kirill Smelkov <kirr@mns.spb.ru>
> > Date: Wed, 6 Mar 2013 14:28:46 +0400
> > Subject: [PATCH v2] format-patch: RFC 2047 says multi-octet character may not be split
> 
> Good use of scissors line; but please drop these four lines after
> it.  The first is unwanted and the rest are redundant.
> 
> > Even though an earlier attempt (bafc478..41dd00bad) cleaned
> > up RFC 2047 encoding, pretty.c::add_rfc2047() still decides
> > where to split the output line by going through the input
> > ...
> > @@ -367,18 +380,15 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
> >  		 * causes ' ' to be encoded as '=20', avoiding this problem.
> >  		 */
> >  
> > -		if (line_len + 2 + (is_special ? 3 : 1) > max_encoded_length) {
> > +		if (line_len + encoded_len + /* ?= */2 > max_encoded_length) {
> > +			/* It will not fit---break the line */
> 
> It doesn't look much clearer with /* ?= */ unless we say something
> that contains the word "close", e.g. "?= to close the encoded part".
> Maybe it is just me.

How about

        if (line_len + encoded_len + 2 > max_encoded_length) {
                /* It won't fit with trailing "?=" --- break the line */

?

> 
> > -		if (is_special) {
> > -			strbuf_addf(sb, "=%02X", ch);
> > -			line_len += 3;
> > -		} else {
> > -			strbuf_addch(sb, ch);
> > -			line_len++;
> > -		}
> > +		for (i = 0; i < chrlen; i++)
> > +			strbuf_addf(sb, encoded_fmt, p[i]);
> > +		line_len += encoded_len;
> 
> Nice code reduction.

Thanks.

Interdiff and updated patch follow. Note I'm sending this from home, so
'From:' line after scissors is kept as necessary.

Kirill

P.S. sorry for the delay - I harmed my arm yesterday.


diff --git a/pretty.c b/pretty.c
index 8fce619..41f04e6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -380,8 +380,8 @@ static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,
 		 * causes ' ' to be encoded as '=20', avoiding this problem.
 		 */
 
-		if (line_len + encoded_len + /* ?= */2 > max_encoded_length) {
-			/* It will not fit---break the line */
+		if (line_len + encoded_len + 2 > max_encoded_length) {
+			/* It won't fit with trailing "?=" --- break the line */
 			strbuf_addf(sb, "?=\n =?%s?q?", encoding);
 			line_len = strlen(encoding) + 5 + 1; /* =??q? plus SP */
 		}

---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>
 split

Even though an earlier attempt (bafc478..41dd00bad) cleaned
up RFC 2047 encoding, pretty.c::add_rfc2047() still decides
where to split the output line by going through the input
one byte at a time, and potentially splits a character in
the middle.  A subject line may end up showing like this:

     ".... fö?? bar".   (instead of  ".... föö bar".)

if split incorrectly.

RFC 2047, section 5 (3) explicitly forbids such behaviour

    Each 'encoded-word' MUST represent an integral number of
    characters.  A multi-octet character may not be split across
    adjacent 'encoded- word's.

that means that e.g. for

    Subject: .... föö bar

encoding

    Subject: =?UTF-8?q?....=20f=C3=B6=C3=B6?=
     =?UTF-8?q?=20bar?=

is correct, and

    Subject: =?UTF-8?q?....=20f=C3=B6=C3?=      <-- NOTE ö is broken here
     =?UTF-8?q?=B6=20bar?=

is not, because "ö" character UTF-8 encoding C3 B6 is split here across
adjacent encoded words.

To fix the problem, make the loop grab one _character_ at a time and
determine its output length to see where to break the output line.  Note
that this version only knows about UTF-8, but the logic to grab one
character is abstracted out in mbs_chrlen() function to make it possible
to extend it to other encodings with the help of iconv in the future.

(With help from Junio C Hamano <gitster@pobox.com>)
Cc: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 pretty.c                | 34 ++++++++++++++++++++++------------
 t/t4014-format-patch.sh | 27 ++++++++++++++-------------
 utf8.c                  | 39 +++++++++++++++++++++++++++++++++++++++
 utf8.h                  |  2 ++
 4 files changed, 77 insertions(+), 25 deletions(-)

diff --git a/pretty.c b/pretty.c
index b57adef..41f04e6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -345,7 +345,7 @@ static int needs_rfc2047_encoding(const char *line, int len,
 	return 0;
 }
 
-static void add_rfc2047(struct strbuf *sb, const char *line, int len,
+static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,
 		       const char *encoding, enum rfc2047_type type)
 {
 	static const int max_encoded_length = 76; /* per rfc2047 */
@@ -355,9 +355,22 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 	strbuf_grow(sb, len * 3 + strlen(encoding) + 100);
 	strbuf_addf(sb, "=?%s?q?", encoding);
 	line_len += strlen(encoding) + 5; /* 5 for =??q? */
-	for (i = 0; i < len; i++) {
-		unsigned ch = line[i] & 0xFF;
-		int is_special = is_rfc2047_special(ch, type);
+
+	while (len) {
+		/*
+		 * RFC 2047, section 5 (3):
+		 *
+		 * Each 'encoded-word' MUST represent an integral number of
+		 * characters.  A multi-octet character may not be split across
+		 * adjacent 'encoded- word's.
+		 */
+		const unsigned char *p = (const unsigned char *)line;
+		int chrlen = mbs_chrlen(&line, &len, encoding);
+		int is_special = (chrlen > 1) || is_rfc2047_special(*p, type);
+
+		/* "=%02X" * chrlen, or the byte itself */
+		const char *encoded_fmt = is_special ? "=%02X"    : "%c";
+		int	    encoded_len = is_special ? 3 * chrlen : 1;
 
 		/*
 		 * According to RFC 2047, we could encode the special character
@@ -367,18 +380,15 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		 * causes ' ' to be encoded as '=20', avoiding this problem.
 		 */
 
-		if (line_len + 2 + (is_special ? 3 : 1) > max_encoded_length) {
+		if (line_len + encoded_len + 2 > max_encoded_length) {
+			/* It won't fit with trailing "?=" --- break the line */
 			strbuf_addf(sb, "?=\n =?%s?q?", encoding);
 			line_len = strlen(encoding) + 5 + 1; /* =??q? plus SP */
 		}
 
-		if (is_special) {
-			strbuf_addf(sb, "=%02X", ch);
-			line_len += 3;
-		} else {
-			strbuf_addch(sb, ch);
-			line_len++;
-		}
+		for (i = 0; i < chrlen; i++)
+			strbuf_addf(sb, encoded_fmt, p[i]);
+		line_len += encoded_len;
 	}
 	strbuf_addstr(sb, "?=");
 }
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 78633cb..b993dae 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -837,25 +837,26 @@ Subject: [PATCH] =?UTF-8?q?f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
- =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar?=
 EOF
 test_expect_success 'format-patch wraps extremely long subject (rfc2047)' '
 	rm -rf patches/ &&
diff --git a/utf8.c b/utf8.c
index 8f6e84b..7f64857 100644
--- a/utf8.c
+++ b/utf8.c
@@ -531,3 +531,42 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 	return out;
 }
 #endif
+
+/*
+ * Returns first character length in bytes for multi-byte `text` according to
+ * `encoding`.
+ *
+ * - The `text` pointer is updated to point at the next character.
+ * - When `remainder_p` is not NULL, on entry `*remainder_p` is how much bytes
+ *   we can consume from text, and on exit `*remainder_p` is reduced by returned
+ *   character length. Otherwise `text` is treated as limited by NUL.
+ */
+int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding)
+{
+	int chrlen;
+	const char *p = *text;
+	size_t r = (remainder_p ? *remainder_p : SIZE_MAX);
+
+	if (r < 1)
+		return 0;
+
+	if (is_encoding_utf8(encoding)) {
+		pick_one_utf8_char(&p, &r);
+
+		chrlen = p ? (p - *text)
+			   : 1 /* not valid UTF-8 -> raw byte sequence */;
+	}
+	else {
+		/*
+		 * TODO use iconv to decode one char and obtain its chrlen
+		 * for now, let's treat encodings != UTF-8 as one-byte
+		 */
+		chrlen = 1;
+	}
+
+	*text += chrlen;
+	if (remainder_p)
+		*remainder_p -= chrlen;
+
+	return chrlen;
+}
diff --git a/utf8.h b/utf8.h
index 501b2bd..1f8ecad 100644
--- a/utf8.h
+++ b/utf8.h
@@ -22,4 +22,6 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 #define reencode_string(a,b,c) NULL
 #endif
 
+int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);
+
 #endif
-- 
1.8.2.rc2.366.g3bc8dda

Re: [PATCH] format-patch: RFC 2047 says multi-octet character may not be split

[Kirill Smelkov]

On Sat, Mar 09, 2013 at 07:27:23PM +0400, Kirill Smelkov wrote:
> ---- 8< ----
> From: Kirill Smelkov <kirr@mns.spb.ru>
>  split

Sorry for the confusion...

---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>

Even though an earlier attempt (bafc478..41dd00bad) cleaned
up RFC 2047 encoding, pretty.c::add_rfc2047() still decides
where to split the output line by going through the input
one byte at a time, and potentially splits a character in
the middle.  A subject line may end up showing like this:

     ".... fö?? bar".   (instead of  ".... föö bar".)

if split incorrectly.

RFC 2047, section 5 (3) explicitly forbids such beaviour

    Each 'encoded-word' MUST represent an integral number of
    characters.  A multi-octet character may not be split across
    adjacent 'encoded- word's.

that means that e.g. for

    Subject: .... föö bar

encoding

    Subject: =?UTF-8?q?....=20f=C3=B6=C3=B6?=
     =?UTF-8?q?=20bar?=

is correct, and

    Subject: =?UTF-8?q?....=20f=C3=B6=C3?=      <-- NOTE ö is broken here
     =?UTF-8?q?=B6=20bar?=

is not, because "ö" character UTF-8 encoding C3 B6 is split here across
adjacent encoded words.

To fix the problem, make the loop grab one _character_ at a time and
determine its output length to see where to break the output line.  Note
that this version only knows about UTF-8, but the logic to grab one
character is abstracted out in mbs_chrlen() function to make it possible
to extend it to other encodings with the help of iconv in the future.

(With help from Junio C Hamano <gitster@pobox.com>)
Cc: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 pretty.c                | 34 ++++++++++++++++++++++------------
 t/t4014-format-patch.sh | 27 ++++++++++++++-------------
 utf8.c                  | 39 +++++++++++++++++++++++++++++++++++++++
 utf8.h                  |  2 ++
 4 files changed, 77 insertions(+), 25 deletions(-)

diff --git a/pretty.c b/pretty.c
index b57adef..41f04e6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -345,7 +345,7 @@ static int needs_rfc2047_encoding(const char *line, int len,
 	return 0;
 }
 
-static void add_rfc2047(struct strbuf *sb, const char *line, int len,
+static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,
 		       const char *encoding, enum rfc2047_type type)
 {
 	static const int max_encoded_length = 76; /* per rfc2047 */
@@ -355,9 +355,22 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 	strbuf_grow(sb, len * 3 + strlen(encoding) + 100);
 	strbuf_addf(sb, "=?%s?q?", encoding);
 	line_len += strlen(encoding) + 5; /* 5 for =??q? */
-	for (i = 0; i < len; i++) {
-		unsigned ch = line[i] & 0xFF;
-		int is_special = is_rfc2047_special(ch, type);
+
+	while (len) {
+		/*
+		 * RFC 2047, section 5 (3):
+		 *
+		 * Each 'encoded-word' MUST represent an integral number of
+		 * characters.  A multi-octet character may not be split across
+		 * adjacent 'encoded- word's.
+		 */
+		const unsigned char *p = (const unsigned char *)line;
+		int chrlen = mbs_chrlen(&line, &len, encoding);
+		int is_special = (chrlen > 1) || is_rfc2047_special(*p, type);
+
+		/* "=%02X" * chrlen, or the byte itself */
+		const char *encoded_fmt = is_special ? "=%02X"    : "%c";
+		int	    encoded_len = is_special ? 3 * chrlen : 1;
 
 		/*
 		 * According to RFC 2047, we could encode the special character
@@ -367,18 +380,15 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		 * causes ' ' to be encoded as '=20', avoiding this problem.
 		 */
 
-		if (line_len + 2 + (is_special ? 3 : 1) > max_encoded_length) {
+		if (line_len + encoded_len + 2 > max_encoded_length) {
+			/* It won't fit with trailing "?=" --- break the line */
 			strbuf_addf(sb, "?=\n =?%s?q?", encoding);
 			line_len = strlen(encoding) + 5 + 1; /* =??q? plus SP */
 		}
 
-		if (is_special) {
-			strbuf_addf(sb, "=%02X", ch);
-			line_len += 3;
-		} else {
-			strbuf_addch(sb, ch);
-			line_len++;
-		}
+		for (i = 0; i < chrlen; i++)
+			strbuf_addf(sb, encoded_fmt, p[i]);
+		line_len += encoded_len;
 	}
 	strbuf_addstr(sb, "?=");
 }
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 78633cb..b993dae 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -837,25 +837,26 @@ Subject: [PATCH] =?UTF-8?q?f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
  =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
  =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
  =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
- =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
- =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
- =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
- =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar?=
 EOF
 test_expect_success 'format-patch wraps extremely long subject (rfc2047)' '
 	rm -rf patches/ &&
diff --git a/utf8.c b/utf8.c
index 8f6e84b..7f64857 100644
--- a/utf8.c
+++ b/utf8.c
@@ -531,3 +531,42 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 	return out;
 }
 #endif
+
+/*
+ * Returns first character length in bytes for multi-byte `text` according to
+ * `encoding`.
+ *
+ * - The `text` pointer is updated to point at the next character.
+ * - When `remainder_p` is not NULL, on entry `*remainder_p` is how much bytes
+ *   we can consume from text, and on exit `*remainder_p` is reduced by returned
+ *   character length. Otherwise `text` is treated as limited by NUL.
+ */
+int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding)
+{
+	int chrlen;
+	const char *p = *text;
+	size_t r = (remainder_p ? *remainder_p : SIZE_MAX);
+
+	if (r < 1)
+		return 0;
+
+	if (is_encoding_utf8(encoding)) {
+		pick_one_utf8_char(&p, &r);
+
+		chrlen = p ? (p - *text)
+			   : 1 /* not valid UTF-8 -> raw byte sequence */;
+	}
+	else {
+		/*
+		 * TODO use iconv to decode one char and obtain its chrlen
+		 * for now, let's treat encodings != UTF-8 as one-byte
+		 */
+		chrlen = 1;
+	}
+
+	*text += chrlen;
+	if (remainder_p)
+		*remainder_p -= chrlen;
+
+	return chrlen;
+}
diff --git a/utf8.h b/utf8.h
index 501b2bd..1f8ecad 100644
--- a/utf8.h
+++ b/utf8.h
@@ -22,4 +22,6 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 #define reencode_string(a,b,c) NULL
 #endif
 
+int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);
+
 #endif
-- 
1.8.2.rc2.366.g3bc8dda

Re: [PATCH] format-patch: RFC 2047 says multi-octet character may not be split

[Kirill Smelkov]

On Sat, Mar 09, 2013 at 11:07:19AM -0800, Junio C Hamano wrote:
> Kirill Smelkov <kirr@navytux.spb.ru> writes:
> > P.S. sorry for the delay - I harmed my arm yesterday.
> 
> Ouch. Take care and be well soon.

Thanks, and thanks fr accepting the patch.