[PATCH 2 of 2] commands.push: use paths API

[Gregory Szorc]

On Mon, Aug 24, 2015 at 10:43 PM, Durham Goode <durham at fb.com> wrote:

>
>
> On 8/24/15 7:30 PM, Gregory Szorc wrote:
>
> On Mon, Aug 24, 2015 at 9:51 PM, Durham Goode <durham at fb.com> wrote:
>
>> On 8/18/15 10:07 PM, Gregory Szorc wrote:
>>
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc at gmail.com>
>>> # Date 1439012387 25200
>>> #      Fri Aug 07 22:39:47 2015 -0700
>>> # Node ID d75d57eea9af5d62a24356f129aa2d774ec32c03
>>> # Parent  1699cd0bac167bd24f710c849d78d6a6c0089ac9
>>> commands.push: use paths API
>>>
>>> ui.path instances now collect most of the data used by commands.push().
>>> Move away from ui.expandpath() and call ui.paths.getpath() to get a
>>> path instance.
>>>
>>> Some "pushing to" output was dropped as one test demonstrates. I believe
>>> the dropped message was redundant with the error message and the change
>>> to be acceptable.
>>>
>>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>>> --- a/mercurial/commands.py
>>> +++ b/mercurial/commands.py
>>> @@ -5273,20 +5273,16 @@ def push(ui, repo, dest=None, **opts):
>>>                   # if we try to push a deleted bookmark, translate it
>>> to null
>>>                   # this lets simultaneous -r, -b options continue
>>> working
>>>                   opts.setdefault('rev', []).append("null")
>>>   -    dest = ui.expandpath(dest or 'default-push', dest or 'default')
>>> -    dest, branches = hg.parseurl(dest, opts.get('branch'))
>>> +    path = ui.paths.getpath(dest or 'default-push', default='default')
>>> +    if not path:
>>> +        raise util.Abort(_('default repository not configured!'),
>>> +                         hint=_('see the "path" section in "hg help
>>> config"'))
>>>
>> This actually breaks hggit.  hggit wraps hg.peer to produce a gitrepo
>> instance when given a file path to a gitrepo.  With this patch, the
>> exception above is thrown because ui.paths.getpath() returns None since it
>> validates that the given path has a .hg directory (which is not the case in
>> hggit).
>>
>> What's the path-y way of fixing this?  Wrapping the logic that does the
>> .hg validation is tough since it exists inside the path class constructor
>> and throws an exception (so the wrapping function doesn't see the returned
>> 'self' when it catches the exceptions).  I could move the path validation
>> logic to another function on ui.path, then hggit could wrap that?
>
>
> First, it is unclear whether this validation should be performed in
> path.__init__ or in paths.getpath(). I think I put it in path.__init__
> because it was slightly easier given the commit arc I took. I wouldn't be
> surprised if this gets changed as I do more paths foo.
>
> I would move the path validation logic (the line startiing with "if (not
> name and not u.scheme" into a module-level function on ui.py and have hggit
> wrap it.
>
> Do I dare look at hggit's source to see how it was bypassing this check
> before?
>
> I don't think this logic existed before.  The old code just went straight
> to calling hg.peer(...) (which hggit handles), and only threw the 'default
> repository not configured' error when hg.peer(...) failed.
>
> Actually, the fact that it now says "default repository not configured"
> when you do 'hg push somebadpath' seems like an incorrect error message.  I
> just tested it and this seems like a regression:
>
> ~/myrepo> hg-3.5 push doesntexist
> pushing to doesntexist
> abort: repository doesntexist not found!
>
> ~/myrepo> hg-latest push doesntexist
> pushing to ssh://myserver.com//data/myrepo
> searching for changes
> ...
>
>
Wow, I'm surprised this error condition isn't tested! An earlier version of
this work had some code to detect this issue. I thought it was too complex
since I got tests to pass without it. Oh well.

I'm away on a business trip and likely won't have time to look at this
until next week. Perhaps this patch should be backed out until I have time
to fix?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150825/749b2aa7/attachment.html>