2245fcd0e160 in stable broke subrepos for python 2.4/2.5

Matt Mackall mpm at selenic.com
Fri Dec 10 19:07:19 CST 2010


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:

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


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. But since the core of it is actually
just simple string manipulation and doesn't require actually moving
around repositories over http, we can break out the core logic into its
own function and doctest it (see tests-doctest.t). Something like:

_defaultpath(root, path):
    '''Canonicalize a path relative to root so we can store 
       it in .hg/hgrc

       >>> _defaultpath("/foo", "/foo/bar")
       "bar"
       >>> _defaultpath("/foo", "../bar")
       "../bar"
       >>> _defaultpath("/foo", "/bar") # absolute
       "/bar"
       >>> _defaultpath("/foo", "http://blah") # URL
       "http://blah"
     '''

It'd be great to do something similar for _abssource, which has also
been problematic.

Going to back out this change for now.

-- 
Mathematics is the supreme nostalgia of our time.




More information about the Mercurial-devel mailing list