| 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
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).
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