[PATCH V3 [RETITLED]] subrepo: append subrepo path to subrepo error messages

[Angel Ezquerra]

On Mon, Dec 17, 2012 at 1:35 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
> Angel Ezquerra wrote, On 12/17/2012 12:27 AM:
>
>> On Mon, Dec 17, 2012 at 12:24 AM, Angel Ezquerra
>> <angel.ezquerra at gmail.com> wrote:
>>>
>>> # HG changeset patch
>>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>>> # Date 1355438273 -3600
>>> # Node ID 964686dbc23ea2e66c5f86ebffe96425096dd89c
>>> # Parent  34a1a639d8358e43f4bcba7b0cff19f4e4e6958d
>>> subrepo: append subrepo path to subrepo error messages
>>>
>>> This change appends the subrepo path to subrepo errors. That is, when
>>> there
>>> is an error performing an operation a subrepo, rather than displaying a
>>> message
>>> such as:
>>>
>>> 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.
>>>
>>> The "(on subrepo MYSUBREPO)" message has been added to those subrepo
>>> methods
>>> were it made sense (by using a decorator).
>>>
>>> Because the state() function already printed the subrepo path when it
>>> threw an
>>> error, that error has been changed to avoid duplicating the subrepo path
>>> in the
>>> error message.
>>>
>>> Note that I have also updated test-subrepo.t to reflect these changes.
>>> The test
>>> passes on windows.
>>>
>> This is a new version of the patch that was titled "".
>> This new version tries to follow Mads advice of using a decorator and
>> adding the subrepo that caused the error to all subrepo errors.
>
>
> I think it was more of an idea than an advice. And not necessarily a good
> idea ;-)
>
> You annotate a lot of functions. Are they really all relevant? And if so:
> Shouldn't there be some test coverage of all the places where you add
> handling? And are you sure you don't end up with some Abort messages being
> annotated twice?
>
>
>> +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)))
>
>
> The _() should only wrap the text, not the % expansion.
>
>
>> @@ -567,14 +587,18 @@
>>           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)
>> +        res = self._repo.push(other, force, newbranch=newbranch)
>> +        return res
>
>
> That is an unnecessary change.
>
>
>> @@ -645,7 +645,7 @@
>>     added 2 changesets with 3 changes to 2 files
>>     (run 'hg update' to get a working copy)
>>     $ hg -R issue1852b update
>> -  abort: default path for subrepository sub/repo not found (glob)
>> +  abort: default path for subrepository not found (on subrepo sub\repo)
>>     [255]
>
>
> This is a first. We usually have problems forgetting that windows paths use
> \ ... but here it is the other way around. The \ should be a / and (glob)
> should be appended to the line so run-tests knows that both variants are
> accepted.
>
>
> test-subrepo-git.t and test-subrepo-svn.t also needs updating (but might be
> a bit tricky to run on windows).
>
>
> A message like
>   abort: destination '$TESTTMP/almost-empty/foo' is not empty (on subrepo
> foo)
> mig be ok, but a commit failing with
>   abort: subrepo s is missing (on subrepo s)
> seems a bit like it is repeating itself.
>
> /Mads

Mads,

I forgot to mention that I think it is not bad to show the local path
even when you show the remote path on the error message. These are
usually very similar but if you use absolute subrepo paths they can be
very different as well.

Cheers,

Angel