git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/5] Cure some format-patch wrapping and encoding issues
@ 2012-10-08 17:33 Jan H. Schönherr
  2012-10-08 17:33 ` [PATCH 1/5] format-patch: do not wrap non-rfc2047 headers too early Jan H. Schönherr
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Jan H. Schönherr @ 2012-10-08 17:33 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Jan H. Schönherr

Hi all.

The main point of this series is to teach git to encode my name
correctly, see patch 4, so that the decoded version is actually
my name, so that send-email does not insist on adding a wrong
superfluous From: line to the mail body.

But as always, you notice some other things going wrong. Here,
patches 1 and 2 make the wrapping of header lines more correct,
i. e., neither too early nor too late.

Patch 3 does some refactoring, which is too unrelated to be included
in patch 4 itself.

Patch 5 points out further problems, but leaves the actual fixing
to someone else.

The series is currently based on the maint branch, but it applies
to the others as well.



During the creation of this series, I came across the strbuf 
wrapping functions, and I wonder if there is an off-by-one issue.

Consider the following excerpt from t4202:

cat > expect << EOF
 This is
  the sixth
  commit.
 This is
  the fifth
  commit.
EOF

test_expect_success 'format %w(12,1,2)' '

	git log -2 --format="%w(12,1,2)This is the %s commit." > actual &&
	test_cmp expect actual
'

So this sets a maximum width of 12 characters. Is that 12 character limit
supposed to include the final newline, or not? Because the test above and
my series are only correct if the final newline is included, i. e., at most
eleven visible characters.

If this should mean at most 12 visible characters instead, then the output
should look like this:

 This is the
  sixth
  commit.
 This is the
  fifth
  commit.

(In that case, I would repost an updated version of this series.)

Regards
Jan


Jan H. Schönherr (5):
  format-patch: do not wrap non-rfc2047 headers too early
  format-patch: do not wrap rfc2047 encoded headers too late
  format-patch: introduce helper function last_line_length()
  format-patch: fix rfc2047 address encoding with respect to rfc822
    specials
  format-patch: tests: check rfc822+rfc2047 in to+cc headers

 pretty.c                | 121 ++++++++++++++++++--------
 t/t4014-format-patch.sh | 227 ++++++++++++++++++++++++++++++------------------
 2 Dateien geändert, 229 Zeilen hinzugefügt(+), 119 Zeilen entfernt(-)

-- 
1.7.12

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/5] format-patch: do not wrap non-rfc2047 headers too early
  2012-10-08 17:33 [PATCH 0/5] Cure some format-patch wrapping and encoding issues Jan H. Schönherr
@ 2012-10-08 17:33 ` Jan H. Schönherr
  2012-10-08 17:33 ` [PATCH 2/5] format-patch: do not wrap rfc2047 encoded headers too late Jan H. Schönherr
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Jan H. Schönherr @ 2012-10-08 17:33 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Jan H. Schönherr

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

Do not wrap the second and later lines of an ASCII header substantially
before the 78 character limit.

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
 pretty.c                |  2 +-
 t/t4014-format-patch.sh | 60 ++++++++++++++++++++++++++++---------------------
 2 Dateien geändert, 35 Zeilen hinzugefügt(+), 27 Zeilen entfernt(-)

diff --git a/pretty.c b/pretty.c
index 8b1ea9f..f5caecb 100644
--- a/pretty.c
+++ b/pretty.c
@@ -286,7 +286,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		if ((i + 1 < len) && (ch == '=' && line[i+1] == '?'))
 			goto needquote;
 	}
-	strbuf_add_wrapped_bytes(sb, line, len, 0, 1, max_length - line_len);
+	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, max_length+1);
 	return;
 
 needquote:
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 959aa26..d66e358 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -752,16 +752,14 @@ M64=$M8$M8$M8$M8$M8$M8$M8$M8
 M512=$M64$M64$M64$M64$M64$M64$M64$M64
 cat >expect <<'EOF'
 Subject: [PATCH] foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
- bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
- foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
- bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
- foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
- bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
- foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
- bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
- foo bar foo bar foo bar foo bar
+ bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
+ foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
+ bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
+ foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
+ bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
+ foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
 EOF
-test_expect_success 'format-patch wraps extremely long headers (ascii)' '
+test_expect_success 'format-patch wraps extremely long subject (ascii)' '
 	echo content >>file &&
 	git add file &&
 	git commit -m "$M512" &&
@@ -807,28 +805,12 @@ test_expect_success 'format-patch wraps extremely long headers (rfc2047)' '
 	test_cmp expect subject
 '
 
-M8="foo_bar_"
-M64=$M8$M8$M8$M8$M8$M8$M8$M8
-cat >expect <<EOF
-From: $M64
- <foobar@foo.bar>
-EOF
-test_expect_success 'format-patch wraps non-quotable headers' '
-	rm -rf patches/ &&
-	echo content >>file &&
-	git add file &&
-	git commit -mfoo --author "$M64 <foobar@foo.bar>" &&
-	git format-patch --stdout -1 >patch &&
-	sed -n "/^From: /p; /^ /p; /^$/q" <patch >from &&
-	test_cmp expect from
-'
-
 check_author() {
 	echo content >>file &&
 	git add file &&
 	GIT_AUTHOR_NAME=$1 git commit -m author-check &&
 	git format-patch --stdout -1 >patch &&
-	grep ^From: patch >actual &&
+	sed -n "/^From: /p; /^ /p; /^$/q" <patch >actual &&
 	test_cmp expect actual
 }
 
@@ -853,6 +835,32 @@ test_expect_success 'rfc2047-encoded headers also double-quote 822 specials' '
 	check_author "Föo B. Bar"
 '
 
+cat >expect <<EOF
+From: foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_
+ <author@example.com>
+EOF
+test_expect_success 'format-patch wraps moderately long from-header (ascii)' '
+	check_author "foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_"
+'
+
+cat >expect <<'EOF'
+From: Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar
+ Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo
+ Bar Foo Bar Foo Bar Foo Bar <author@example.com>
+EOF
+test_expect_success 'format-patch wraps extremely long from-header (ascii)' '
+	check_author "Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar"
+'
+
+cat >expect <<'EOF'
+From: "Foo.Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar
+ Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo
+ Bar Foo Bar Foo Bar Foo Bar" <author@example.com>
+EOF
+test_expect_success 'format-patch wraps extremely long from-header (rfc822)' '
+	check_author "Foo.Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar"
+'
+
 cat >expect <<'EOF'
 Subject: header with . in it
 EOF
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/5] format-patch: do not wrap rfc2047 encoded headers too late
  2012-10-08 17:33 [PATCH 0/5] Cure some format-patch wrapping and encoding issues Jan H. Schönherr
  2012-10-08 17:33 ` [PATCH 1/5] format-patch: do not wrap non-rfc2047 headers too early Jan H. Schönherr
@ 2012-10-08 17:33 ` Jan H. Schönherr
       [not found]   ` <7v7gqzfnpj.fsf@alter.siamese.dyndns.org>
  2012-10-08 17:33 ` [PATCH 3/5] format-patch: introduce helper function last_line_length() Jan H. Schönherr
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 10+ messages in thread
From: Jan H. Schönherr @ 2012-10-08 17:33 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Jan H. Schönherr

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

Encoded characters add more than one character at once to an encoded
header. Include all characters that are about to be added in the length
calculation for wrapping.

Additionally, RFC 2047 imposes a maximum line length of 76 characters
if that line contains an rfc2047 encoded word.

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
 pretty.c                | 24 +++++++++++---------
 t/t4014-format-patch.sh | 58 +++++++++++++++++++++++++++++--------------------
 2 Dateien geändert, 49 Zeilen hinzugefügt(+), 33 Zeilen entfernt(-)

diff --git a/pretty.c b/pretty.c
index f5caecb..daf8581 100644
--- a/pretty.c
+++ b/pretty.c
@@ -263,13 +263,22 @@ static void add_rfc822_quoted(struct strbuf *out, const char *s, int len)
 
 static int is_rfc2047_special(char ch)
 {
+	/*
+	 * We encode ' ' using '=20' even though rfc2047
+	 * allows using '_' for readability.  Unfortunately,
+	 * many programs do not understand this and just
+	 * leave the underscore in place.
+	 */
+	if (ch == ' ' || ch == '\n')
+		return 1;
+
 	return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
 }
 
 static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		       const char *encoding)
 {
-	static const int max_length = 78; /* per rfc2822 */
+	static const int max_length = 76; /* per rfc2047 */
 	int i;
 	int line_len;
 
@@ -286,7 +295,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		if ((i + 1 < len) && (ch == '=' && line[i+1] == '?'))
 			goto needquote;
 	}
-	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, max_length+1);
+	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, 78+1);
 	return;
 
 needquote:
@@ -295,19 +304,14 @@ needquote:
 	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);
 
-		if (line_len >= max_length - 2) {
+		if (line_len + 2 + (is_special ? 3 : 1) > max_length) {
 			strbuf_addf(sb, "?=\n =?%s?q?", encoding);
 			line_len = strlen(encoding) + 5 + 1; /* =??q? plus SP */
 		}
 
-		/*
-		 * We encode ' ' using '=20' even though rfc2047
-		 * allows using '_' for readability.  Unfortunately,
-		 * many programs do not understand this and just
-		 * leave the underscore in place.
-		 */
-		if (is_rfc2047_special(ch) || ch == ' ' || ch == '\n') {
+		if (is_special) {
 			strbuf_addf(sb, "=%02X", ch);
 			line_len += 3;
 		}
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index d66e358..1d5636d 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -772,30 +772,31 @@ M8="föö bar "
 M64=$M8$M8$M8$M8$M8$M8$M8$M8
 M512=$M64$M64$M64$M64$M64$M64$M64$M64
 cat >expect <<'EOF'
-Subject: [PATCH] =?UTF-8?q?f=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=B6?=
- =?UTF-8?q?=C3=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=B6?=
- =?UTF-8?q?=C3=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=B6?=
- =?UTF-8?q?=C3=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=B6?=
- =?UTF-8?q?=C3=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=B6?=
- =?UTF-8?q?=C3=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=B6?=
- =?UTF-8?q?=C3=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=B6?=
- =?UTF-8?q?=C3=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=B6?=
- =?UTF-8?q?=C3=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=B6?=
- =?UTF-8?q?=C3=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=B6?=
- =?UTF-8?q?=C3=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?=
+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?=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=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?=
 EOF
-test_expect_success 'format-patch wraps extremely long headers (rfc2047)' '
+test_expect_success 'format-patch wraps extremely long subject (rfc2047)' '
 	rm -rf patches/ &&
 	echo content >>file &&
 	git add file &&
@@ -862,6 +863,17 @@ test_expect_success 'format-patch wraps extremely long from-header (rfc822)' '
 '
 
 cat >expect <<'EOF'
+From: =?UTF-8?q?Fo=C3=B6=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo?=
+ =?UTF-8?q?=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20?=
+ =?UTF-8?q?Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar?=
+ =?UTF-8?q?=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20?=
+ =?UTF-8?q?Foo=20Bar=20Foo=20Bar?= <author@example.com>
+EOF
+test_expect_success 'format-patch wraps extremely long from-header (rfc2047)' '
+	check_author "Foö Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar"
+'
+
+cat >expect <<'EOF'
 Subject: header with . in it
 EOF
 test_expect_success 'subject lines do not have 822 atom-quoting' '
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/5] format-patch: introduce helper function last_line_length()
  2012-10-08 17:33 [PATCH 0/5] Cure some format-patch wrapping and encoding issues Jan H. Schönherr
  2012-10-08 17:33 ` [PATCH 1/5] format-patch: do not wrap non-rfc2047 headers too early Jan H. Schönherr
  2012-10-08 17:33 ` [PATCH 2/5] format-patch: do not wrap rfc2047 encoded headers too late Jan H. Schönherr
@ 2012-10-08 17:33 ` Jan H. Schönherr
  2012-10-08 17:33 ` [PATCH 4/5] format-patch: fix rfc2047 address encoding with respect to rfc822 specials Jan H. Schönherr
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Jan H. Schönherr @ 2012-10-08 17:33 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Jan H. Schönherr

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

Currently, an open-coded loop to calculate the length of the last
line of a string buffer is used in multiple places.

Move that code into a function of its own.

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
 pretty.c | 25 +++++++++++++------------
 1 Datei geändert, 13 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-)

diff --git a/pretty.c b/pretty.c
index daf8581..ee76219 100644
--- a/pretty.c
+++ b/pretty.c
@@ -240,6 +240,17 @@ static int has_rfc822_specials(const char *s, int len)
 	return 0;
 }
 
+static int last_line_length(struct strbuf *sb)
+{
+	int i;
+
+	/* How many bytes are already used on the last line? */
+	for (i = sb->len - 1; i >= 0; i--)
+		if (sb->buf[i] == '\n')
+			break;
+	return sb->len - (i + 1);
+}
+
 static void add_rfc822_quoted(struct strbuf *out, const char *s, int len)
 {
 	int i;
@@ -280,13 +291,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 {
 	static const int max_length = 76; /* per rfc2047 */
 	int i;
-	int line_len;
-
-	/* How many bytes are already used on the current line? */
-	for (i = sb->len - 1; i >= 0; i--)
-		if (sb->buf[i] == '\n')
-			break;
-	line_len = sb->len - (i+1);
+	int line_len = last_line_length(sb);
 
 	for (i = 0; i < len; i++) {
 		int ch = line[i];
@@ -344,7 +349,6 @@ void pp_user_info(const struct pretty_print_context *pp,
 	if (pp->fmt == CMIT_FMT_EMAIL) {
 		char *name_tail = strchr(line, '<');
 		int display_name_length;
-		int final_line;
 		if (!name_tail)
 			return;
 		while (line < name_tail && isspace(name_tail[-1]))
@@ -359,10 +363,7 @@ void pp_user_info(const struct pretty_print_context *pp,
 			add_rfc2047(sb, quoted.buf, quoted.len, encoding);
 			strbuf_release(&quoted);
 		}
-		for (final_line = 0; final_line < sb->len; final_line++)
-			if (sb->buf[sb->len - final_line - 1] == '\n')
-				break;
-		if (namelen - display_name_length + final_line > 78) {
+		if (namelen - display_name_length + last_line_length(sb) > 78) {
 			strbuf_addch(sb, '\n');
 			if (!isspace(name_tail[0]))
 				strbuf_addch(sb, ' ');
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/5] format-patch: fix rfc2047 address encoding with respect to rfc822 specials
  2012-10-08 17:33 [PATCH 0/5] Cure some format-patch wrapping and encoding issues Jan H. Schönherr
                   ` (2 preceding siblings ...)
  2012-10-08 17:33 ` [PATCH 3/5] format-patch: introduce helper function last_line_length() Jan H. Schönherr
@ 2012-10-08 17:33 ` Jan H. Schönherr
  2012-10-08 17:33 ` [PATCH 5/5] format-patch: tests: check rfc822+rfc2047 in to+cc headers Jan H. Schönherr
       [not found] ` <7vfw5nfoq9.fsf@alter.siamese.dyndns.org>
  5 siblings, 0 replies; 10+ messages in thread
From: Jan H. Schönherr @ 2012-10-08 17:33 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Jan H. Schönherr

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

According to RFC 2047 and RFC 822, rfc2047 encoded words and and rfc822
quoted strings do not mix.

Be more strict about rfc2047 encoded words in addresses, so that it is a
bit more conform to RFC 2047.

(Especially, my own name gets correctly decoded as Jan H. Schönherr
(without quotes) and not as "Jan H. Schönherr" (with quotes).)

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
 pretty.c                | 80 ++++++++++++++++++++++++++++++++++++++-----------
 t/t4014-format-patch.sh | 11 +++++--
 2 Dateien geändert, 71 Zeilen hinzugefügt(+), 20 Zeilen entfernt(-)

diff --git a/pretty.c b/pretty.c
index ee76219..f3a7383 100644
--- a/pretty.c
+++ b/pretty.c
@@ -231,7 +231,7 @@ static int is_rfc822_special(char ch)
 	}
 }
 
-static int has_rfc822_specials(const char *s, int len)
+static int needs_rfc822_quoting(const char *s, int len)
 {
 	int i;
 	for (i = 0; i < len; i++)
@@ -272,7 +272,12 @@ static void add_rfc822_quoted(struct strbuf *out, const char *s, int len)
 	strbuf_addch(out, '"');
 }
 
-static int is_rfc2047_special(char ch)
+enum rfc2047_type {
+	RFC2047_SUBJECT,
+	RFC2047_ADDRESS,
+};
+
+static int is_rfc2047_special(char ch, enum rfc2047_type type)
 {
 	/*
 	 * We encode ' ' using '=20' even though rfc2047
@@ -283,33 +288,62 @@ static int is_rfc2047_special(char ch)
 	if (ch == ' ' || ch == '\n')
 		return 1;
 
-	return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
+	if (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'))
+		return 1;
+
+	if (type != RFC2047_ADDRESS)
+		return 0;
+
+	/*
+	 * rfc2047, section 5.3:
+	 *
+	 *    As a replacement for a 'word' entity within a 'phrase', for example,
+	 *    one that precedes an address in a From, To, or Cc header.  The ABNF
+	 *    definition for 'phrase' from RFC 822 thus becomes:
+	 *
+	 *    phrase = 1*( encoded-word / word )
+	 *
+	 *    In this case the set of characters that may be used in a "Q"-encoded
+	 *    'encoded-word' is restricted to: <upper and lower case ASCII
+	 *    letters, decimal digits, "!", "*", "+", "-", "/", "=", and "_"
+	 *    (underscore, ASCII 95.)>.  An 'encoded-word' that appears within a
+	 *    'phrase' MUST be separated from any adjacent 'word', 'text' or
+	 *    'special' by 'linear-white-space'.
+	 */
+
+	/* '=' and '_' are special cases and have been checked above */
+	return !(isalnum(ch) || ch == '!' || ch == '*' || ch == '+' || ch == '-' || ch == '/');
 }
 
-static void add_rfc2047(struct strbuf *sb, const char *line, int len,
-		       const char *encoding)
+static int needs_rfc2047_encoding(const char *line, int len,
+				  enum rfc2047_type type)
 {
-	static const int max_length = 76; /* per rfc2047 */
 	int i;
-	int line_len = last_line_length(sb);
 
 	for (i = 0; i < len; i++) {
 		int ch = line[i];
 		if (non_ascii(ch) || ch == '\n')
-			goto needquote;
+			return 1;
 		if ((i + 1 < len) && (ch == '=' && line[i+1] == '?'))
-			goto needquote;
+			return 1;
 	}
-	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, 78+1);
-	return;
 
-needquote:
+	return 0;
+}
+
+static void add_rfc2047(struct strbuf *sb, const char *line, int len,
+		       const char *encoding, enum rfc2047_type type)
+{
+	static const int max_length = 76; /* per rfc2047 */
+	int i;
+	int line_len = last_line_length(sb);
+
 	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);
+		int is_special = is_rfc2047_special(ch, type);
 
 		if (line_len + 2 + (is_special ? 3 : 1) > max_length) {
 			strbuf_addf(sb, "?=\n =?%s?q?", encoding);
@@ -355,13 +389,18 @@ void pp_user_info(const struct pretty_print_context *pp,
 			name_tail--;
 		display_name_length = name_tail - line;
 		strbuf_addstr(sb, "From: ");
-		if (!has_rfc822_specials(line, display_name_length)) {
-			add_rfc2047(sb, line, display_name_length, encoding);
-		} else {
+		if (needs_rfc2047_encoding(line, display_name_length, RFC2047_ADDRESS)) {
+			add_rfc2047(sb, line, display_name_length,
+						encoding, RFC2047_ADDRESS);
+		} else if (needs_rfc822_quoting(line, display_name_length)) {
 			struct strbuf quoted = STRBUF_INIT;
 			add_rfc822_quoted(&quoted, line, display_name_length);
-			add_rfc2047(sb, quoted.buf, quoted.len, encoding);
+			strbuf_add_wrapped_bytes(sb, quoted.buf, quoted.len,
+								-6, 1, 78+1);
 			strbuf_release(&quoted);
+		} else {
+			strbuf_add_wrapped_bytes(sb, line, display_name_length,
+								-6, 1, 78+1);
 		}
 		if (namelen - display_name_length + last_line_length(sb) > 78) {
 			strbuf_addch(sb, '\n');
@@ -1292,7 +1331,12 @@ void pp_title_line(const struct pretty_print_context *pp,
 	strbuf_grow(sb, title.len + 1024);
 	if (pp->subject) {
 		strbuf_addstr(sb, pp->subject);
-		add_rfc2047(sb, title.buf, title.len, encoding);
+		if (needs_rfc2047_encoding(title.buf, title.len, RFC2047_SUBJECT))
+			add_rfc2047(sb, title.buf, title.len, encoding,
+				    RFC2047_SUBJECT);
+		else
+			strbuf_add_wrapped_bytes(sb, title.buf, title.len,
+						 -last_line_length(sb), 1, 78+1);
 	} else {
 		strbuf_addbuf(sb, &title);
 	}
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 1d5636d..1a3b6e8 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -830,9 +830,16 @@ test_expect_success 'format-patch quotes double-quote in headers' '
 '
 
 cat >expect <<'EOF'
-From: =?UTF-8?q?"F=C3=B6o=20B.=20Bar"?= <author@example.com>
+From: =?UTF-8?q?F=C3=B6o=20Bar?= <author@example.com>
 EOF
-test_expect_success 'rfc2047-encoded headers also double-quote 822 specials' '
+test_expect_success 'format-patch uses rfc2047-encoded headers when necessary' '
+	check_author "Föo Bar"
+'
+
+cat >expect <<'EOF'
+From: =?UTF-8?q?F=C3=B6o=20B=2E=20Bar?= <author@example.com>
+EOF
+test_expect_success 'rfc2047-encoded headers leave no rfc822 specials' '
 	check_author "Föo B. Bar"
 '
 
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 5/5] format-patch: tests: check rfc822+rfc2047 in to+cc headers
  2012-10-08 17:33 [PATCH 0/5] Cure some format-patch wrapping and encoding issues Jan H. Schönherr
                   ` (3 preceding siblings ...)
  2012-10-08 17:33 ` [PATCH 4/5] format-patch: fix rfc2047 address encoding with respect to rfc822 specials Jan H. Schönherr
@ 2012-10-08 17:33 ` Jan H. Schönherr
       [not found]   ` <7v391nfmzn.fsf@alter.siamese.dyndns.org>
       [not found] ` <7vfw5nfoq9.fsf@alter.siamese.dyndns.org>
  5 siblings, 1 reply; 10+ messages in thread
From: Jan H. Schönherr @ 2012-10-08 17:33 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Jan H. Schönherr

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

Do some checks for RFC 822 and RFC 2047 support in To:
and Cc: headers and fix ambiguous old checks.

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
 t/t4014-format-patch.sh | 98 +++++++++++++++++++++++++++++++++----------------
 1 Datei geändert, 66 Zeilen hinzugefügt(+), 32 Zeilen entfernt(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 1a3b6e8..65ab4c9 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -110,73 +110,107 @@ test_expect_success 'replay did not screw up the log message' '
 
 test_expect_success 'extra headers' '
 
-	git config format.headers "To: R. E. Cipient <rcipient@example.com>
+	git config format.headers "To: R E Cipient <rcipient@example.com>
 " &&
-	git config --add format.headers "Cc: S. E. Cipient <scipient@example.com>
+	git config --add format.headers "Cc: S E Cipient <scipient@example.com>
 " &&
 	git format-patch --stdout master..side > patch2 &&
 	sed -e "/^\$/q" patch2 > hdrs2 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>\$" hdrs2 &&
-	grep "^Cc: S. E. Cipient <scipient@example.com>\$" hdrs2
+	grep "^To: R E Cipient <rcipient@example.com>\$" hdrs2 &&
+	grep "^Cc: S E Cipient <scipient@example.com>\$" hdrs2
 
 '
 
 test_expect_success 'extra headers without newlines' '
 
-	git config --replace-all format.headers "To: R. E. Cipient <rcipient@example.com>" &&
-	git config --add format.headers "Cc: S. E. Cipient <scipient@example.com>" &&
+	git config --replace-all format.headers "To: R E Cipient <rcipient@example.com>" &&
+	git config --add format.headers "Cc: S E Cipient <scipient@example.com>" &&
 	git format-patch --stdout master..side >patch3 &&
 	sed -e "/^\$/q" patch3 > hdrs3 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>\$" hdrs3 &&
-	grep "^Cc: S. E. Cipient <scipient@example.com>\$" hdrs3
+	grep "^To: R E Cipient <rcipient@example.com>\$" hdrs3 &&
+	grep "^Cc: S E Cipient <scipient@example.com>\$" hdrs3
 
 '
 
 test_expect_success 'extra headers with multiple To:s' '
 
-	git config --replace-all format.headers "To: R. E. Cipient <rcipient@example.com>" &&
-	git config --add format.headers "To: S. E. Cipient <scipient@example.com>" &&
+	git config --replace-all format.headers "To: R E Cipient <rcipient@example.com>" &&
+	git config --add format.headers "To: S E Cipient <scipient@example.com>" &&
 	git format-patch --stdout master..side > patch4 &&
 	sed -e "/^\$/q" patch4 > hdrs4 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>,\$" hdrs4 &&
-	grep "^ *S. E. Cipient <scipient@example.com>\$" hdrs4
+	grep "^To: R E Cipient <rcipient@example.com>,\$" hdrs4 &&
+	grep "^ *S E Cipient <scipient@example.com>\$" hdrs4
 '
 
-test_expect_success 'additional command line cc' '
+test_expect_success 'additional command line cc (ascii)' '
 
-	git config --replace-all format.headers "Cc: R. E. Cipient <rcipient@example.com>" &&
+	git config --replace-all format.headers "Cc: R E Cipient <rcipient@example.com>" &&
+	git format-patch --cc="S E Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch5 &&
+	grep "^Cc: R E Cipient <rcipient@example.com>,\$" patch5 &&
+	grep "^ *S E Cipient <scipient@example.com>\$" patch5
+'
+
+test_expect_failure 'additional command line cc (rfc822)' '
+
+	git config --replace-all format.headers "Cc: R E Cipient <rcipient@example.com>" &&
 	git format-patch --cc="S. E. Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch5 &&
-	grep "^Cc: R. E. Cipient <rcipient@example.com>,\$" patch5 &&
-	grep "^ *S. E. Cipient <scipient@example.com>\$" patch5
+	grep "^Cc: R E Cipient <rcipient@example.com>,\$" patch5 &&
+	grep "^ *"S. E. Cipient" <scipient@example.com>\$" patch5
 '
 
 test_expect_success 'command line headers' '
 
 	git config --unset-all format.headers &&
-	git format-patch --add-header="Cc: R. E. Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch6 &&
-	grep "^Cc: R. E. Cipient <rcipient@example.com>\$" patch6
+	git format-patch --add-header="Cc: R E Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch6 &&
+	grep "^Cc: R E Cipient <rcipient@example.com>\$" patch6
 '
 
 test_expect_success 'configuration headers and command line headers' '
 
-	git config --replace-all format.headers "Cc: R. E. Cipient <rcipient@example.com>" &&
-	git format-patch --add-header="Cc: S. E. Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch7 &&
-	grep "^Cc: R. E. Cipient <rcipient@example.com>,\$" patch7 &&
-	grep "^ *S. E. Cipient <scipient@example.com>\$" patch7
+	git config --replace-all format.headers "Cc: R E Cipient <rcipient@example.com>" &&
+	git format-patch --add-header="Cc: S E Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch7 &&
+	grep "^Cc: R E Cipient <rcipient@example.com>,\$" patch7 &&
+	grep "^ *S E Cipient <scipient@example.com>\$" patch7
 '
 
-test_expect_success 'command line To: header' '
+test_expect_success 'command line To: header (ascii)' '
 
 	git config --unset-all format.headers &&
+	git format-patch --to="R E Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch8 &&
+	grep "^To: R E Cipient <rcipient@example.com>\$" patch8
+'
+
+test_expect_failure 'command line To: header (rfc822)' '
+
 	git format-patch --to="R. E. Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch8 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>\$" patch8
+	grep "^To: "R. E. Cipient" <rcipient@example.com>\$" patch8
+'
+
+test_expect_failure 'command line To: header (rfc2047)' '
+
+	git format-patch --to="R Ä Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch8 &&
+	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" patch8
 '
 
-test_expect_success 'configuration To: header' '
+test_expect_success 'configuration To: header (ascii)' '
+
+	git config format.to "R E Cipient <rcipient@example.com>" &&
+	git format-patch --stdout master..side | sed -e "/^\$/q" >patch9 &&
+	grep "^To: R E Cipient <rcipient@example.com>\$" patch9
+'
+
+test_expect_failure 'configuration To: header (rfc822)' '
 
 	git config format.to "R. E. Cipient <rcipient@example.com>" &&
 	git format-patch --stdout master..side | sed -e "/^\$/q" >patch9 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>\$" patch9
+	grep "^To: "R. E. Cipient" <rcipient@example.com>\$" patch9
+'
+
+test_expect_failure 'configuration To: header (rfc2047)' '
+
+	git config format.to "R Ä Cipient <rcipient@example.com>" &&
+	git format-patch --stdout master..side | sed -e "/^\$/q" >patch9 &&
+	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" patch9
 '
 
 # check_patch <patch>: Verify that <patch> looks like a half-sane
@@ -190,11 +224,11 @@ check_patch () {
 test_expect_success '--no-to overrides config.to' '
 
 	git config --replace-all format.to \
-		"R. E. Cipient <rcipient@example.com>" &&
+		"R E Cipient <rcipient@example.com>" &&
 	git format-patch --no-to --stdout master..side |
 	sed -e "/^\$/q" >patch10 &&
 	check_patch patch10 &&
-	! grep "^To: R. E. Cipient <rcipient@example.com>\$" patch10
+	! grep "^To: R E Cipient <rcipient@example.com>\$" patch10
 '
 
 test_expect_success '--no-to and --to replaces config.to' '
@@ -212,21 +246,21 @@ test_expect_success '--no-to and --to replaces config.to' '
 test_expect_success '--no-cc overrides config.cc' '
 
 	git config --replace-all format.cc \
-		"C. E. Cipient <rcipient@example.com>" &&
+		"C E Cipient <rcipient@example.com>" &&
 	git format-patch --no-cc --stdout master..side |
 	sed -e "/^\$/q" >patch12 &&
 	check_patch patch12 &&
-	! grep "^Cc: C. E. Cipient <rcipient@example.com>\$" patch12
+	! grep "^Cc: C E Cipient <rcipient@example.com>\$" patch12
 '
 
 test_expect_success '--no-add-header overrides config.headers' '
 
 	git config --replace-all format.headers \
-		"Header1: B. E. Cipient <rcipient@example.com>" &&
+		"Header1: B E Cipient <rcipient@example.com>" &&
 	git format-patch --no-add-header --stdout master..side |
 	sed -e "/^\$/q" >patch13 &&
 	check_patch patch13 &&
-	! grep "^Header1: B. E. Cipient <rcipient@example.com>\$" patch13
+	! grep "^Header1: B E Cipient <rcipient@example.com>\$" patch13
 '
 
 test_expect_success 'multiple files' '
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/5] format-patch: do not wrap rfc2047 encoded headers too late
       [not found]   ` <7v7gqzfnpj.fsf@alter.siamese.dyndns.org>
@ 2012-10-10  9:31     ` "Jan H. Schönherr"
  0 siblings, 0 replies; 10+ messages in thread
From: "Jan H. Schönherr" @ 2012-10-10  9:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

Am 09.10.2012 21:30, schrieb Junio C Hamano:
> Jan H. Schönherr <schnhrr@cs.tu-berlin.de> writes:
...
>>  static int is_rfc2047_special(char ch)
>>  {
>> +	/*
>> +	 * We encode ' ' using '=20' even though rfc2047
>> +	 * allows using '_' for readability.  Unfortunately,
>> +	 * many programs do not understand this and just
>> +	 * leave the underscore in place.
>> +	 */
> 
> The sentence break made me read the above three times to understand
> what it is trying to say.  "Unfortunately" refers to what happens if
> we were to use '_', but it initially appeared to be describing some
> bug due to our encoding ' ' as '=20'.  Perhaps like this?
> 
> 	/*
> 	 * rfc2047 allows '_' to encode ' ' for readability, but
> 	 * many programs do not understand ...; encode ' ' using
> 	 * '=20' instead to avoid the problem.
> 	 */

I was just moving that comment (and the following check) around,
but I'll update the comment in the next version.

>> +	if (ch == ' ' || ch == '\n')
>> +		return 1;
> 
> The comment justifies why this "if (ch == ' ')", which could be part
> of the "return" below, separately is done, but nothing explains why
> you add '\n' (and not other controls, e.g. '\t') to the mix.

The check for '\n' was introduced in commit c22e7de3
("format-patch: rfc2047-encode newlines in headers").

The commit log was:

    These should generally never happen, as we already
    concatenate multiples in subjects into a single line. But
    let's be defensive, since not encoding them means we will
    output malformed headers.

Having again a look at RFC 2047, I see that we should be
even more strict and not allow any non-printable character to
be passed through unencoded. I guess that adds another patch to
the series. Hmm... Maybe I can split patch 4 into two patches,
one that mostly fixes is_rfc2047_special() and one that
avoids 822 quoting when doing 2047 encoding.

> 
>>  	return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
>>  }
>>  
>>  static void add_rfc2047(struct strbuf *sb, const char *line, int len,
>>  		       const char *encoding)
>>  {
>> -	static const int max_length = 78; /* per rfc2822 */
>> +	static const int max_length = 76; /* per rfc2047 */
>>  	int i;
>>  	int line_len;
>>  
>> @@ -286,7 +295,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
>>  		if ((i + 1 < len) && (ch == '=' && line[i+1] == '?'))
>>  			goto needquote;
>>  	}
>> -	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, max_length+1);
>> +	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, 78+1);
>>  	return;
> 
> Yuck.  If you do want to retain 78 for non-quoted output for
> backward compatibility, that is OK, but if that is the case, please
> introduce a new constant "max_quoted_length" or something to stand
> for 76 and use it in the "needquote:" part below.

Will do.

Regards
Jan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 5/5] format-patch: tests: check rfc822+rfc2047 in to+cc headers
       [not found]   ` <7v391nfmzn.fsf@alter.siamese.dyndns.org>
@ 2012-10-10 10:44     ` "Jan H. Schönherr"
  2012-10-10 17:02       ` Junio C Hamano
  0 siblings, 1 reply; 10+ messages in thread
From: "Jan H. Schönherr" @ 2012-10-10 10:44 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

Am 09.10.2012 21:45, schrieb Junio C Hamano:
> Jan H. Schönherr <schnhrr@cs.tu-berlin.de> writes:
> 
>> +test_expect_failure 'additional command line cc (rfc822)' '
>> +
>> +	git config --replace-all format.headers "Cc: R E Cipient <rcipient@example.com>" &&
>>  	git format-patch --cc="S. E. Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch5 &&
>> -	grep "^Cc: R. E. Cipient <rcipient@example.com>,\$" patch5 &&
>> -	grep "^ *S. E. Cipient <scipient@example.com>\$" patch5
>> +	grep "^Cc: R E Cipient <rcipient@example.com>,\$" patch5 &&
>> +	grep "^ *"S. E. Cipient" <scipient@example.com>\$" patch5
> 
> Hrm.
> 
> As we are not in the business of parsing out whatever junk given
> with --cc or --recipient from the command line or configuration, but
> are merely parroting them to the output stream, isn't this a
> user-error in the test that gives --cc='S. E. Cipient <a@ddre.ss>'
> instead of giving --cc='"S. E. Cipient" <a@ddre.ss>'?  Same comment
> on the new 'expect-failure' tests.

Originally, I just wanted to emphasize, that --to and --cc are
currently handled differently than in git-send-email, where
all this quoting/encoding is done.

And it is much more convenient to add
	--cc 'Jan H. Schönherr <...>'
than
	--cc '=?UTF-8?q?Jan=20H=2E=20Sch=C3=B6nherr?= <...>'

Even more, since I would expect git to correctly handle
addresses given in a format that is also used elsewhere
within git.


However, I agree that we are not responsible to check/quote/encode
anything when the user supplies whole headers (though we probably
could).


But if I cannot convince you, I'll just drop this patch. :)

Regards
Jan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/5] Cure some format-patch wrapping and encoding issues
       [not found] ` <7vfw5nfoq9.fsf@alter.siamese.dyndns.org>
@ 2012-10-10 10:49   ` "Jan H. Schönherr"
  0 siblings, 0 replies; 10+ messages in thread
From: "Jan H. Schönherr" @ 2012-10-10 10:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

Am 09.10.2012 21:07, schrieb Junio C Hamano:
> Jan H. Schönherr <schnhrr@cs.tu-berlin.de> writes:
> 
>> During the creation of this series, I came across the strbuf 
>> wrapping functions, and I wonder if there is an off-by-one issue.
>>
>> Consider the following excerpt from t4202:
...
> 
> Yeah, that does sound like an off-by-one bug.  When we as end users
> say %w(72), we do expect some lines fill to the 72nd column, not
> stopping at the 71st.  I suspect that dates back to the very first
> implementation of %w() but I think we should fix it (perhaps as a
> separate patch either the earliest or the last in the series).

I will include a fix for that, then.

(But I won't be able the send out the next round of this series
before next week.)

Regards
Jan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 5/5] format-patch: tests: check rfc822+rfc2047 in to+cc headers
  2012-10-10 10:44     ` "Jan H. Schönherr"
@ 2012-10-10 17:02       ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2012-10-10 17:02 UTC (permalink / raw)
  To: Jan H. Schönherr; +Cc: git, Jeff King

"Jan H. Schönherr" <schnhrr@cs.tu-berlin.de> writes:

> Am 09.10.2012 21:45, schrieb Junio C Hamano:
>> Jan H. Schönherr <schnhrr@cs.tu-berlin.de> writes:
>> 
>>> +test_expect_failure 'additional command line cc (rfc822)' '
>>> +
>>> +	git config --replace-all format.headers "Cc: R E Cipient <rcipient@example.com>" &&
>>>  	git format-patch --cc="S. E. Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch5 &&
>>> -	grep "^Cc: R. E. Cipient <rcipient@example.com>,\$" patch5 &&
>>> -	grep "^ *S. E. Cipient <scipient@example.com>\$" patch5
>>> +	grep "^Cc: R E Cipient <rcipient@example.com>,\$" patch5 &&
>>> +	grep "^ *"S. E. Cipient" <scipient@example.com>\$" patch5
>> 
>> Hrm.
>> 
>> As we are not in the business of parsing out whatever junk given
>> with --cc or --recipient from the command line or configuration, but
>> are merely parroting them to the output stream, isn't this a
>> user-error in the test that gives --cc='S. E. Cipient <a@ddre.ss>'
>> instead of giving --cc='"S. E. Cipient" <a@ddre.ss>'?  Same comment
>> on the new 'expect-failure' tests.
>
> Originally, I just wanted to emphasize, that --to and --cc are
> currently handled differently than in git-send-email, where
> all this quoting/encoding is done.
>
> And it is much more convenient to add
> 	--cc 'Jan H. Schönherr <...>'
> than
> 	--cc '=?UTF-8?q?Jan=20H=2E=20Sch=C3=B6nherr?= <...>'
>
> Even more, since I would expect git to correctly handle
> addresses given in a format that is also used elsewhere
> within git.
>
>
> However, I agree that we are not responsible to check/quote/encode
> anything when the user supplies whole headers (though we probably
> could).
>
> But if I cannot convince you, I'll just drop this patch. :)

It wasn't about convincing or not convincing me.

I couldn't read, just from "expect_failure" and "Do some checks
for...", what the intention of the tests and the proposed future
plans were.

If the proposed commit log message (or comments before these
"expect_failure" tests) said something like this:

    "git send-email" historically did not parse the user supplied
    extra header values (e.g. --cc, --recipient) and just replayed
    them, but that forces users to add them in encoded form, e.g.

 	--cc '=?UTF-8?q?Jan=20H=2E=20Sch=C3=B6nherr?= <...>'

    which is inconvenient. We would want to update send-email to
    accept human-readable

 	--cc 'Jan H. Schönherr <...>'

    and encode in the future.  Add test_expect_failure tests as a
    reminder.

that would have avoided such confusion, and even more importantly,
made it easier for us to start discussion on the proposed future
direction.  I am personally on the fence.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2012-10-10 17:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-08 17:33 [PATCH 0/5] Cure some format-patch wrapping and encoding issues Jan H. Schönherr
2012-10-08 17:33 ` [PATCH 1/5] format-patch: do not wrap non-rfc2047 headers too early Jan H. Schönherr
2012-10-08 17:33 ` [PATCH 2/5] format-patch: do not wrap rfc2047 encoded headers too late Jan H. Schönherr
     [not found]   ` <7v7gqzfnpj.fsf@alter.siamese.dyndns.org>
2012-10-10  9:31     ` "Jan H. Schönherr"
2012-10-08 17:33 ` [PATCH 3/5] format-patch: introduce helper function last_line_length() Jan H. Schönherr
2012-10-08 17:33 ` [PATCH 4/5] format-patch: fix rfc2047 address encoding with respect to rfc822 specials Jan H. Schönherr
2012-10-08 17:33 ` [PATCH 5/5] format-patch: tests: check rfc822+rfc2047 in to+cc headers Jan H. Schönherr
     [not found]   ` <7v391nfmzn.fsf@alter.siamese.dyndns.org>
2012-10-10 10:44     ` "Jan H. Schönherr"
2012-10-10 17:02       ` Junio C Hamano
     [not found] ` <7vfw5nfoq9.fsf@alter.siamese.dyndns.org>
2012-10-10 10:49   ` [PATCH 0/5] Cure some format-patch wrapping and encoding issues "Jan H. Schönherr"

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).