* [PATCH b4 v2 0/6] Handle patch trailers with special characters
@ 2024-10-27 22:34 Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 1/6] src/tests: check trailer extinfo Brandon Maier
` (6 more replies)
0 siblings, 7 replies; 8+ messages in thread
From: Brandon Maier @ 2024-10-27 22:34 UTC (permalink / raw)
To: Kernel.org Tools; +Cc: Brandon Maier, Konstantin Ryabitsev
Fixes bug reported in https://github.com/mricon/b4/issues/50
Patch trailers that contain commas are not parsed correctly, this causes
the trailer to be set to a blank string. The issue is special characters
in emails are supposed to be quoted per RFC 2822. But it's fairly common
for patch trailers to be hand-written and be improperly formatted.
The trailer tests were hiding some of the parsing issues, so the first
few patches fix up the test.
While fixing this bug I discovered the trailer parser also breaks even
when the trailer is properly quoted, so I fix that in this series as
well.
Signed-off-by: Brandon Maier <brandon.maier@gmail.com>
---
Changes in v2:
- Fixed other tests that broke when the email parsing changed
- Link to v1: https://patch.msgid.link/20241027-trailer-special-chars-v1-0-1bd180dba425@gmail.com
---
Brandon Maier (6):
src/tests: check trailer extinfo
src/tests: fix trailer test hiding parser errors
b4: handle trailers with improper formatting
src/tests: test for trailers with improper formatting
b4: handle trailers with special characters
src/tests: test for trailers with special characters
src/b4/__init__.py | 6 +++---
.../samples/trailers-followup-bare-address-ref-defaults.txt | 2 +-
.../samples/trailers-followup-name-parens-ref-defaults.txt | 2 +-
src/tests/samples/trailers-test-extinfo.txt | 1 +
src/tests/samples/trailers-test-simple.txt | 1 +
src/tests/test___init__.py | 13 ++++++++++---
6 files changed, 17 insertions(+), 8 deletions(-)
---
base-commit: 2a6338e451a0c1e81f214f48c820c1e52d76b2f1
change-id: 20241027-trailer-special-chars-78b97ff63ea9
Best regards,
--
Brandon Maier <brandon.maier@gmail.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH b4 v2 1/6] src/tests: check trailer extinfo
2024-10-27 22:34 [PATCH b4 v2 0/6] Handle patch trailers with special characters Brandon Maier
@ 2024-10-27 22:34 ` Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 2/6] src/tests: fix trailer test hiding parser errors Brandon Maier
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Brandon Maier @ 2024-10-27 22:34 UTC (permalink / raw)
To: Kernel.org Tools; +Cc: Brandon Maier, Konstantin Ryabitsev
The b4.LoreTrailer.__eq__ ignores the 'extinfo' of a trailer. So the
trailer test is not checking that extinfo is parsed correctly.
Signed-off-by: Brandon Maier <brandon.maier@gmail.com>
---
src/tests/test___init__.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/tests/test___init__.py b/src/tests/test___init__.py
index 297e5120a8c8adf241660f06e9a83c1ff4a0fd3a..dbc92c7e8352728c2f2a951f7b9b1496ccbaf2c5 100644
--- a/src/tests/test___init__.py
+++ b/src/tests/test___init__.py
@@ -79,6 +79,7 @@ def test_parse_trailers(sampledir, source, expected):
mytype, myname, myvalue, myextinfo = expected.pop(0)
mytr = b4.LoreTrailer(name=myname, value=myvalue, extinfo=myextinfo)
assert tr == mytr
+ assert tr.extinfo == mytr.extinfo
assert tr.type == mytype
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH b4 v2 2/6] src/tests: fix trailer test hiding parser errors
2024-10-27 22:34 [PATCH b4 v2 0/6] Handle patch trailers with special characters Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 1/6] src/tests: check trailer extinfo Brandon Maier
@ 2024-10-27 22:34 ` Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 3/6] b4: handle trailers with improper formatting Brandon Maier
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Brandon Maier @ 2024-10-27 22:34 UTC (permalink / raw)
To: Kernel.org Tools; +Cc: Brandon Maier, Konstantin Ryabitsev
The trailer parsing test compares the values of the b4.LoreTrailer
object created from the test input against a b4.LoreTrailer object
created from our expected test input.
This means if there is an error in how b4.LoreTrailer parses input, it
won't be detected. This was discovered from a reported bug with parsing
email trailers with embedded ','.
Bug: https://github.com/mricon/b4/issues/50
---
src/tests/test___init__.py | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/tests/test___init__.py b/src/tests/test___init__.py
index dbc92c7e8352728c2f2a951f7b9b1496ccbaf2c5..7fe43b0158460f06d0a2b52d2689d26e7ef7f840 100644
--- a/src/tests/test___init__.py
+++ b/src/tests/test___init__.py
@@ -57,7 +57,7 @@ def test_save_git_am_mbox(sampledir, tmp_path, source, regex, flags, ismbox):
@pytest.mark.parametrize('source,expected', [
('trailers-test-simple',
- [('person', 'Reviewed-By', 'Bogus Bupkes <bogus@example.com>', None),
+ [('person', 'Reviewed-by', 'Bogus Bupkes <bogus@example.com>', None),
('utility', 'Fixes', 'abcdef01234567890', None),
('utility', 'Link', 'https://msgid.link/some@msgid.here', None),
]),
@@ -77,10 +77,14 @@ def test_parse_trailers(sampledir, source, expected):
assert len(expected) == len(trs)
for tr in trs:
mytype, myname, myvalue, myextinfo = expected.pop(0)
+ assert tr.name == myname
+ assert tr.value == myvalue
+ assert tr.extinfo == myextinfo
+ assert tr.type == mytype
+
mytr = b4.LoreTrailer(name=myname, value=myvalue, extinfo=myextinfo)
assert tr == mytr
assert tr.extinfo == mytr.extinfo
- assert tr.type == mytype
@pytest.mark.parametrize('source,serargs,amargs,reference,b4cfg', [
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH b4 v2 3/6] b4: handle trailers with improper formatting
2024-10-27 22:34 [PATCH b4 v2 0/6] Handle patch trailers with special characters Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 1/6] src/tests: check trailer extinfo Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 2/6] src/tests: fix trailer test hiding parser errors Brandon Maier
@ 2024-10-27 22:34 ` Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 4/6] src/tests: test for " Brandon Maier
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Brandon Maier @ 2024-10-27 22:34 UTC (permalink / raw)
To: Kernel.org Tools; +Cc: Brandon Maier, Konstantin Ryabitsev
A patch trailer with an improperly formatted email parses to an empty
string. For example see the linked github issue, where a trailer's
display name contains an unquoted comma.
As patch trailers are often hand written this can happen frequently. Try
to gracefully handle this case by checking if an email parses correctly,
and if it doesn't fallback to displaying it verbatim.
Fixes: https://github.com/mricon/b4/issues/50
Signed-off-by: Brandon Maier <brandon.maier@gmail.com>
---
src/b4/__init__.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/b4/__init__.py b/src/b4/__init__.py
index 5115a9cd9b8746f84eb104a2be69fe121724167e..cf112e3bce2911c78cff18aac267608f796957ab 100644
--- a/src/b4/__init__.py
+++ b/src/b4/__init__.py
@@ -1063,8 +1063,10 @@ def __init__(self, name: Optional[str] = None, value: Optional[str] = None, exti
# Normalize the value with parsed data
if self.addr[0]:
self.value = f'{self.addr[0]} <{self.addr[1]}>'
- else:
+ elif self.addr[1]:
self.value = self.addr[1]
+ else:
+ self.type = 'unknown'
else:
self.type = 'unknown'
self.lname = self.name.lower()
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH b4 v2 4/6] src/tests: test for trailers with improper formatting
2024-10-27 22:34 [PATCH b4 v2 0/6] Handle patch trailers with special characters Brandon Maier
` (2 preceding siblings ...)
2024-10-27 22:34 ` [PATCH b4 v2 3/6] b4: handle trailers with improper formatting Brandon Maier
@ 2024-10-27 22:34 ` Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 5/6] b4: handle trailers with special characters Brandon Maier
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Brandon Maier @ 2024-10-27 22:34 UTC (permalink / raw)
To: Kernel.org Tools; +Cc: Brandon Maier, Konstantin Ryabitsev
Check that a trailer with an improperly formatted email address will
still parse to something usable.
Signed-off-by: Brandon Maier <brandon.maier@gmail.com>
---
src/tests/samples/trailers-test-extinfo.txt | 1 +
src/tests/test___init__.py | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/tests/samples/trailers-test-extinfo.txt b/src/tests/samples/trailers-test-extinfo.txt
index aa61e8d635923c2e2539b82c60f69b489ad7cee3..5417cdd87bfdf4ddb278e125a6dd04d2e86af141 100644
--- a/src/tests/samples/trailers-test-extinfo.txt
+++ b/src/tests/samples/trailers-test-extinfo.txt
@@ -4,6 +4,7 @@ Date: Tue, 30 Aug 2022 11:19:07 -0400
This is a simple trailer parsing test.
+Reported-by: Some, One <somewhere@example.com>
Reviewed-by: Bogus Bupkes <bogus@example.com>
[for the parts that are bogus]
Fixes: abcdef01234567890
diff --git a/src/tests/test___init__.py b/src/tests/test___init__.py
index 7fe43b0158460f06d0a2b52d2689d26e7ef7f840..85ccd190677daebc30b9987eb811d44a15dce79b 100644
--- a/src/tests/test___init__.py
+++ b/src/tests/test___init__.py
@@ -62,7 +62,8 @@ def test_save_git_am_mbox(sampledir, tmp_path, source, regex, flags, ismbox):
('utility', 'Link', 'https://msgid.link/some@msgid.here', None),
]),
('trailers-test-extinfo',
- [('person', 'Reviewed-by', 'Bogus Bupkes <bogus@example.com>', '[for the parts that are bogus]'),
+ [('unknown', 'Reported-by', 'Some, One <somewhere@example.com>', None),
+ ('person', 'Reviewed-by', 'Bogus Bupkes <bogus@example.com>', '[for the parts that are bogus]'),
('utility', 'Fixes', 'abcdef01234567890', None),
('person', 'Tested-by', 'Some Person <bogus2@example.com>', ' [this person visually indented theirs]'),
('utility', 'Link', 'https://msgid.link/some@msgid.here', ' # initial submission'),
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH b4 v2 5/6] b4: handle trailers with special characters
2024-10-27 22:34 [PATCH b4 v2 0/6] Handle patch trailers with special characters Brandon Maier
` (3 preceding siblings ...)
2024-10-27 22:34 ` [PATCH b4 v2 4/6] src/tests: test for " Brandon Maier
@ 2024-10-27 22:34 ` Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 6/6] src/tests: test for " Brandon Maier
2025-01-22 15:40 ` [PATCH b4 v2 0/6] Handle patch " Konstantin Ryabitsev
6 siblings, 0 replies; 8+ messages in thread
From: Brandon Maier @ 2024-10-27 22:34 UTC (permalink / raw)
To: Kernel.org Tools; +Cc: Brandon Maier, Konstantin Ryabitsev
Parsing an email trailer with special characters breaks the formatting.
For example this trailer.
> Reviewed-by: "Doe, Jane" <jane@example.com>
Will get parsed into this.
> Reviewed-by: Doe, Jane <jane@example.com>
This is because b4.LoreTrailer attempts to reformat the trailer using a
naive string format. Instead use the Python standard library formataddr
function which correctly handles special characters.
Signed-off-by: Brandon Maier <brandon.maier@gmail.com>
---
src/b4/__init__.py | 6 ++----
src/tests/samples/trailers-followup-bare-address-ref-defaults.txt | 2 +-
src/tests/samples/trailers-followup-name-parens-ref-defaults.txt | 2 +-
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/b4/__init__.py b/src/b4/__init__.py
index cf112e3bce2911c78cff18aac267608f796957ab..0785be8ffcef08fb5cd4c484a40e6fe856446cfe 100644
--- a/src/b4/__init__.py
+++ b/src/b4/__init__.py
@@ -1061,10 +1061,8 @@ def __init__(self, name: Optional[str] = None, value: Optional[str] = None, exti
self.type = 'person'
self.addr = email.utils.parseaddr(value)
# Normalize the value with parsed data
- if self.addr[0]:
- self.value = f'{self.addr[0]} <{self.addr[1]}>'
- elif self.addr[1]:
- self.value = self.addr[1]
+ if self.addr[0] or self.addr[1]:
+ self.value = email.utils.formataddr(self.addr)
else:
self.type = 'unknown'
else:
diff --git a/src/tests/samples/trailers-followup-bare-address-ref-defaults.txt b/src/tests/samples/trailers-followup-bare-address-ref-defaults.txt
index 7b1ebef3c9d9b9c60288dc07bcf7f82f066c6934..82e1576019bd9f4d69f9a2ed9b783340a5ea16b2 100644
--- a/src/tests/samples/trailers-followup-bare-address-ref-defaults.txt
+++ b/src/tests/samples/trailers-followup-bare-address-ref-defaults.txt
@@ -14,7 +14,7 @@ Cc: bare-address@example.org
Reviewed-by: Original Reviewer <original-reviewer@example.com>
Link: https://msgid.link/some@msgid.here
Signed-off-by: Original Submitter <original-submitter@example.com>
-Reviewed-by: Followup Reviewer1 (corporate) <followup-reviewer1@example.com>
+Reviewed-by: "Followup Reviewer1 (corporate)" <followup-reviewer1@example.com>
Tested-by: Followup Reviewer2 <followup-reviewer2@example.com>
diff --git a/b4/junk.py b/b4/junk.py
diff --git a/src/tests/samples/trailers-followup-name-parens-ref-defaults.txt b/src/tests/samples/trailers-followup-name-parens-ref-defaults.txt
index 06b883126bd3ea9f7a4d22c58fc10e9e89d6bde7..dfc013c89c415107ff2517c1715e523982df2afc 100644
--- a/src/tests/samples/trailers-followup-name-parens-ref-defaults.txt
+++ b/src/tests/samples/trailers-followup-name-parens-ref-defaults.txt
@@ -13,7 +13,7 @@ Fixes: abcdef01234567890
Reviewed-by: Original Reviewer <original-reviewer@example.com>
Link: https://msgid.link/some@msgid.here
Signed-off-by: Original Submitter <original-submitter@example.com>
-Reviewed-by: Followup Reviewer1 (corporate) <followup-reviewer1@example.com>
+Reviewed-by: "Followup Reviewer1 (corporate)" <followup-reviewer1@example.com>
Tested-by: Followup Reviewer2 <followup-reviewer2@example.com>
diff --git a/b4/junk.py b/b4/junk.py
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH b4 v2 6/6] src/tests: test for trailers with special characters
2024-10-27 22:34 [PATCH b4 v2 0/6] Handle patch trailers with special characters Brandon Maier
` (4 preceding siblings ...)
2024-10-27 22:34 ` [PATCH b4 v2 5/6] b4: handle trailers with special characters Brandon Maier
@ 2024-10-27 22:34 ` Brandon Maier
2025-01-22 15:40 ` [PATCH b4 v2 0/6] Handle patch " Konstantin Ryabitsev
6 siblings, 0 replies; 8+ messages in thread
From: Brandon Maier @ 2024-10-27 22:34 UTC (permalink / raw)
To: Kernel.org Tools; +Cc: Brandon Maier, Konstantin Ryabitsev
Test that email trailers with special characters are parsed correctly.
Signed-off-by: Brandon Maier <brandon.maier@gmail.com>
---
src/tests/samples/trailers-test-simple.txt | 1 +
src/tests/test___init__.py | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/tests/samples/trailers-test-simple.txt b/src/tests/samples/trailers-test-simple.txt
index da64caff8dfaa0cbc630c8ae3515f4508cfd0764..a82416354012d3c32f99b39d1918ac343f2b66cc 100644
--- a/src/tests/samples/trailers-test-simple.txt
+++ b/src/tests/samples/trailers-test-simple.txt
@@ -4,6 +4,7 @@ Date: Tue, 30 Aug 2022 11:19:07 -0400
This is a simple trailer parsing test.
+Reported-by: "Doe, Jane" <jane@example.com>
Reviewed-by: Bogus Bupkes <bogus@example.com>
Fixes: abcdef01234567890
Link: https://msgid.link/some@msgid.here
diff --git a/src/tests/test___init__.py b/src/tests/test___init__.py
index 85ccd190677daebc30b9987eb811d44a15dce79b..a4637b9c797367da462f062511fdf4e018a5b81e 100644
--- a/src/tests/test___init__.py
+++ b/src/tests/test___init__.py
@@ -57,7 +57,8 @@ def test_save_git_am_mbox(sampledir, tmp_path, source, regex, flags, ismbox):
@pytest.mark.parametrize('source,expected', [
('trailers-test-simple',
- [('person', 'Reviewed-by', 'Bogus Bupkes <bogus@example.com>', None),
+ [('person', 'Reported-by', '"Doe, Jane" <jane@example.com>', None),
+ ('person', 'Reviewed-by', 'Bogus Bupkes <bogus@example.com>', None),
('utility', 'Fixes', 'abcdef01234567890', None),
('utility', 'Link', 'https://msgid.link/some@msgid.here', None),
]),
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH b4 v2 0/6] Handle patch trailers with special characters
2024-10-27 22:34 [PATCH b4 v2 0/6] Handle patch trailers with special characters Brandon Maier
` (5 preceding siblings ...)
2024-10-27 22:34 ` [PATCH b4 v2 6/6] src/tests: test for " Brandon Maier
@ 2025-01-22 15:40 ` Konstantin Ryabitsev
6 siblings, 0 replies; 8+ messages in thread
From: Konstantin Ryabitsev @ 2025-01-22 15:40 UTC (permalink / raw)
To: Kernel.org Tools, Brandon Maier
On Sun, 27 Oct 2024 17:34:53 -0500, Brandon Maier wrote:
> Fixes bug reported in https://github.com/mricon/b4/issues/50
>
> Patch trailers that contain commas are not parsed correctly, this causes
> the trailer to be set to a blank string. The issue is special characters
> in emails are supposed to be quoted per RFC 2822. But it's fairly common
> for patch trailers to be hand-written and be improperly formatted.
>
> [...]
Applied, thanks!
[1/6] src/tests: check trailer extinfo
commit: 0d0d2e2e53013302d0625eefb4ee519166802c1b
[2/6] src/tests: fix trailer test hiding parser errors
commit: 285445c83c066f8ba39657d8bf360df729b60002
[3/6] b4: handle trailers with improper formatting
commit: cc2d6bfbc6d2b819d8d291c3b7470642893d69fa
[4/6] src/tests: test for trailers with improper formatting
commit: e1ba36d1a62be7972c46668c89214dda00b8d6f9
[5/6] b4: handle trailers with special characters
commit: 83327d2fbad9506afc2a3e30566025345e157eca
[6/6] src/tests: test for trailers with special characters
commit: f147f985fa6ad6558603b83fa0a7bf38aeefd436
Best regards,
--
Konstantin Ryabitsev <konstantin@linuxfoundation.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-01-22 15:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-27 22:34 [PATCH b4 v2 0/6] Handle patch trailers with special characters Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 1/6] src/tests: check trailer extinfo Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 2/6] src/tests: fix trailer test hiding parser errors Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 3/6] b4: handle trailers with improper formatting Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 4/6] src/tests: test for " Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 5/6] b4: handle trailers with special characters Brandon Maier
2024-10-27 22:34 ` [PATCH b4 v2 6/6] src/tests: test for " Brandon Maier
2025-01-22 15:40 ` [PATCH b4 v2 0/6] Handle patch " Konstantin Ryabitsev
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).