pagure

#3141 Fix new commit notification mails with non-ASCII (#1814)
Merged by pingou. Opened by adamwill.
adamwill/pagure notify-unicode  into  master

Download 3141.patch

This is an alternative to #3140. As @puiterwijk figured out,
when a new commit notification mail would contain non-ASCII
text - i.e. when the commit author's full name or the commit
topic contain non-ASCII text - sending the mail would fail. In
3.x at least, this resulted in the service that sends the mails
silently getting stuck; we don't know how the service behaves
when this happens in current git (it was changed from trollius
to celery), but the underlying bug is still there.

It fails because of unicode conversion issues in the new commit
notification function. send_email expects the body text to be
provided as a unicode instance, but notify_new_commits works
with str instances throughout (including the commit author and
commit topic values; these ultimately come in as UTF-8 encoded
strings, from the pagure.git.read_output function). Ultimately
it produces a UTF-8 encoded str which send_email then tries
to encode again, and that crashes.

This approach fixes the problem in a way that's as consistent
as possible with the other functions in the file - the body text
is produced by %s-style string formatting of a unicode, and we
ensure the things that get put into it are also unicode type,
for the ones which can contain non-ASCII text. I also added some
comments about this area, noted the expected type for text in
the send_email docstring, and added a test.

Fixes https://pagure.io/pagure/issue/1814

Signed-off-by: Adam Williamson awilliam@redhat.com

One thing that annoys OCD me here is the test doesn't actually fail in the same way as it fails 'in production', if you take the test and run it on current git master. In production, git master crashes in msg = MIMEText(text.encode('utf-8'), 'plain', 'utf-8'), but if you run the test case on current git master, it crashes when constructing commits_string. I think this means the mocked return values for get_author and get_commit_subject are not quite matching the form those functions actually return, but I can't quite figure out why or how to change it so they do match. The test should still be sufficient, it just annoys me.

Obligatory Python Unicode Question: did you check this is both py2-safe and py3-safe?

=)

No, since you told me that Pagure in general isn't Py3-safe. I suspect the interesting question for Py3 purposes would be what type the output of pagure.lib.git.read_output would be, run under Python 3.

...and the answer to that appears to be, it doesn't work in Python 3. :) Apart from the Python 2-style prints, which are trivial to patch, it fails at out = out.rstrip('\n\r') with TypeError: a bytes-like object is required, not 'str', indicating that in Python 3, the output we get from subprocess - that's out - is a bytes (bytestring), not a str (Python 3, unicode-y string).

Now I write that, I'm fairly sure I've dealt with that same thing before, probably in fedfind or something.

The patch as submitted at this moment works in prod.

Note that there is a PR open to make pagure py3 friendly, so it would be nice to have the new PR not knowingly make this harder :)

This whole area is going to be a complete minefield for Py3 conversion, I suspect. I don't think the PR makes a significant difference in either direction. If anything it would make it very slightly easier, as this PR is at least explicit about what's going on, and includes helpful comments!

edit: if you point me to that PR, I can see if its author addressed the failure of read_output yet and if so how, and see if this PR is compatible with their choice.
Edited by adamwill

Alright, let's rebase and get this in :)

rebased onto a8fef88e754439c4b900167f8bc8296deeeb551e

Pull-Request has been merged by pingou
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.