Patchwork [4,of,5,paths,v3] ui: change how default is handled by getpath()

login
register
mail settings
Submitter Gregory Szorc
Date March 8, 2015, 7:54 p.m.
Message ID <46c14d03f3e05aa9992d.1425844457@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/7944/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Gregory Szorc - March 8, 2015, 7:54 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1423509114 28800
#      Mon Feb 09 11:11:54 2015 -0800
# Node ID 46c14d03f3e05aa9992d074311c5e92924c2dda3
# Parent  0f9c6588afcbaaca41b8a5ff17d8969e7f6d157a
ui: change how default is handled by getpath()

Callers of ui.expandpath perform a pattern like
|ui.expandpath(dest or 'default-push', dest or 'default')|. The
intent of this pattern is somewhat difficult to read and requires
callers contain logic for path fallback.

Since we now have a proper paths API, we now have the opportunity
to move this logic from the caller into the new API so callers
aren't burdened with it.

This patch changes getpath() to perform default fallback internally,
rather than burden callers.

The new code is a bit more complicated than I would like, especially
the new implementation of ui.expandpath. However, this complication
will eventually absorb complication in callers of ui.expandpath and
will making calling code simpler. This will be addressed in subsequent
patches.
Pierre-Yves David - March 10, 2015, 3:29 a.m.
On 03/08/2015 12:54 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1423509114 28800
> #      Mon Feb 09 11:11:54 2015 -0800
> # Node ID 46c14d03f3e05aa9992d074311c5e92924c2dda3
> # Parent  0f9c6588afcbaaca41b8a5ff17d8969e7f6d157a
> ui: change how default is handled by getpath()
>
> Callers of ui.expandpath perform a pattern like
> |ui.expandpath(dest or 'default-push', dest or 'default')|. The
> intent of this pattern is somewhat difficult to read and requires
> callers contain logic for path fallback.
>
> Since we now have a proper paths API, we now have the opportunity
> to move this logic from the caller into the new API so callers
> aren't burdened with it.
>
> This patch changes getpath() to perform default fallback internally,
> rather than burden callers.
>
> The new code is a bit more complicated than I would like, especially
> the new implementation of ui.expandpath. However, this complication
> will eventually absorb complication in callers of ui.expandpath and
> will making calling code simpler. This will be addressed in subsequent
> patches.

This patch is scary and the implementation looks like a soup of 
if/elif/else statement I cannot get my head around.

Can we build something simpler or document why this has to be some scary ?

> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -531,9 +531,16 @@ 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
>
> -        p = self.paths.getpath(loc, default=default)
> +        if loc == 'default-push':
> +            p = self.paths.getpath(loc, default=True)
> +        elif loc == 'default':
> +            p = self.paths.getpath(loc)
> +        else:
> +            assert default != 'default-push'
> +            p = self.paths.getpath(loc, default=default=='default')
> +

Can we get a single `self.paths.getpath` call. preparing parameter on 
previous line if necessary ? This will make the conditional clearer 
(because shorter) and the code easier to modify (less line to touch if 
getpath changes))

I'm also curious about why this is not a valid version (and probably 
deserve a comment):

   p = self.paths.getpath(loc, default=(default in ('default', 
'default-push)')

>           if p:
>               return p.loc
>           return loc
>
> @@ -949,22 +956,34 @@ class paths(dict):
>                   continue
>
>               self[name] = path(name, rawloc=loc)
>
> -    def getpath(self, name, default=None):
> +    def getpath(self, name, default=False):
>           """Return a ``path`` for the specified name, falling back to a default.
>
> -        Returns the first of ``name`` or ``default`` that is present, or None
> -        if neither is present.
> +        If ``default`` is True, we attempt to resolve the default path.
> +        If ``default`` is the string ``push``, we attempt to resolve the
> +        default push path.

I've not idea why we have to use such logic here. it seems a bit 
witchcrafty to me.

> +
> +        Returns ``None`` if a path could not be found.
>           """
>           try:
>               return self[name]
>           except KeyError:
> -            if default is not None:
> -                try:
> -                    return self[default]
> -                except KeyError:
> -                    pass
> +            pass
> +
> +        if default == 'push':
> +            try:
> +                return self['default-push']
> +            except KeyError:
> +                # Fall through to "default"
> +                pass

so me try except…

use either membership testing:

   if default == 'push' and 'default-push' in self:
      return self['default-push']

or use `get`:

   if default == 'push':
       val = self.get('default-push')
       if val is not None:
           return val

> +
> +        if default:
> +            try:
> +                return self['default']
> +            except KeyError:
> +                pass

same here.

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -531,9 +531,16 @@  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
 
-        p = self.paths.getpath(loc, default=default)
+        if loc == 'default-push':
+            p = self.paths.getpath(loc, default=True)
+        elif loc == 'default':
+            p = self.paths.getpath(loc)
+        else:
+            assert default != 'default-push'
+            p = self.paths.getpath(loc, default=default=='default')
+
         if p:
             return p.loc
         return loc
 
@@ -949,22 +956,34 @@  class paths(dict):
                 continue
 
             self[name] = path(name, rawloc=loc)
 
-    def getpath(self, name, default=None):
+    def getpath(self, name, default=False):
         """Return a ``path`` for the specified name, falling back to a default.
 
-        Returns the first of ``name`` or ``default`` that is present, or None
-        if neither is present.
+        If ``default`` is True, we attempt to resolve the default path.
+        If ``default`` is the string ``push``, we attempt to resolve the
+        default push path.
+
+        Returns ``None`` if a path could not be found.
         """
         try:
             return self[name]
         except KeyError:
-            if default is not None:
-                try:
-                    return self[default]
-                except KeyError:
-                    pass
+            pass
+
+        if default == 'push':
+            try:
+                return self['default-push']
+            except KeyError:
+                # Fall through to "default"
+                pass
+
+        if default:
+            try:
+                return self['default']
+            except KeyError:
+                pass
 
         return None
 
 class path(object):