-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
gh-135307: Fix email error when policy max_line_length is set to 0 or None #135367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
a210d49
e9de710
0a5f379
d0dc3c0
77ea798
52e590c
603ff74
38e6381
fa173ac
c24ec76
befa25d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| import email.message | ||
| import email.errors | ||
| from email import quoprimime | ||
| import sys | ||
|
|
||
| class ContentManager: | ||
|
|
||
|
|
@@ -142,22 +143,23 @@ def _encode_base64(data, max_line_length): | |
|
|
||
|
|
||
| def _encode_text(string, charset, cte, policy): | ||
| # max_line_length 0/None means no limit, ie: infinitely long. | ||
|
zangjiucheng marked this conversation as resolved.
Outdated
|
||
| maxlen = policy.max_line_length or sys.maxsize | ||
| lines = string.encode(charset).splitlines() | ||
| linesep = policy.linesep.encode('ascii') | ||
| def embedded_body(lines): return linesep.join(lines) + linesep | ||
| def normal_body(lines): return b'\n'.join(lines) + b'\n' | ||
| if cte is None: | ||
| # Use heuristics to decide on the "best" encoding. | ||
| if max((len(x) for x in lines), default=0) <= policy.max_line_length: | ||
| if max((len(x) for x in lines), default=0) <= maxlen: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use: @bitdancer What is the style you'd recommend in general in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just my comment: we're not required to take the fastest route here, since we may not have a heavy load on the email module. Still wish to hear more comments before we resolve this :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, mailman can put a pretty heavy load on the email package, and it is one of the biggest consumers of the package. While I know there are opportunities for performance improvements in the new email code that I haven't gotten around to investigating, I personally tend to do what others call micro-optimizations when I notice them[*], unless they make the code really hard to understand. Not that I know which is faster, here. [*] I read an article once where Knuth felt his comment had been misinterpreted, and pointed out that if you routinely ignore optimization opportunities you pretty quickly end up with a really slow program.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@bitdancer Thanks for sharing the additional context — it’s really helpful to know there’s a concrete performance impact for real users like Mailman. I agree that starting with small, focused micro-optimizations makes sense, especially since we’re planning a broader optimization pass for the email module. I’ve incorporated your suggestion and updated the comment in the code accordingly.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FTR, I don't think this will change much because we've seen times when code is slower with an map() and sometimes slower with a comprehension. If we want to make the code faster, we should run better benchmarks. |
||
| try: | ||
| return '7bit', normal_body(lines).decode('ascii') | ||
| except UnicodeDecodeError: | ||
| pass | ||
| if policy.cte_type == '8bit': | ||
| return '8bit', normal_body(lines).decode('ascii', 'surrogateescape') | ||
| sniff = embedded_body(lines[:10]) | ||
| sniff_qp = quoprimime.body_encode(sniff.decode('latin-1'), | ||
| policy.max_line_length) | ||
| sniff_qp = quoprimime.body_encode(sniff.decode('latin-1'), maxlen) | ||
| sniff_base64 = binascii.b2a_base64(sniff) | ||
| # This is a little unfair to qp; it includes lineseps, base64 doesn't. | ||
| if len(sniff_qp) > len(sniff_base64): | ||
|
|
@@ -172,9 +174,9 @@ def normal_body(lines): return b'\n'.join(lines) + b'\n' | |
| data = normal_body(lines).decode('ascii', 'surrogateescape') | ||
| elif cte == 'quoted-printable': | ||
| data = quoprimime.body_encode(normal_body(lines).decode('latin-1'), | ||
| policy.max_line_length) | ||
| maxlen) | ||
| elif cte == 'base64': | ||
| data = _encode_base64(embedded_body(lines), policy.max_line_length) | ||
| data = _encode_base64(embedded_body(lines), maxlen) | ||
| else: | ||
| raise ValueError("Unknown content transfer encoding {}".format(cte)) | ||
| return cte, data | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| :mod:`email`: Ensure policy accepts unlimited line lengths by | ||
| treating falsey values as :data:`sys.maxsize`. |
Uh oh!
There was an error while loading. Please reload this page.