Patchwork [1,of,9,paths,v2] ui: represents paths as classes

login
register
mail settings
Submitter Gregory Szorc
Date March 1, 2015, 9:50 p.m.
Message ID <c458cf1a414e2d153484.1425246640@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/7862/
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 1423341730 28800
#      Sat Feb 07 12:42:10 2015 -0800
# Node ID c458cf1a414e2d1534841fd55274678e93c8c707
# Parent  7a21944731557cd18dc5c53969318d80ec547e2d
ui: represents paths as classes

Many have long wanted for paths to have expanded functionality and
flexibility.

In order to make that transition possible, we need to start
representing paths as something more than simple strings.

This patch introduces two classes:

1) "path" for representing a single path instance
2) "paths" for representing a collection of "paths"

Since we don't like patches that introduce new code without any
consumers, we convert ui.expandpath() to use the new APIs internally.
Upcoming patches will start exposing "path" instances to consumers
that currently interface with string paths.
Matt Mackall - March 3, 2015, 11:29 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 1423341730 28800
> #      Sat Feb 07 12:42:10 2015 -0800
> # Node ID c458cf1a414e2d1534841fd55274678e93c8c707
> # Parent  7a21944731557cd18dc5c53969318d80ec547e2d
> ui: represents paths as classes

> -import errno, getpass, os, socket, sys, tempfile, traceback
> +import errno, getpass, os, socket, sys, tempfile, traceback, weakref

Ok, I'm already sad. I'd really like to have fewer weakrefs (read: zero)
in the codebase rather than more. This case seems to be because we defer
loading the paths and thus need the paths object to hold a ui so we can
call ui.configitems.

Could we instead make paths a propertycache value that holds a normal
dict? Or at least a comment explaining why we went with a weakref.
Gregory Szorc - March 8, 2015, 6:26 p.m.
On Tue, Mar 3, 2015 at 3:29 PM, 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 1423341730 28800
> > #      Sat Feb 07 12:42:10 2015 -0800
> > # Node ID c458cf1a414e2d1534841fd55274678e93c8c707
> > # Parent  7a21944731557cd18dc5c53969318d80ec547e2d
> > ui: represents paths as classes
>
> > -import errno, getpass, os, socket, sys, tempfile, traceback
> > +import errno, getpass, os, socket, sys, tempfile, traceback, weakref
>
> Ok, I'm already sad. I'd really like to have fewer weakrefs (read: zero)
> in the codebase rather than more. This case seems to be because we defer
> loading the paths and thus need the paths object to hold a ui so we can
> call ui.configitems.
>
> Could we instead make paths a propertycache value that holds a normal
> dict? Or at least a comment explaining why we went with a weakref.
>

My initial thinking here was:

1) There is enough support code around "get a path" that hanging utility
methods off a common container class would be worthwhile.
2) The config can change as the program runs, so always using the current
config state to retrieve path information seemed justified.

I've reconsidered #2. I think a propertycache should be suitable. Worst
case we can invalidate the cache when the config changes (which the ui
instance knows about).

I'm rewriting this part of make "paths" inherit from dict and to use a
propertycache, not weakref.
Gregory Szorc - March 8, 2015, 7:53 p.m.
I'm going to send the first "half" of this series as v3 soon. The latter
patches need a bit more work before landing. The main reason I patchbombed
such a large series initially was to show people where this was going. Now
that I think I've established that direction, I'll try to keep the patch
count down.

On Sun, Mar 1, 2015 at 1:50 PM, Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1423341730 28800
> #      Sat Feb 07 12:42:10 2015 -0800
> # Node ID c458cf1a414e2d1534841fd55274678e93c8c707
> # Parent  7a21944731557cd18dc5c53969318d80ec547e2d
> ui: represents paths as classes
>
> Many have long wanted for paths to have expanded functionality and
> flexibility.
>
> In order to make that transition possible, we need to start
> representing paths as something more than simple strings.
>
> This patch introduces two classes:
>
> 1) "path" for representing a single path instance
> 2) "paths" for representing a collection of "paths"
>
> Since we don't like patches that introduce new code without any
> consumers, we convert ui.expandpath() to use the new APIs internally.
> Upcoming patches will start exposing "path" instances to consumers
> that currently interface with string paths.
>
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -5,9 +5,9 @@
>  # This software may be used and distributed according to the terms of the
>  # GNU General Public License version 2 or any later version.
>
>  from i18n import _
> -import errno, getpass, os, socket, sys, tempfile, traceback
> +import errno, getpass, os, socket, sys, tempfile, traceback, weakref
>  import config, scmutil, util, error, formatter
>  from node import hex
>
>  samplehgrcs = {
> @@ -85,8 +85,9 @@ class ui(object):
>          self._ucfg = config.config() # untrusted
>          self._trustusers = set()
>          self._trustgroups = set()
>          self.callhooks = True
> +        self.paths = paths(self)
>
>          if src:
>              self.fout = src.fout
>              self.ferr = src.ferr
> @@ -530,12 +531,12 @@ class ui(object):
>          """Return repository location relative to cwd or from [paths]"""
>          if util.hasscheme(loc) or os.path.isdir(os.path.join(loc, '.hg')):
>              return loc
>
> -        path = self.config('paths', loc)
> -        if not path and default is not None:
> -            path = self.config('paths', default)
> -        return path or loc
> +        p = self.paths.getpath(loc, default=default)
> +        if p:
> +            return p.loc
> +        return loc
>
>      def pushbuffer(self, error=False):
>          """install a buffer to capture standard output of the ui object
>
> @@ -922,4 +923,57 @@ class ui(object):
>          ui.write(s, 'label') is equivalent to
>          ui.write(ui.label(s, 'label')).
>          '''
>          return msg
> +
> +class paths(object):
> +    """Represents a collection of paths and their configs.
> +
> +    Data is derived from ui instances and the config files they have
> loaded.
> +    """
> +    def __init__(self, ui):
> +        self._uiref = weakref.ref(ui)
> +
> +    @property
> +    def ui(self):
> +        return self._uiref()
> +
> +    def __iter__(self):
> +        """Iterate all available paths, as ``path`` instances."""
> +        for name, loc in self.ui.configitems('paths'):
> +            # No URL is the same as not existing.
> +            if not loc:
> +                continue
> +            yield path(name, loc=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.
> +
> +        Returns None if the specified path or the default path was not
> +        found.
> +        """
> +        try:
> +            return self[name]
> +        except KeyError:
> +            if default is not None:
> +                try:
> +                    return self[default]
> +                except KeyError:
> +                    pass
> +
> +        return None
> +
> +class path(object):
> +    """Represents an individual path and its configuration."""
> +
> +    def __init__(self, name, loc=None):
> +        """Construct a path from its config options."""
> +        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
>

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -5,9 +5,9 @@ 
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
 from i18n import _
-import errno, getpass, os, socket, sys, tempfile, traceback
+import errno, getpass, os, socket, sys, tempfile, traceback, weakref
 import config, scmutil, util, error, formatter
 from node import hex
 
 samplehgrcs = {
@@ -85,8 +85,9 @@  class ui(object):
         self._ucfg = config.config() # untrusted
         self._trustusers = set()
         self._trustgroups = set()
         self.callhooks = True
+        self.paths = paths(self)
 
         if src:
             self.fout = src.fout
             self.ferr = src.ferr
@@ -530,12 +531,12 @@  class ui(object):
         """Return repository location relative to cwd or from [paths]"""
         if util.hasscheme(loc) or os.path.isdir(os.path.join(loc, '.hg')):
             return loc
 
-        path = self.config('paths', loc)
-        if not path and default is not None:
-            path = self.config('paths', default)
-        return path or loc
+        p = self.paths.getpath(loc, default=default)
+        if p:
+            return p.loc
+        return loc
 
     def pushbuffer(self, error=False):
         """install a buffer to capture standard output of the ui object
 
@@ -922,4 +923,57 @@  class ui(object):
         ui.write(s, 'label') is equivalent to
         ui.write(ui.label(s, 'label')).
         '''
         return msg
+
+class paths(object):
+    """Represents a collection of paths and their configs.
+
+    Data is derived from ui instances and the config files they have loaded.
+    """
+    def __init__(self, ui):
+        self._uiref = weakref.ref(ui)
+
+    @property
+    def ui(self):
+        return self._uiref()
+
+    def __iter__(self):
+        """Iterate all available paths, as ``path`` instances."""
+        for name, loc in self.ui.configitems('paths'):
+            # No URL is the same as not existing.
+            if not loc:
+                continue
+            yield path(name, loc=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.
+
+        Returns None if the specified path or the default path was not
+        found.
+        """
+        try:
+            return self[name]
+        except KeyError:
+            if default is not None:
+                try:
+                    return self[default]
+                except KeyError:
+                    pass
+
+        return None
+
+class path(object):
+    """Represents an individual path and its configuration."""
+
+    def __init__(self, name, loc=None):
+        """Construct a path from its config options."""
+        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