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

[Pierre-Yves David]

On 03/08/2015 12:54 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc at 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.

-- 
Pierre-Yves David