a bug about format-patch of multibyte characters comment

a bug about format-patch of multibyte characters comment

[xiaozhu]

Hi,

I found a bug when I use format-patch to export a patch which contains comment with
some multibyte characters. I also found the relation source, but I can't understand
the source clearly, so I think I need a help to know how can I fix it.

At first, the symptom.

I commit a fix to my repository with comment like following:
-----------------------------------------------------
XXXXXXXXXXXX
YYYYYY
-----------------------------------------------------

two lines of multibyte language comment.

then I use format-patch to export this fix, I get a patch file like following:

------------------------------------------------------------------------------
 From d3532c3263a02a2367a3aa5c9cc3f0bd738b79b1 Mon Sep 17 00:00:00 2001
From: xz <xz>
Date: Fri, 11 Feb 2011 21:30:35 +0900
Subject: [PATCH] =?UTF-8?q?=E6=97=A5=E6=9C=AC=E8=AA=9E=E3=81=8C=E5=A4=A7=E4=B8=88=E5=A4=AB
=20=E6=94=B9=E8=A1=8C=E3=81=99=E3=82=8B?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
  testfile.txt |    4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/testfile.txt b/testfile.txt
index 1e5d832..da982fd 100644
--- a/testfile.txt
+++ b/testfile.txt
@@ -1 +1,3 @@
-sadfasdf
\ No newline at end of file
+sadfasdf

..........

-------------------------------------------------------------------------------

If I use am to apply this patch, am can't analyze the comment correctly, then the
committed comment will become
"=?UTF-8?q?=E6=97=A5=E6=9C=AC=E8=AA=9E=E3=81=8C=E5=A4=A7=E4=B8=88=E5=A4=AB".

Above is the symptom.

Then I did some try, I modify the comment to 3 lines:
-----------------------------------------------------
XXXXXXXXXXXX

YYYYYY
-----------------------------------------------------

add a empty line, then I get a patch like following:
------------------------------------------------------------------------------
 From d3532c3263a02a2367a3aa5c9cc3f0bd738b79b1 Mon Sep 17 00:00:00 2001
From: xz <xz>
Date: Fri, 11 Feb 2011 21:30:35 +0900
Subject: [PATCH] =?UTF-8?q?=E6=97=A5=E6=9C=AC=E8=AA=9E=E3=81=8C=E5=A4=A7=E4=B8=88=E5=A4=AB?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

YYYYYY
---
  testfile.txt |    4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/testfile.txt b/testfile.txt
index 1e5d832..da982fd 100644
--- a/testfile.txt
+++ b/testfile.txt
@@ -1 +1,3 @@
-sadfasdf
\ No newline at end of file
+sadfasdf

..........

-------------------------------------------------------------------------------

this patch will be applied successfully. So I know the problem is about the subject creating.
I search the source, then I found the following function at "pretty.c:655":

const char *format_subject(struct strbuf *sb, const char *msg,
			   const char *line_separator)
{
	int first = 1;

	for (;;) {
		const char *line = msg;
		int linelen = get_one_line(line);

		msg += linelen;
		
		if (!linelen || is_empty_line(line, &linelen))
			break;

		if (!sb)cat
			continue;
		strbuf_grow(sb, linelen + 2);
		if (!first)
			strbuf_addstr(sb, line_separator);
		strbuf_add(sb, line, linelen);
		first = 0;
	}
	return msg;
}

At first I want to know: Does this function means that always add the first line
of comment to the argument sb, then return the rest? Is there any other thing that I
didn't considered?

I found 4 place where to call this function, I think there is no problem about 3
of them, but I don't know is there any other problem to the rest one which is
at "pretty.c:931".

At last, if what I think is correct, I plan to fix it as following:

const char *format_subject(struct strbuf *sb, const char *msg,
			   const char *line_separator)
{
	int first = 1;

	//for (;;) {
		const char *line = msg;
		int linelen = get_one_line(line);

		msg += linelen;
		
		if (!linelen || is_empty_line(line, &linelen)) return msg;
			//break;

		if (!sb) return msg;
			//continue;
		strbuf_grow(sb, linelen + 2);
		if (!first)
			strbuf_addstr(sb, line_separator);
		strbuf_add(sb, line, linelen);
		first = 0;
	//}
	return msg;
}

I dont't think it is necessary to have a loop here, so I want to remove
the loop. Is there anybody can confirm my fix for me?

Re: a bug about format-patch of multibyte characters comment

["Martin Krüger"]

Hi

I recently stumbled over the same problem:

http://thread.gmane.org/gmane.comp.version-control.git/162792/focus=162797


best regards.

martin

Re: a bug about format-patch of multibyte characters comment

[Jeff King]

On Sat, Feb 12, 2011 at 07:13:15PM +0900, xiaozhu wrote:

> I commit a fix to my repository with comment like following:
> -----------------------------------------------------
> XXXXXXXXXXXX
> YYYYYY
> -----------------------------------------------------
> 
> two lines of multibyte language comment.

OK.

> then I use format-patch to export this fix, I get a patch file like following:
> 
> ------------------------------------------------------------------------------
> From d3532c3263a02a2367a3aa5c9cc3f0bd738b79b1 Mon Sep 17 00:00:00 2001
> From: xz <xz>
> Date: Fri, 11 Feb 2011 21:30:35 +0900
> Subject: [PATCH] =?UTF-8?q?=E6=97=A5=E6=9C=AC=E8=AA=9E=E3=81=8C=E5=A4=A7=E4=B8=88=E5=A4=AB
> =20=E6=94=B9=E8=A1=8C=E3=81=99=E3=82=8B?=

Yeah, this is wrong. There should be a whitespace indentation in a
multi-line header, or the whole thing should be on one line. The newline
in your commit subject is apparently leaking through, and it should be
qp-encoded.

> const char *format_subject(struct strbuf *sb, const char *msg,
> 			   const char *line_separator)
> {
> 	int first = 1;
> 
> 	for (;;) {
> 		const char *line = msg;
> 		int linelen = get_one_line(line);
> 
> 		msg += linelen;
> 		
> 		if (!linelen || is_empty_line(line, &linelen))
> 			break;
> 
> 		if (!sb)cat
> 			continue;
> 		strbuf_grow(sb, linelen + 2);
> 		if (!first)
> 			strbuf_addstr(sb, line_separator);
> 		strbuf_add(sb, line, linelen);
> 		first = 0;
> 	}
> 	return msg;
> }
> 
> At first I want to know: Does this function means that always add the first line
> of comment to the argument sb, then return the rest? Is there any other thing that I
> didn't considered?

No. It adds all lines in the first paragraph into sb. Which, if you
follow the usual git convention, is a single line. But we also see
commits imported from other version control systems where that
convention is not as prevalent. In practice, using the whole first
paragraph seems to make the most sense as a subject.

> At last, if what I think is correct, I plan to fix it as following:
> 
> const char *format_subject(struct strbuf *sb, const char *msg,
> 			   const char *line_separator)
> {
> 	int first = 1;
> 
> 	//for (;;) {
> 		const char *line = msg;
> 		int linelen = get_one_line(line);
> 
> 		msg += linelen;
> 		
> 		if (!linelen || is_empty_line(line, &linelen)) return msg;
> 			//break;
> 
> 		if (!sb) return msg;
> 			//continue;
> 		strbuf_grow(sb, linelen + 2);
> 		if (!first)
> 			strbuf_addstr(sb, line_separator);
> 		strbuf_add(sb, line, linelen);
> 		first = 0;
> 	//}
> 	return msg;
> }

No, that's not right. It breaks the use-whole-paragraph feature. The
right fix is to encode the embedded newline in the subject properly.

-Peff

Re: a bug about format-patch of multibyte characters comment

[Jeff King]

On Sun, Feb 13, 2011 at 02:53:37AM -0500, Jeff King wrote:

> No, that's not right. It breaks the use-whole-paragraph feature. The
> right fix is to encode the embedded newline in the subject properly.

Hrm. It is actually a little more complex. Right now for a subject like:

  one
  two
  three

we will convert that to the subject "one two three" for "git log
--oneline" output. But for email messages, we actually return "one\n
two\n three", which is conflating header folding with the actual
construction of the title.

Shouldn't we still be generating "one two three", encoding it via
rfc2047 if necessary, and _then_ deciding if folding is required? Yes,
individual lines in a multi-line subject are good candidates for
folding, but don't we need to be checking for and folding long lines
anyway?

-Peff

Re: a bug about format-patch of multibyte characters comment

[xiaozhu]

> Shouldn't we still be generating "one two three", encoding it via
> rfc2047 if necessary, and _then_ deciding if folding is required? Yes,
> individual lines in a multi-line subject are good candidates for
> folding, but don't we need to be checking for and folding long lines
> anyway?

It seems that by rfc2047 there is no multi-line subject spec. A subject
with multi-line will be always conflated to one single line. And also
that if we just generate the subject within multi-line just like the
current implemention, yes, we can modify the git-am to decode it correctly,
but most of the mail client will can not show it correctly.

So it seems that there is only one way that combining the whole first
paragraph to a single line? But it will be a nightmare for some long comment.

Re: a bug about format-patch of multibyte characters comment

[Jeff King]

On Sun, Feb 13, 2011 at 05:45:41PM +0900, xiaozhu wrote:

> >Shouldn't we still be generating "one two three", encoding it via
> >rfc2047 if necessary, and _then_ deciding if folding is required? Yes,
> >individual lines in a multi-line subject are good candidates for
> >folding, but don't we need to be checking for and folding long lines
> >anyway?
> 
> It seems that by rfc2047 there is no multi-line subject spec. A subject
> with multi-line will be always conflated to one single line.

Sorry, I don't quite parse what you're saying. If the header takes up
multiple lines, then yes, that gets decoded as a single line by rfc822
header folding. I would then expect that result to be rfc2047-decoded if
necessary, and in theory it could contain encoded newlines.

> And also that if we just generate the subject within multi-line just
> like the current implemention, yes, we can modify the git-am to decode
> it correctly, but most of the mail client will can not show it
> correctly.

Again, I don't quite understand what you're saying. The output generated
by format-patch now is _not_ valid according to rfc2822. Changing git-am
to parse its bogus output won't help that.

> So it seems that there is only one way that combining the whole first
> paragraph to a single line? But it will be a nightmare for some long comment.

It's not the only way, but it is how we treat multi-line subjects in all
other parts of git, so it is at least consistent (and that behavior was
agreed upon after seeing what is worse: truncating to a single line, or
merging lines).

-Peff

Re: a bug about format-patch of multibyte characters comment

[Johannes Sixt]

On Sonntag, 13. Februar 2011, Jeff King wrote:
> On Sat, Feb 12, 2011 at 07:13:15PM +0900, xiaozhu wrote:
> > Subject: [PATCH]
> > =?UTF-8?q?=E6=97=A5=E6=9C=AC=E8=AA=9E=E3=81=8C=E5=A4=A7=E4=B8=88=E5=A4=AB
> > =20=E6=94=B9=E8=A1=8C=E3=81=99=E3=82=8B?=
>
> Yeah, this is wrong. There should be a whitespace indentation in a
> multi-line header, or the whole thing should be on one line. The newline
> in your commit subject is apparently leaking through, and it should be
> qp-encoded.

Isn't it wrong that format-patch (and --pretty=email) does any quoting in the 
first place? Isn't it the task of the MUA (git-send-email) to do the quoting?

For example, when I import a format-patch generated patch into a mail message, 
I don't want to see:

 From: =?UTF-8?q?Joh=C3=A4nnes=20S=C3=BCxt?= <me@localhost>

but rather:

 From: Johännes Süxt <me@localhost>

I know this is a bit late, but this really comes as a surprise. (I've never 
had to pay attention to this behavior in the past...)

-- Hannes

Re: a bug about format-patch of multibyte characters comment

[Jeff King]

On Sun, Feb 13, 2011 at 10:48:58AM +0100, Johannes Sixt wrote:

> On Sonntag, 13. Februar 2011, Jeff King wrote:
> > On Sat, Feb 12, 2011 at 07:13:15PM +0900, xiaozhu wrote:
> > > Subject: [PATCH]
> > > =?UTF-8?q?=E6=97=A5=E6=9C=AC=E8=AA=9E=E3=81=8C=E5=A4=A7=E4=B8=88=E5=A4=AB
> > > =20=E6=94=B9=E8=A1=8C=E3=81=99=E3=82=8B?=
> >
> > Yeah, this is wrong. There should be a whitespace indentation in a
> > multi-line header, or the whole thing should be on one line. The newline
> > in your commit subject is apparently leaking through, and it should be
> > qp-encoded.
> 
> Isn't it wrong that format-patch (and --pretty=email) does any quoting in the 
> first place? Isn't it the task of the MUA (git-send-email) to do the quoting?

I dunno. I guess it depends on how you view the output of format-patch.
Is it an mbox containing rfc2822-valid messages? Then it ought to be
quoted.

Certainly when opening the output in an editor, it is prettier to have
it unquoted. But what do MUAs expect when opening such an mbox? mutt
seems to handle unencoded utf8 just fine, but I don't know about other
MUAs.

> For example, when I import a format-patch generated patch into a mail message, 
> I don't want to see:
> 
>  From: =?UTF-8?q?Joh=C3=A4nnes=20S=C3=BCxt?= <me@localhost>
> 
> but rather:
> 
>  From: Johännes Süxt <me@localhost>
> 
> I know this is a bit late, but this really comes as a surprise. (I've never 
> had to pay attention to this behavior in the past...)

I can see how that would be annoying if your workflow is to paste
format-patch output into an email. But it has been that way literally
for years (I just tried v1.5.0, and format-patch produces
rfc2047-encoded headers). So yes, you are a bit late. :)

-Peff

Re: a bug about format-patch of multibyte characters comment

[xiaozhu]

On 2011/02/13 17:52, Jeff King wrote:
> On Sun, Feb 13, 2011 at 05:45:41PM +0900, xiaozhu wrote:
>
>>> Shouldn't we still be generating "one two three", encoding it via
>>> rfc2047 if necessary, and _then_ deciding if folding is required? Yes,
>>> individual lines in a multi-line subject are good candidates for
>>> folding, but don't we need to be checking for and folding long lines
>>> anyway?
>>
>> It seems that by rfc2047 there is no multi-line subject spec. A subject
>> with multi-line will be always conflated to one single line.
>
> Sorry, I don't quite parse what you're saying. If the header takes up
> multiple lines, then yes, that gets decoded as a single line by rfc822
> header folding. I would then expect that result to be rfc2047-decoded if
> necessary, and in theory it could contain encoded newlines.

I am not similar with mail format. I read the rfc2047 again, but I didn't
see any description about line separator encoding. Perhaps a base64 encoded-word
will contain the line separator involuntarily? I also found a sample in
rfc2047 it show us a line broken subject mail, but it didn't say any thing
about line separator encoding.

>> And also that if we just generate the subject within multi-line just
>> like the current implemention, yes, we can modify the git-am to decode
>> it correctly, but most of the mail client will can not show it
>> correctly.
>
> Again, I don't quite understand what you're saying. The output generated
> by format-patch now is _not_ valid according to rfc2822. Changing git-am
> to parse its bogus output won't help that.
>
>> So it seems that there is only one way that combining the whole first
>> paragraph to a single line? But it will be a nightmare for some long comment.
>
> It's not the only way, but it is how we treat multi-line subjects in all
> other parts of git, so it is at least consistent (and that behavior was
> agreed upon after seeing what is worse: truncating to a single line, or
> merging lines).
>
> -Peff

A sample of rfc2047 show us a legal line broken subject mail, like following:
------------------------------------------------------------------
  Subject: =?ISO-8859-1?B?SWYgeW91IGNhbiByZWFkIHRoaXMgeW8=?=
     =?ISO-8859-2?B?dSB1bmRlcnN0YW5kIHRoZSBleGFtcGxlLg==?=
------------------------------------------------------------------

I understand that the current format-patch is not not valid to rfc2822/rfc2047,
but even a valid one just like above, most of the mail client will can not show it
correctly, they show the first line only, I think that's a problem of user
friendliness.

-xzer

Re: a bug about format-patch of multibyte characters comment

[xzer]

> A sample of rfc2047 show us a legal line broken subject mail, like
> following:
> ------------------------------------------------------------------
>  Subject: =?ISO-8859-1?B?SWYgeW91IGNhbiByZWFkIHRoaXMgeW8=?=
>    =?ISO-8859-2?B?dSB1bmRlcnN0YW5kIHRoZSBleGFtcGxlLg==?=
> ------------------------------------------------------------------
>
> I understand that the current format-patch is not not valid to
> rfc2822/rfc2047,
> but even a valid one just like above, most of the mail client will can not
> show it
> correctly, they show the first line only, I think that's a problem of user
> friendliness.

I am sorry I made a mistake, there is no problem of mail client, I just
create a wrong format to test. So now I think if we can generate a valid
rfc2047 patch file, and then make the am also analyze the patch file
correctly, there is no problem. Isn't it?

-xzer

Re: a bug about format-patch of multibyte characters comment

[Jeff King]

On Sun, Feb 13, 2011 at 07:14:20PM +0900, xiaozhu wrote:

> I am not similar with mail format. I read the rfc2047 again, but I didn't
> see any description about line separator encoding. Perhaps a base64 encoded-word
> will contain the line separator involuntarily? I also found a sample in
> rfc2047 it show us a line broken subject mail, but it didn't say any thing
> about line separator encoding.

If the encoded-word contains the line separator, wouldn't it be a
literal character in the header value then? I.e., using rfc2047 you can
embed a literal newline into your subject (or possibly, a 0x0a byte may
be part of a multi-byte character; that can't happen in utf8, but I
believe it can in utf16).

> A sample of rfc2047 show us a legal line broken subject mail, like following:
> ------------------------------------------------------------------
>  Subject: =?ISO-8859-1?B?SWYgeW91IGNhbiByZWFkIHRoaXMgeW8=?=
>     =?ISO-8859-2?B?dSB1bmRlcnN0YW5kIHRoZSBleGFtcGxlLg==?=
> ------------------------------------------------------------------
> 
> I understand that the current format-patch is not not valid to rfc2822/rfc2047,
> but even a valid one just like above, most of the mail client will can not show it
> correctly, they show the first line only, I think that's a problem of user
> friendliness.

Then those mail clients are broken. That should first be unfolded to put
both encoded-words on the same line (separated by whitespace, I think,
though it doesn't matter for encoded words), and then each encoded word
should be decoded (the resulting text is "If you can read this you
understand the example.").

Mutt and other MUAs do this just fine. If you have a MUA that doesn't,
complain to the author.

-Peff

Re: a bug about format-patch of multibyte characters comment

[Jeff King]

On Sun, Feb 13, 2011 at 07:22:14PM +0900, xzer wrote:

> > A sample of rfc2047 show us a legal line broken subject mail, like
> > following:
> > ------------------------------------------------------------------
> >  Subject: =?ISO-8859-1?B?SWYgeW91IGNhbiByZWFkIHRoaXMgeW8=?=
> >    =?ISO-8859-2?B?dSB1bmRlcnN0YW5kIHRoZSBleGFtcGxlLg==?=
> > ------------------------------------------------------------------
> >
> > I understand that the current format-patch is not not valid to
> > rfc2822/rfc2047,
> > but even a valid one just like above, most of the mail client will can not
> > show it
> > correctly, they show the first line only, I think that's a problem of user
> > friendliness.
> 
> I am sorry I made a mistake, there is no problem of mail client, I just
> create a wrong format to test. So now I think if we can generate a valid
> rfc2047 patch file, and then make the am also analyze the patch file
> correctly, there is no problem. Isn't it?

Ah, OK, our mails just crossed paths. git-am already parses this
correctly (actually, it is git-mailinfo that parses on behalf of
git-am). We just need format-patch to generate it (and it should also
probably be folding long lines in general).

-Peff

Re: a bug about format-patch of multibyte characters comment

[xiaozhu]

> Ah, OK, our mails just crossed paths. git-am already parses this
> correctly (actually, it is git-mailinfo that parses on behalf of
> git-am). We just need format-patch to generate it (and it should also
> probably be folding long lines in general).

I tested it, yes, the git-am parses it "correctly". There is only
one problem that we lost the line break after import a patch. Is
there a way that we can retain the line break after import the
patch?

-xzer