Patchwork extensions: don't fail to load on Windows if path ends with a '\' (issue4187)

login
register
mail settings
Submitter Ed Morley
Date March 4, 2014, 11:17 p.m.
Message ID <74e8f96578fdad93d682.1393975021@ed-laptop>
Download mbox | patch
Permalink /patch/3840/
State Superseded
Commit c04459f959ffa306e518cf41daf5872b78d2ae23
Headers show

Comments

Ed Morley - March 4, 2014, 11:17 p.m.
# HG changeset patch
# User Ed Morley <emorley@mozilla.com>
# Date 1393974750 0
#      Tue Mar 04 23:12:30 2014 +0000
# Node ID 74e8f96578fdad93d682f676441d1b04f1c34901
# Parent  5beb49fd5958cd46cb933a8bccf482b56eb55b18
extensions: don't fail to load on Windows if path ends with a '\' (issue4187)

Same fix as 5c794e7331e7 but now works on Windows too.
Augie Fackler - March 4, 2014, 11:22 p.m.
On Mar 4, 2014, at 6:17 PM, Ed Morley <emorley@mozilla.com> wrote:

> # HG changeset patch
> # User Ed Morley <emorley@mozilla.com>
> # Date 1393974750 0
> #      Tue Mar 04 23:12:30 2014 +0000
> # Node ID 74e8f96578fdad93d682f676441d1b04f1c34901
> # Parent  5beb49fd5958cd46cb933a8bccf482b56eb55b18
> extensions: don't fail to load on Windows if path ends with a '\' (issue4187)

Looks trivially correct, queued despite my lack of a windows test machine.

> 
> Same fix as 5c794e7331e7 but now works on Windows too.
> 
> diff -r 5beb49fd5958 -r 74e8f96578fd mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -46,7 +46,7 @@ def loadpath(path, module_name):
>     path = util.expandpath(path)
>     if os.path.isdir(path):
>         # module/__init__.py style
> -        d, f = os.path.split(path.rstrip('/'))
> +        d, f = os.path.split(path.rstrip(os.sep))
>         fd, fpath, desc = imp.find_module(f, [d])
>         return imp.load_module(module_name, fd, fpath, desc)
>     else:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Siddharth Agarwal - March 5, 2014, 2:27 a.m.
On 03/04/2014 03:17 PM, Ed Morley wrote:
> # HG changeset patch
> # User Ed Morley <emorley@mozilla.com>
> # Date 1393974750 0
> #      Tue Mar 04 23:12:30 2014 +0000
> # Node ID 74e8f96578fdad93d682f676441d1b04f1c34901
> # Parent  5beb49fd5958cd46cb933a8bccf482b56eb55b18
> extensions: don't fail to load on Windows if path ends with a '\' (issue4187)
>
> Same fix as 5c794e7331e7 but now works on Windows too.
>
> diff -r 5beb49fd5958 -r 74e8f96578fd mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -46,7 +46,7 @@ def loadpath(path, module_name):
>       path = util.expandpath(path)
>       if os.path.isdir(path):
>           # module/__init__.py style
> -        d, f = os.path.split(path.rstrip('/'))
> +        d, f = os.path.split(path.rstrip(os.sep))

Does this work if you use / at the end on Windows? That should still 
continue to work.
Olle Lundberg - March 5, 2014, 8:58 a.m.
On Wed, Mar 5, 2014 at 3:27 AM, Siddharth Agarwal <sid0@fb.com> wrote:

> On 03/04/2014 03:17 PM, Ed Morley wrote:
>
>> # HG changeset patch
>> # User Ed Morley <emorley@mozilla.com>
>> # Date 1393974750 0
>> #      Tue Mar 04 23:12:30 2014 +0000
>> # Node ID 74e8f96578fdad93d682f676441d1b04f1c34901
>> # Parent  5beb49fd5958cd46cb933a8bccf482b56eb55b18
>> extensions: don't fail to load on Windows if path ends with a '\'
>> (issue4187)
>>
>> Same fix as 5c794e7331e7 but now works on Windows too.
>>
>> diff -r 5beb49fd5958 -r 74e8f96578fd mercurial/extensions.py
>> --- a/mercurial/extensions.py
>> +++ b/mercurial/extensions.py
>> @@ -46,7 +46,7 @@ def loadpath(path, module_name):
>>       path = util.expandpath(path)
>>       if os.path.isdir(path):
>>           # module/__init__.py style
>> -        d, f = os.path.split(path.rstrip('/'))
>> +        d, f = os.path.split(path.rstrip(os.sep))
>>
>
> Does this work if you use / at the end on Windows? That should still
> continue to work.


Nope, the patch should probably be something along the lines of:

path = util.normpath(util.expandpath(path))
[...]
d, f = os.path.split(path.rstrip('/'))


>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Ed Morley - March 5, 2014, 9:15 a.m.
On 05 March 2014 08:58:56, Olle wrote:
> Nope, the patch should probably be something along the lines of:
>
> path = util.normpath(util.expandpath(path))
> [...]
> d, f = os.path.split(path.rstrip('/')__)

Ah it hadn't occurred to me that using forward slashes on Windows would 
ever work - but checking shows that .split works on any separator, not 
just os.sep - so agree we should use normpath() then.

That makes the rstrip() redundant, since normpath() drops trailing 
slashes in addition to converting forward to backward slashes.

Not sure what the protocol is for updating patches, so will follow up 
in a separate email with it. (Used to the Bugzilla attachment with 
splinter review and tools for exporting to/from hg/bugzilla workflow).

Ed
Mads Kiilerich - March 5, 2014, 3:10 p.m.
On 03/05/2014 10:15 AM, Ed Morley wrote:
> On 05 March 2014 08:58:56, Olle wrote:
>> Nope, the patch should probably be something along the lines of:
>>
>> path = util.normpath(util.expandpath(path))
>> [...]
>> d, f = os.path.split(path.rstrip('/')__)
>
> Ah it hadn't occurred to me that using forward slashes on Windows 
> would ever work - but checking shows that .split works on any 
> separator, not just os.sep

More precisely: Python path handling libraries are quite complex and do 
also consider os.altsep - http://docs.python.org/2/library/os#os.altsep

/Mads

Patch

diff -r 5beb49fd5958 -r 74e8f96578fd mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -46,7 +46,7 @@  def loadpath(path, module_name):
     path = util.expandpath(path)
     if os.path.isdir(path):
         # module/__init__.py style
-        d, f = os.path.split(path.rstrip('/'))
+        d, f = os.path.split(path.rstrip(os.sep))
         fd, fpath, desc = imp.find_module(f, [d])
         return imp.load_module(module_name, fd, fpath, desc)
     else: