Patchwork [5,of,9,paths,v2] ui: move URL and path detection into getpath()

login
register
mail settings
Submitter Gregory Szorc
Date March 1, 2015, 9:50 p.m.
Message ID <0aaf05f23aae7fdc4404.1425246644@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/7866/
State Changes Requested
Headers show

Comments

Gregory Szorc - March 1, 2015, 9:50 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1423427350 28800
#      Sun Feb 08 12:29:10 2015 -0800
# Node ID 0aaf05f23aae7fdc440412af2f9815daaa9c4123
# Parent  4d9ba0789bf42ad77c407c84b19a6486652730ed
ui: move URL and path detection into getpath()

ui.expandpath() has code for recognizing URLs or local filesystem
paths. Our goal is to use path instances in more locations. Since many
call sites pass in arguments that could be URLs or filesystem paths,
we move this code into paths.getpath() so that more callers can be
switched to using the new API directly instead of ui.expandpath().

The added check for whether ``name`` is defined is added because
future callers may pass in empty values (expandpath doesn't
appear to receive empty values today).

Upcoming patches will perform additional processing on the "local"
argument to path.__init__. For now, we don't distinguish between the
origin and shoehorn everything into "loc."
Matt Mackall - March 4, 2015, 7:47 p.m.
On Sun, 2015-03-01 at 13:50 -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1423427350 28800
> #      Sun Feb 08 12:29:10 2015 -0800
> # Node ID 0aaf05f23aae7fdc440412af2f9815daaa9c4123
> # Parent  4d9ba0789bf42ad77c407c84b19a6486652730ed
> ui: move URL and path detection into getpath()
> 
> ui.expandpath() has code for recognizing URLs or local filesystem
> paths. Our goal is to use path instances in more locations. Since many
> call sites pass in arguments that could be URLs or filesystem paths,
> we move this code into paths.getpath() so that more callers can be
> switched to using the new API directly instead of ui.expandpath().

It's weird to me that the logic for this is not in the path object
contructor? The paths class has no more information and it seems
unlikely we'll want to manually construct paths that subvert the logic?

It's also a bit weird that this is implemented as member variables
rather than trivial methods. It seems like we'd want to have things like
path.url() and path.safeurl() (wrapping util.hidepassword).
Gregory Szorc - March 5, 2015, 7:25 p.m.
On Wed, Mar 4, 2015 at 11:47 AM, Matt Mackall <mpm@selenic.com> wrote:

> On Sun, 2015-03-01 at 13:50 -0800, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1423427350 28800
> > #      Sun Feb 08 12:29:10 2015 -0800
> > # Node ID 0aaf05f23aae7fdc440412af2f9815daaa9c4123
> > # Parent  4d9ba0789bf42ad77c407c84b19a6486652730ed
> > ui: move URL and path detection into getpath()
> >
> > ui.expandpath() has code for recognizing URLs or local filesystem
> > paths. Our goal is to use path instances in more locations. Since many
> > call sites pass in arguments that could be URLs or filesystem paths,
> > we move this code into paths.getpath() so that more callers can be
> > switched to using the new API directly instead of ui.expandpath().
>
> It's weird to me that the logic for this is not in the path object
> contructor? The paths class has no more information and it seems
> unlikely we'll want to manually construct paths that subvert the logic?
>

I don't disagree with your opinions. I intentionally attempted to minimize
the amount of extra refactoring that went into this series. My thinking was
that it would maximize the chances for acceptance. I figured making things
sane could be deferred to once the world is using the new API.


> It's also a bit weird that this is implemented as member variables
> rather than trivial methods. It seems like we'd want to have things like
> path.url() and path.safeurl() (wrapping util.hidepassword).
>

I probably didn't make this clear enough, but my intent was for path
instances to be immutable. I figured having an internal representation of
some of the most widely used variables (namely the raw "loc" and the parsed
url) made sense. I suppose some of the lesser-used properties could be
defined as properties or trivial methods instead of as pre-generated
attributes.

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -528,11 +528,8 @@  class ui(object):
         return user
 
     def expandpath(self, loc, default=None):
         """Return repository location relative to cwd or from [paths]"""
-        if util.hasscheme(loc) or os.path.isdir(os.path.join(loc, '.hg')):
-            return loc
-
         if loc == 'default-push':
             p = self.paths.getpath(loc, default=True)
         elif loc == 'default':
             p = self.paths.getpath(loc)
@@ -955,26 +952,37 @@  class paths(object):
             # per-path attributes.
             if '.' in name:
                 continue
 
-            yield path(name, loc=loc)
+            yield path(name, url=loc)
 
     def __getitem__(self, key):
         for path in self:
             if path.name == key:
                 return path
         raise KeyError('path not known: %s' % key)
 
     def getpath(self, name, default=None):
-        """Return a ``path`` for the specified name.
+        """Return a ``path`` from a name or location.
+
+        Arguments can be named paths (from the config) or locations. Locations
+        are URIs or filesystem paths that are directories having ``.hg``
+        directories.
 
         If ``default`` is True, we attempt to resolve the default path
         if ``name`` could not be resolved. If ``default`` is the string
         ``push``, we attempt to resolve the default push path.
 
         Returns None if the specified path or the default path was not
         found.
         """
+        if name:
+            if util.hasscheme(name):
+                return path(None, url=name)
+
+            if os.path.isdir(os.path.join(name, '.hg')):
+                return path(name, local=name)
+
         try:
             return self[name]
         except KeyError:
             pass
@@ -996,10 +1004,14 @@  class paths(object):
 
 class path(object):
     """Represents an individual path and its configuration."""
 
-    def __init__(self, name, loc=None):
-        """Construct a path from its config options."""
+    def __init__(self, name, url=None, local=None):
+        """Construct a path from its config options.
+
+        The primary URL for the path is defined as either a URL via ``url``
+        (preferred) or from a local, relative filesystem path (``local``).
+        """
         self.name = name
-        # We will eventually parse loc into URLs. For now, we carry the
-        # existing API forward to make the transition simpler.
-        self.loc = loc
+        # We will eventually do intelligent things depending on the
+        # source type. Until then, store a single variable for simplicity.
+        self.loc = url or local