gh-144156: Fix email header folding concatenating encoded words#144692
gh-144156: Fix email header folding concatenating encoded words#144692robsdedude wants to merge 12 commits intopython:mainfrom
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
robsdedude
changed the title
|
FYI I've been looking at this and will have a review soonish. You are correct that the gh-92281 fix was buggy. |
bitdancer
left a comment
bitdancer
left a comment
There was a problem hiding this comment.
Thanks for fixing this!
I edited this review a bunch, so mistake could have crept in to my code suggestions, though I did test them. The randomizer test should catch any transcription errors if nothing else does.
| text_space = remaining_space - chrome_len | ||
| if text_space <= 0: | ||
| lines.append(' ') | ||
| continue |
There was a problem hiding this comment.
One of my new tests revealed a pre-existing problem here:
| continue | |
| if text_space <= 0: | |
| newline = _steal_trailing_WSP_if_exists(lines) | |
| lines.append(newline or ' ') | |
| new_last_ew = len(lines[-1]) | |
| continue |
There was a problem hiding this comment.
This suggestion does not seem to fix the test cases test_ew_folding_round_trip_1 and test_ew_folding_round_trip_2 you posted in a comment below 🤔
There was a problem hiding this comment.
Hmm. Maybe there's a bug in your steal all whitespace function? Using my suggestion the tests pass for me, but I see you didn't make that change, at least in what I can see. On the other hand, github also doesn't show the fix applied, so I'm not sure where you are at, exactly.
However, I also see that I failed to run the full test suite before posting the review, and there's a whitespace change in three existing tests. I think it's a bugfix, but I'll have to look closer when I get a chance later today.
There was a problem hiding this comment.
Yeah, I only had the change applied locally. I think I found the issue. The suggestion here looks like the lines.append(' ') right after if text_space <= 0: should remain there. I suppose it shouldn't. Makes more sense that way when reading the code, too 🙃
With the suggestion applied correctly to my local working tree I now see the same: your new tests pass, 3 old tests started failing.
I went ahead and adjusted the test expectations. Let me know what you think once you've had a chance to have a closer look.
There was a problem hiding this comment.
Yeah that suggestion looks borked. I don't see any way to dismiss it, unfortunately.
The test expectation changes are correct, that extra space after Subject was a bug. I may even have opened an issue about it, but if so I can't find it. Probably just made a "note to self" that I've since lost.
| @@ -0,0 +1,3 @@ | |||
| Fix folding of email headers violating `RFC 2047`_ with two consecutive encoded words without separating linear-white-space. | |||
|
|
|||
| .. _RFC 2047: https://www.rfc-editor.org/rfc/rfc2047 | |||
There was a problem hiding this comment.
| .. _RFC 2047: https://www.rfc-editor.org/rfc/rfc2047 | |
| Fix the folding of headers by the :mod:`email` library when :rfc:`2047` encoded words are used. Now whitespace is correctly preserved and also correctly added between adjacent encoded words. The latter property was broken by the fix for gh-92081, which mostly fixed previous failures to preserve whitespace. |
Out of curiosity I checked, and according to the changelogs the buggy fix was released in 3.14 alpha 1, 3.13 beta 2 and 3.12.5. Unfortunately the latter is now final. Maybe we should include that in the news item? I'm not sure what the policy is on that.
There was a problem hiding this comment.
Maybe we should include that in the news item?
I'm all for documenting known issues and shortcomings. However, I doubt people would find this info in the release notes unless they're doing "software archeology", i.e., they already know (roughly) what info they're looking for.
I'm not sure what the policy is on that.
Neither am I. I'm pretty much a first-time contributor 😅
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Co-authored-by: R. David Murray <rdmurray@bitdance.com>
Misc/NEWS.d/next/Core_and_Builtins/2026-02-10-22-05-51.gh-issue-144156.UbrC7F.rst
Outdated
Show resolved
Hide resolved
Make use of the fact that folder must never produce a line of only WSP.
| return wsp | ||
|
|
||
|
|
||
| def _last_word_is_sill_ew(_last_word_is_ew, added_str): |
There was a problem hiding this comment.
I find this name a bit confusing. What it actually does is "return true if last_word_is_ew is true and added_str consists of whitespace only". I can't think of a name that is shorter and as descriptive as the actual code. So I'm not in favor of this function.
| text_space = remaining_space - chrome_len | ||
| if text_space <= 0: | ||
| lines.append(' ') | ||
| continue |
There was a problem hiding this comment.
Yeah that suggestion looks borked. I don't see any way to dismiss it, unfortunately.
The test expectation changes are correct, that extra space after Subject was a bug. I may even have opened an issue about it, but if so I can't find it. Probably just made a "note to self" that I've since lost.
3 participants
Uh oh!
There was an error while loading. Please reload this page.