[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 ;-)

LOL. Let's see if we can make this more complete version work. If not
I can go back to the simple, initial proposal.

> You annotate a lot of functions. Are they really all relevant?

Since pretty much every mercurial command can throw an exception (e.g.
if the repisitory is broken) I thought that it would be a good idea to
wrap most subrepo methods. The ones I did not wrap are those that are
either private or which do not seem to possibly throw an error.

Do you think the error should be a bit more targeted, and just
decorate some key methods (e.g. push, get, commit, merge)?

> And if so:
> Shouldn't there be some test coverage of all the places where you add
> handling?

I don't think that should be necessary. The new functionality that I
add, which is the decorator is tested by the existing tests that I
modified already. The decorator works the same regardless of the
method that it decorates so why do more tests?

> And are you sure you don't end up with some Abort messages being
> annotated twice?

That is a good point. Most decorated functions are "top level methods"
which do not seem to be called by other subrepo methods, but there may
be some cases were that could happen. Also it could possibly happen if
there were several nested subrepos.

One solution would be to subclass the util.Abort exception (e.g. as
SubrepoAbort), and only append '(in subrepo %s)' when the catched
exception is not of the subclass exeception type.

>> +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.

Thanks, I'll fix that.

>> @@ -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.

Left over from the simple version of the patch, sorry. I'll fix that as well.

>
>> @@ -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.

:-) I guess that proves the value of getting the test suite to work on
windows, as hard as that has been.

I did not know what "(glob)" does. In hindsight it makes a lot of
sense. I'll fix that as well.

> test-subrepo-git.t and test-subrepo-svn.t also needs updating (but might be
> a bit tricky to run on windows).

Yeah, I don't think those work. I don't even have git, much less svn installed.
I could split this patch in three, resubmit it with the hg only
changes, which I can test right of the bat, and maybe resend the git
and svn changes if I get them to work somehow (perhaps on a Linux VM)?

> 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.

I agree. If I create the SubrepoAbort class that could be fixed by
throwing SubrepoAbort wherever the error message already includes the
subrepo path info. What do you think?

Thanks again for your review,

Angel