Re: [PATCH] format-patch: RFC 2047 says multi-octet character may not be split
On Sat, Mar 09, 2013 at 11:07:19AM -0800, Junio C Hamano wrote: > Kirill Smelkov 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: RFC 2047 says multi-octet character may not be split
On Sat, Mar 09, 2013 at 07:27:23PM +0400, Kirill Smelkov wrote:
> 8<
> From: Kirill Smelkov
> split
Sorry for the confusion...
8<
From: Kirill Smelkov
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 )
Cc: Jan H. Schönherr
Signed-off-by: Kirill Smelkov
---
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=
Re: [PATCH] format-patch: RFC 2047 says multi-octet character may not be split
On Thu, Mar 07, 2013 at 10:05:30AM -0800, Junio C Hamano wrote:
> Kirill Smelkov 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 46b9cddc63c07cb5513cfbf6d2098c66bcdf Mon Sep 17 00:00:00 2001
> > From: Kirill Smelkov
> > 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
split
Even though an earlier attem
Re: [PATCH] format-patch: RFC 2047 says multi-octet character may not be split
Junio,
On Wed, Mar 06, 2013 at 09:47:53AM -0800, Junio C Hamano wrote:
> Kirill Smelkov 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
[PATCH] format-patch: RFC 2047 says multi-octet character may not be split
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
Signed-off-by: Kirill Smelkov
---
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
