2245fcd0e160 in stable broke subrepos for python 2.4/2.5

[Mads Kiilerich]

Matt Mackall wrote, On 12/11/2010 02:07 AM:
> On Fri, 2010-12-10 at 12:10 +0100, Thomas Arendsen Hein wrote:
>> Hi Mads!
>>
>> changeset:   13105:2245fcd0e160
>> branch:      stable
>> parent:      13103:6e79a3bb8c79
>> user:        Mads Kiilerich<mads at kiilerich.com>
>> date:        Fri Dec 10 01:30:16 2010 +0100
>> files:       mercurial/subrepo.py tests/test-subrepo-relative-path.t
>> description:
>> subrepo: initialize subrepo relative default paths relative to their root
>> ...
>> +                    if not os.path.isabs(value):
>> +                        value = os.path.relpath(os.path.abspath(value), root)
>>
>> os.path.relpath was introduced in python 2.6
>>
>> Can you fix this and add a test for using os.path.relpath in check-code?
> Worse than that, the code is actually doing the wrong thing. With this
> patch:

Oh. And oh again. And I'm sorry for not being able to fix it immediately.

> diff -r 22eb8aea82d4 mercurial/subrepo.py
> --- a/mercurial/subrepo.py	Fri Dec 10 16:15:59 2010 -0600
> +++ b/mercurial/subrepo.py	Fri Dec 10 18:49:50 2010 -0600
> @@ -334,7 +334,9 @@
>               def addpathconfig(key, value):
>                   if value:
>                       if not os.path.isabs(value):
> +                        value2 = util.canonpath(root, root, value)
>                           value = os.path.relpath(os.path.abspath(value), root)
> +                        print "ne", value, value2
>                       fp.write('%s = %s\n' % (key, value))
>                       self._repo.ui.setconfig('paths', key, value)
>
> I get:
>
> --- /home/mpm/hg/stable/tests/test-subrepo-relative-path.t
> +++ /home/mpm/hg/stable/tests/test-subrepo-relative-path.t.err
> @@ -44,6 +44,7 @@
>     adding file changes
>     added 1 changesets with 3 changes to 3 files
>     updating to branch default
> +  ne ../../http:/localhost:$HGPORT/sub http:/localhost:$HGPORT/sub

I did wonder if canonpath or pathto could be used instead of relpath, 
but neither testing nor reading indicated that they were intended to 
solve this problem. In this case we don't care about the canonical path, 
so pathto looks more appropriate? Dare we "fix" it to accept absolute n2 
and ignore root - and perhaps handle different path separators, or 
should we introduce another function?

> Looks like os.path.abspath kills our '//' and canonpath is doing the same.
>
>
> It's pretty clear at this point that this path code is too fragile to
> test in our usual holistic way.

Ramblings ahead:

Yes, and I think the code is fragile because we must handle url paths 
explicitly here and in other places. A "Path with scheme" can be 
absolute even though os.path.isabs tells the opposite, and I guess 
scheme paths can be relative too - depending on what our scope is and 
how much url-like syntax we want to support.

We could add special handling and testing of schemes here. That comes 
pretty close to what Brodie has been working on with his "urlsplit" 
patches on http://bitbucket.org/brodie/mercurial-crew-mq/.hg/patches . 
There are good reasons to keep core path handling simple simple and 
represent paths as strings and have helper functions, rather than 
creating smart objects. But perhaps it would make sense to use url 
objects all the places where we can handle urls, and then use methods 
for isabs, join, normpath, relpath and so on.

It seems like some errors could be avoided if we had a more clear 
distinction between the different kinds of paths:

* File system paths, with native path separator and casing semantics 
(ideally per file system, not per os), possible also drive letters, 
handled by os.path - this is what is used at the low level file system 
API calls.

* Mercurial paths, that is what is used internally, mostly like 
posixpath, always "/" separator and case sensitive, maps to file system 
paths.

* UI paths, can be fs paths or internal paths or URLs, might accept 
native path separator and sloppy casing, neither fully ntpath nor 
posixpath, needs special isabs and normpath etc.

(It seems like re: and glob: (and fileset specifications?) emits what I 
call UI paths, but it makes it a bit tricky that they sometimes 
represents fs paths and sometimes Mercurial paths. That has caused some 
confusion and bugs over time.)

/Mads