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

Note: this is intended to fix https://pagure.io/pagure/issue/1814 .

This one is actually kinda subtle. The way all the functions in this module are 'really' supposed to work is to pass the mail body to send_email as a unicode instance. send_email then calls msg = MIMEText(text.encode('utf-8'), 'plain', 'utf-8') (where text is the body).

All the other functions do something like this:

body = u'''sometext %s somemoretext %s''' % (foo, bar)
send_email(body...)

But this function does something different. Before going into that, let's note an interesting property of the way all the other functions do it. It works fine so long as foo and bar are instances of unicode or instances of str containing only ASCII characters:

>>> u'foo %s' % unichr(40960)
u'foo \ua000'
>>> u'foo %s' % 'bar'
u'foo bar'
>>>

However, it fails if foo or bar is a str that contains something other than ASCII, like this:

>>> u'foo %s' % unichr(40960).encode('utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xea in position 0: ordinal not in range(128)

Now, I think that's actually why this one function was written differently from all the others. There's a bit early in this function (notify_new_commits) where we get the commit ID, author and subject of each commit, and use them to create a string:

for commit in commits:
    commits_info.append({
        'commit': commit,
        'author': pagure.lib.git.get_author(commit, abspath),
        'subject': pagure.lib.git.get_commit_subject(commit, abspath)
    })
commits_string = '\n'.join('{0}    {1}    {2}'.format(
    commit_info['commit'], commit_info['author'], commit_info['subject'])

We've gotta pay close attention to what's going on here. Specifically, we get author and subject from pagure.lib.git, which - I checked - ultimately gets them from subprocess.Popen().communicate() via a helper function called read_output. I tested that, and it returns (I'm pretty sure) a UTF-8 encoded str - if you create a directory with a file named in Japanese and write a little test script that uses Pagure's read_output to run ls on that directory, you get an object of type str which includes the Japanese filename.

Then we take that UTF-8 encoded str and stuff it into that '{0} {1} {2}'.format(... bit, so in the end we wind up with a UTF-8 encoded str called commits_string. If neither the author nor the subject happens to have any non-ASCII characters in it, this will work whatever messy things you try to do with it, but if it does have non-ASCII characters in it, things are harder.

So I suspect the author of this function initially tried to follow the pattern of all the other functions, and tried to do something like this:

    email_body = u'''
The following commits were pushed to the repo "%s" on branch
"%s", which you are following:
%s
To view more about the commits, visit:
%s
''' % ( project.fullname, branch, commits_string, commit_url)

but then somehow realized (or found) that it would crash on non-ASCII text, as we noticed earlier. So he changed it to use '''blahblah {foo}'''.format(foo=foo), instead - which works there (so long as none of the other values used is a unicode instance containing non-ASCII characters, of course...are repo or branch names allowed to have non-ASCII characters?), but means the body text we wind up passing to send_email is not a unicode instance (as it expects), but a UTF-8 encoded str instance.

So long as it doesn't actually contain any non-ASCII characters, this happens to get through OK. But if it does, then we fail when send_email does msg = MIMEText(text.encode('utf-8'), 'plain', 'utf-8'), because ultimately what we're doing is taking an encoded string and trying to encode it...which doesn't work, as we can easily demonstrate:

>>> unichr(40960).encode('utf-8').encode('utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xea in position 0: ordinal not in range(128)
>>>

So...now I understand exactly what's going on here, and am satisfied. :) This PR should indeed fix it (as it decodes the body text to a unicode instance before calling send_email, which is what send_email expects), but the commit topic is kinda wrong, and I think we could fix it in a way that's a bit more consistent with the rest of the file. I also think we should explain all of this a bit in the interests of avoiding problems in future. So I'm gonna send an alternative PR.

So I'm gonna send an alternative PR.

@adamwill please use @puiterwijk 's commit and add a line Merges #3140 in either commit message (either edit Patrick's or in yours) so when I merge your PR it closes this one as well :)

I don't see how I can 'use' Patrick's commit - I want to do something slightly different than it?

https://pagure.io/pagure/pull-request/3141 is my alternative, which is a bit more in-line with the rest of the file, and also adds a test.

Thanks, @adamwill. I'll close this one.