Bug in description handling

[Dan Villiom Podlaski Christiansen]

On 14 Aug 2010, at 09:01, Martijn Pieters wrote:

> On 13 Aug 2010, at 23:13, Matt Mackall wrote:
>> Alright, now we've got three patches for this and we're going to  
>> need to
>> pick one. This patch looks ok except for monkeypatching locale - we
>> should just directly find an encoding and skip the def/monkeypatch
>> steps.
>
> Updated:
>
>   http://bitbucket.org/mjpieters/mercurial-crew-mq/src/b7b3064646ec/pending/getpreferredencoding.patch
>
> I inlined the POSIX version of getpreferredencoding into a private  
> function.

I have two gripes with this patch:

To begin with, checking if sys.platform is 'mac' is pointless. Only  
Mac OS 9 has this in sys.platform, and I believe support for it was  
dropped in Python 2.4. This is the earliest version that Mercurial  
works with, and even *if* you had a compatible Python binary running  
there, it wouldn't work, as osutil hasn't been implemented for that  
system.

In addition, the version check doesn't deal with patched Python  
binaries that don't need the override. I have 7 Python binaries on my  
Mac:

Python 2.4.6, 2.5.5, 2.6.5, 2.7 & 3.1.2 from MacPorts.
Python 2.5.4 & 2.6.1 from Apple, included with Mac OS X 10.6.

Of these, only the 2.4.6 & 2.5.5 binaries from MacPorts yields ‘mac- 
roman’ for locale.getpreferredencoding(). The rest correctly yield  
‘UTF-8’. Thus, the assumption that any Python earlier than 2.7 is  
broken isn't safe.

I posted[1] a patch to this list yesterday, based on your earlier  
patch. (Perhaps I should have Cc'ed you; I didn't think of that…)  
Instead of always overriding getpreferredencoding(), only a value of  
‘mac-roman’ causes the fixed method to be invoked. After all, it's  
rather unlikely that users actually want this encoding, and even in  
this unlikely scenario, your method should be safe, right? I'm not  
sure any of this actually affects users, though…

[1] <http://selenic.com/pipermail/mercurial-devel/2010-August/023604.html 
 >

--

Dan Villiom Podlaski Christiansen
danchr at gmail.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 1943 bytes
Desc: not available
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20100814/b9c9bf70/attachment.bin>