[PATCH V2] subrepo: append subrepo path to subrepo push error messages

[Mads Kiilerich]

Angel Ezquerra wrote, On 12/16/2012 11:43 PM:
> On Sun, Dec 16, 2012 at 2:57 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> Angel Ezquerra wrote, On 12/16/2012 01:29 AM:
>>
>>> # HG changeset patch
>>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>>> # Date 1355438273 -3600
>>> # Node ID cec32425a026c4aeb9a6495086b1a1fbe916be24
>>> # Parent  34a1a639d8358e43f4bcba7b0cff19f4e4e6958d
>>> subrepo: append subrepo path to subrepo push error messages
>>>
>>> This change appends the subrepo path to subrepo push errors. That is, when
>>> there
>>> is an error pushing a subrepo, rather than displaying:
>>>
>>> pushing subrepo MYSUBREPO to PATH
>>> searching for changes
>>> abort: push creates new remote head HEADHASH!
>>> hint: did you forget to merge? use push -f to force
>>>
>>> mercurial will show:
>>>
>>> pushing subrepo MYSUBREPO to PATH
>>> searching for changes
>>> abort: push creates new remote head HEADHASH! (on subrepo MYSUBREPO)
>>> hint: did you forget to merge? use push -f to force
>>>
>>> The rationale for this change is that the current error messages make it
>>> hard
>>> for TortoiseHg (and similar tools) to tell the user which subrepo caused
>>> the
>>> push failure.
>>>
>>> Note that I have also updated test-subrepo.t to reflect this change. The
>>> test
>>> passes on windows.
>>>
>>> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
>>> --- a/mercurial/subrepo.py
>>> +++ b/mercurial/subrepo.py
>>> @@ -567,7 +567,12 @@
>>>            self._repo.ui.status(_('pushing subrepo %s to %s\n') %
>>>                (subrelpath(self), dsturl))
>>>            other = hg.peer(self._repo, {'ssh': ssh}, dsturl)
>>> -        return self._repo.push(other, force, newbranch=newbranch)
>>> +        try:
>>> +            res = self._repo.push(other, force, newbranch=newbranch)
>>> +        except error.Abort, ex:
>>> +            errormsg = ex.message + (' (on subrepo %s)' %
>>> subrelpath(self))
>>
>> The message should be marked up for localization.
> You are right. I believe that it would not be enough to put a "_" call
> around the ' (on subrepo %s)' string, because it would not be found,
> right? Instead I must do:
>
>      errormsg = _('%s (on subrepo %s)' % (ex.message, subrelpath(self)))
>
> Is that correct?

No. _() will be found and the message extracted almost no matter where 
you hide it. You can test with
   xgettext mercurial/subrepo.py
and look at messages.po

> I think push is more important in the sense that it is (in my
> experience) the command that most commonly fails. That being said
> probably most (if not all) other commands could benefit from this. I
> decided to send this simple patch that fixes the main problem but if
> you guys agree I can try to do the same for other commands.
>
> As for using a decorator, are you thinking of something like this?:
>
> def handlessubrepoerror(func):
>      def decoratedmethod(self, *args, **kargs):
>          try:
>              res = func(self, *args, **kargs)
>          except error.Abort, ex:
>              errormsg = _('%s (on subrepo %s)' % (ex.message, subrelpath(self)))
>              raise util.Abort(errormsg, hint=ex.hint)
>          return res
>      return decoratedmethod
>
> If so, would we need to decorate every method on all the subrepo
> subclasses?

Yes, something like that. But it would perhaps be overkill.

> Maybe there is a simpler way to apply such a generator to
> all methods?

Some fancy meta class programming could do it ... but it would be very 
magic and probably not a good idea.

> Also, I worry that adding ' (on subrepo %s)' to every
> error message may result on weird error messages? Maybe it should be
> changed to '(error happened on subrepo %s)' or something of the sort?

Keeping it short is probably what matters the most here.

(And it seems like we have a messy 50/50 between "subrepo" and 
"subrepository" in the user facing messages.)

/Mads