a bug about format-patch of multibyte characters comment

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

index 1e5d832..da982fd 100644
@@ -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(-)

index 1e5d832..da982fd 100644
@@ -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?

Hi

I recently stumbled over the same problem:

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

best regards.

martin

OK.

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.

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.

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

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

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.

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.

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.

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

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.

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

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

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

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

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

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

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

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.

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