[merge] win32 cleanups

[John Arbash Meinel]

Alexander Belchenko wrote:
> John Arbash Meinel пишет:
>> Anyway, I wanted to let people know these fixes were out there, and
>> maybe get a review/+1. Even though not all tests are passing, more tests
>> pass now, and it shouldn't break Linux stuff.
> 
> I try to review your patch.
> 


>> === modified file 'bzrlib/msgeditor.py'
>> --- bzrlib/msgeditor.py    2006-06-06 12:06:20 +0000
>> +++ bzrlib/msgeditor.py    2006-06-11 03:22:39 +0000
>> @@ -26,6 +26,7 @@
>>  import bzrlib
>>  import bzrlib.config as config
>>  from bzrlib.errors import BzrError
>> +from bzrlib.trace import mutter
>>  
>>  
>>  def _get_editor():
>> @@ -56,6 +57,7 @@
>>      for e in _get_editor():
>>          edargs = e.split(' ')
>>          try:
>> +            ## mutter("trying editor: %r", (edargs +[filename]))
>>              x = call(edargs + [filename])
>>          except OSError, e:
>>             # We're searching for an editor, so catch safe errors and
>> continue
>>
> 
> Probably this part you were used to track down problem with spawning
> 'del' command, so actually it don't needed anymore. You can remove this
> stuff.
> 

I could get rid of it. When the group was together, we discussed
'mutter', and felt that leaving it there, but commented out was a
reasonable tradeoff. You needed it to do debugging, so if someone else
was doing more debugging in that area, they could just uncomment the
mutter() statements. But we don't want them writing to ~/.bzr.log unless
it would actually be useful for debugging a user's problem.

> 
> [Then skip to cite big part of patch because it seems correct]
> 
> 
> @@ -320,11 +327,12 @@
>>  
>>      if sys.platform == 'win32' and url.startswith('file:///'):
>>          # Strip off the drive letter
>> -        if path[2:3] not in '\\/':
>> +        # path is currently /C:/foo
>> +        if path[2:3] not in ':|' or path[3:4] not in '\\/':
>>              raise errors.InvalidURL(url,                  'win32
>> file:/// paths need a drive letter')
>> -        url_base += path[1:4] # file:///C|/
>> -        path = path[3:]
>> +        url_base += path[0:3] # file:// + /c:
>> +        path = path[3:] # /foo
>>  
> 
> You are using path[2:3] in situation when path[2] is enough. Your
> variant seems to me slightly unclear, because you actually need only one
> character but not slice. I can understand your point, it's just my -0.02.
> 
> -- 
> Alexander

I'm using a slice because that is guaranteed to return. For example:
path = ''
path[2:3] == ''
path[2] => IndexError

Basically, I'm just avoiding getting an index error. And since the empty
string is not in ':|' or '\\/' if the path was not long enough, I would
still raise InvalidURL.
I realize I could do a
if len(path) < 4 or path[2] not in ':|' or path[3] not in '\\/':

I just felt this was easier.
John
=:->



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060611/d0840321/attachment.pgp