Patchwork [V4] subrepo: append subrepo path to subrepo error messages

login
register
mail settings
Submitter Angel Ezquerra
Date Dec. 18, 2012, 10:35 p.m.
Message ID <f15c5ad6479fd34258da.1355870144@Lucia-PC>
Download mbox | patch
Permalink /patch/187/
State Superseded
Commit 9e3910db4e787787c315808f37ceecf695cb18c9
Headers show

Comments

Angel Ezquerra - Dec. 18, 2012, 10:35 p.m.
# HG changeset patch
# User Angel Ezquerra <angel.ezquerra at gmail.com>
# Date 1355438273 -3600
# Node ID f15c5ad6479fd34258da8f78ff87c0ec6e90fd36
# 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! (in 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 "(in subrepo MYSUBREPO)" message has been added to those subrepo methods
were it made sense (by using a decorator). We avoid appending "(in subrepo XXX)"
multiple times when subrepos are nexted by throwing a "SubrepoAbort" exception
after the extra message is appended. The decorator will then "ignore" (i.e. just
re-raise) the exception and never add the message again.

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 several subrepo related tests to reflect these
changes.
Angel Ezquerra - Dec. 18, 2012, 10:40 p.m.
On Tue, Dec 18, 2012 at 11:35 PM, Angel Ezquerra
<angel.ezquerra at gmail.com> wrote:
> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra at gmail.com>
> # Date 1355438273 -3600
> # Node ID f15c5ad6479fd34258da8f78ff87c0ec6e90fd36
> # 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! (in 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 "(in subrepo MYSUBREPO)" message has been added to those subrepo methods
> were it made sense (by using a decorator). We avoid appending "(in subrepo XXX)"
> multiple times when subrepos are nexted by throwing a "SubrepoAbort" exception
> after the extra message is appended. The decorator will then "ignore" (i.e. just
> re-raise) the exception and never add the message again.
>
> 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 several subrepo related tests to reflect these
> changes.
>

This new version tries to address the issues raised by Mads, Matt
Harbison and Laurens. In particular I hope to have gotten the
localization code right, changed the message from '(on subrepo XXX)'
to '(in subrepo XXX)' for consistency's sake, avoid appending the
message more than once and updated more of the subrepo tests (the ones
I could run on windows work fine).

Other than some tests that may fail on non windows platforms the only
remaining issue (IMHO) is whether we should reduce the number of
methods that are decorated to catch subrepo errors.

It would be great if someone could run the tests on Linux. All other
comments are also welcome!

Cheers,

Angel
Matt Harbison - Dec. 19, 2012, 12:37 a.m.
On Tue, 18 Dec 2012 23:40:37 +0100, Angel Ezquerra wrote:

> On Tue, Dec 18, 2012 at 11:35 PM, Angel Ezquerra
> <angel.ezquerra at gmail.com> wrote:
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>> # Date 1355438273 -3600
>> # Node ID f15c5ad6479fd34258da8f78ff87c0ec6e90fd36
>> # Parent  34a1a639d8358e43f4bcba7b0cff19f4e4e6958d
>> subrepo: append subrepo path to subrepo error messages
>>

[..]

> This new version tries to address the issues raised by Mads, Matt
> Harbison and Laurens. In particular I hope to have gotten the
> localization code right, changed the message from '(on subrepo XXX)' to
> '(in subrepo XXX)' for consistency's sake, avoid appending the message
> more than once and updated more of the subrepo tests (the ones I could
> run on windows work fine).
> 
> Other than some tests that may fail on non windows platforms the only
> remaining issue (IMHO) is whether we should reduce the number of methods
> that are decorated to catch subrepo errors.
> 
> It would be great if someone could run the tests on Linux. All other
> comments are also welcome!
> 
> Cheers,
> 
> Angel

The whole test suite runs cleanly on Linux.

One question I didn't think of before, but does python support exception 
chaining like Java does so you can get the whole trace, or will it be 
truncated where the abort is caught and the abort subclass raised?  (Or is 
that what the hint is?)  I think the feature is worthwhile even if it 
can't be chained, but it might make debugging more of a pain.

Just a thought for thg- if such an error occurs, maybe the subrepo name in 
the message should be a clickable hyperlink?

--Matt
Matt Harbison - Dec. 19, 2012, 2:49 a.m.
Pierre-Yves David wrote:
> On 19 d?c. 2012, at 01:37, Matt Harbison wrote:
>
>> On Tue, 18 Dec 2012 23:40:37 +0100, Angel Ezquerra wrote:
>>
>>> On Tue, Dec 18, 2012 at 11:35 PM, Angel Ezquerra
>>> <angel.ezquerra at gmail.com>  wrote:
>>>> # HG changeset patch
>>>> # User Angel Ezquerra<angel.ezquerra at gmail.com>
>>>> # Date 1355438273 -3600
>>>> # Node ID f15c5ad6479fd34258da8f78ff87c0ec6e90fd36
>>>> # Parent  34a1a639d8358e43f4bcba7b0cff19f4e4e6958d
>>>> subrepo: append subrepo path to subrepo error messages
>>>>
>> [..]
>>
>>> This new version tries to address the issues raised by Mads, Matt
>>> Harbison and Laurens. In particular I hope to have gotten the
>>> localization code right, changed the message from '(on subrepo XXX)' to
>>> '(in subrepo XXX)' for consistency's sake, avoid appending the message
>>> more than once and updated more of the subrepo tests (the ones I could
>>> run on windows work fine).
>>>
>>> Other than some tests that may fail on non windows platforms the only
>>> remaining issue (IMHO) is whether we should reduce the number of methods
>>> that are decorated to catch subrepo errors.
>>>
>>> It would be great if someone could run the tests on Linux. All other
>>> comments are also welcome!
>>>
>>> Cheers,
>>>
>>> Angel
>> The whole test suite runs cleanly on Linux.
>>
>> One question I didn't think of before, but does python support exception
>> chaining like Java does so you can get the whole trace, or will it be
>> truncated where the abort is caught and the abort subclass raised?  (Or is
>> that what the hint is?)  I think the feature is worthwhile even if it
>> can't be chained, but it might make debugging more of a pain.
>
> Yes \o/
>
> http://www.python.org/dev/peps/pep-0409/
>
> In python 3.3 /o\

That's too bad.  Would it be possible/acceptable (in another patch) to 
pass the original exception to the custom exception's constructor, and 
in dispatch._runcatch(), pass the original from the custom exception to 
ui.traceback()?  I realize that this is kinda nasty from a code 
organization POV, (why should dispatch know about something in the 
subrepo module?), but I think this is outweighed by being able to get 
the whole stacktrace with a simple --traceback when users report problems.

Are there any other similar issues with other parts of the code?  If so, 
maybe the custom exception should be a generic ChainedAbort class or 
something so this hack is reusable?

--Matt
Bryan O'Sullivan - Dec. 20, 2012, 12:59 a.m.
On Tue, Dec 18, 2012 at 2:35 PM, Angel Ezquerra <angel.ezquerra at gmail.com>wrote:

> +def handlessubrepoerror(func):
>

Could you maybe rename this to "annotatesubrepoerror"? I keep wondering
what a handless ubrepo is.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20121219/aec4c2f8/attachment.html>
Angel Ezquerra - Dec. 20, 2012, 8:06 a.m.
On Thu, Dec 20, 2012 at 1:59 AM, Bryan O'Sullivan <bos at serpentine.com> wrote:
> On Tue, Dec 18, 2012 at 2:35 PM, Angel Ezquerra <angel.ezquerra at gmail.com>
> wrote:
>>
>> +def handlessubrepoerror(func):
>
>
> Could you maybe rename this to "annotatesubrepoerror"? I keep wondering what
> a handless ubrepo is.

Sure, I will do so and resend the patch. Not being a native speaker,
that did not occur to me until you pointed it out.

Do you have any other comments on the patch?

Thanks!

Angel
Bryan O'Sullivan - Dec. 20, 2012, 7:43 p.m.
On Thu, Dec 20, 2012 at 12:06 AM, Angel Ezquerra
<angel.ezquerra at gmail.com>wrote:

> Sure, I will do so and resend the patch. Not being a native speaker,
> that did not occur to me until you pointed it out.
>

:)


> Do you have any other comments on the patch?
>

No, otherwise it looks good. Thanks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20121220/355cc490/attachment.html>
Angel Ezquerra - Dec. 20, 2012, 7:59 p.m.
On Thu, Dec 20, 2012 at 8:43 PM, Bryan O'Sullivan <bos at serpentine.com> wrote:
> On Thu, Dec 20, 2012 at 12:06 AM, Angel Ezquerra <angel.ezquerra at gmail.com>
> wrote:
>>
>> Sure, I will do so and resend the patch. Not being a native speaker,
>> that did not occur to me until you pointed it out.
>
>
> :)
>
>>
>> Do you have any other comments on the patch?
>
>
> No, otherwise it looks good. Thanks.

Awesome!

I just resent the patch with your suggested change. I re-ran the
subrepo part of the test suite on Linux and it still passes all tests
(except the svn one, since I do not have SVN installed).

Cheers,

Angel

Patch

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -14,6 +14,23 @@ 
 
 nullstate = ('', '', 'empty')
 
+class SubrepoAbort(error.Abort):
+    """Exception class used to avoid handling a subrepo error more than once"""
+
+def handlessubrepoerror(func):
+    def decoratedmethod(self, *args, **kargs):
+        try:
+            res = func(self, *args, **kargs)
+        except SubrepoAbort, ex:
+            # This exception has already been handled
+            raise ex
+        except error.Abort, ex:
+            errormsg = _('%s (in subrepo %s)') % (str(ex), subrelpath(self))
+            # avoid handling this exception by raising a SubrepoAbort exception
+            raise SubrepoAbort(errormsg, hint=ex.hint)
+        return res
+    return decoratedmethod
+
 def state(ctx, ui):
     """return a state dict, mapping subrepo paths configured in .hgsub
     to tuple: (source from .hgsub, revision from .hgsubstate, kind
@@ -244,8 +261,7 @@ 
         if repo.ui.config('paths', 'default'):
             return repo.ui.config('paths', 'default')
     if abort:
-        raise util.Abort(_("default path for subrepository %s not found") %
-            reporelpath(repo))
+        raise util.Abort(_("default path for subrepository not found"))
 
 def itersubrepos(ctx1, ctx2):
     """find subrepos in ctx1 or ctx2"""
@@ -402,6 +418,7 @@ 
                 self._repo.ui.setconfig(s, k, v)
         self._initrepo(r, state[0], create)
 
+    @handlessubrepoerror
     def _initrepo(self, parentrepo, source, create):
         self._repo._subparent = parentrepo
         self._repo._subsource = source
@@ -422,10 +439,12 @@ 
                 addpathconfig('default-push', defpushpath)
             fp.close()
 
+    @handlessubrepoerror
     def add(self, ui, match, dryrun, listsubrepos, prefix, explicitonly):
         return cmdutil.add(ui, self._repo, match, dryrun, listsubrepos,
                            os.path.join(prefix, self._path), explicitonly)
 
+    @handlessubrepoerror
     def status(self, rev2, **opts):
         try:
             rev1 = self._state[1]
@@ -437,6 +456,7 @@ 
                                % (inst, subrelpath(self)))
             return [], [], [], [], [], [], []
 
+    @handlessubrepoerror
     def diff(self, ui, diffopts, node2, match, prefix, **opts):
         try:
             node1 = node.bin(self._state[1])
@@ -452,6 +472,7 @@ 
             self._repo.ui.warn(_('warning: error "%s" in subrepository "%s"\n')
                                % (inst, subrelpath(self)))
 
+    @handlessubrepoerror
     def archive(self, ui, archiver, prefix, match=None):
         self._get(self._state + ('hg',))
         abstractsubrepo.archive(self, ui, archiver, prefix, match)
@@ -463,6 +484,7 @@ 
             submatch = matchmod.narrowmatcher(subpath, match)
             s.archive(ui, archiver, os.path.join(prefix, self._path), submatch)
 
+    @handlessubrepoerror
     def dirty(self, ignoreupdate=False):
         r = self._state[1]
         if r == '' and not ignoreupdate: # no state recorded
@@ -479,6 +501,7 @@ 
     def checknested(self, path):
         return self._repo._checknested(self._repo.wjoin(path))
 
+    @handlessubrepoerror
     def commit(self, text, user, date):
         # don't bother committing in the subrepo if it's only been
         # updated
@@ -490,6 +513,7 @@ 
             return self._repo['.'].hex() # different version checked out
         return node.hex(n)
 
+    @handlessubrepoerror
     def remove(self):
         # we can't fully delete the repository as it may contain
         # local-only history
@@ -519,12 +543,14 @@ 
                 bookmarks.updatefromremote(self._repo.ui, self._repo, other,
                                            srcurl)
 
+    @handlessubrepoerror
     def get(self, state, overwrite=False):
         self._get(state)
         source, revision, kind = state
         self._repo.ui.debug("getting subrepo %s\n" % self._path)
         hg.updaterepo(self._repo, revision, overwrite)
 
+    @handlessubrepoerror
     def merge(self, state):
         self._get(state)
         cur = self._repo['.']
@@ -551,6 +577,7 @@ 
         else:
             mergefunc()
 
+    @handlessubrepoerror
     def push(self, opts):
         force = opts.get('force')
         newbranch = opts.get('new_branch')
@@ -569,12 +596,15 @@ 
         other = hg.peer(self._repo, {'ssh': ssh}, dsturl)
         return self._repo.push(other, force, newbranch=newbranch)
 
+    @handlessubrepoerror
     def outgoing(self, ui, dest, opts):
         return hg.outgoing(ui, self._repo, _abssource(self._repo, True), opts)
 
+    @handlessubrepoerror
     def incoming(self, ui, source, opts):
         return hg.incoming(ui, self._repo, _abssource(self._repo, False), opts)
 
+    @handlessubrepoerror
     def files(self):
         rev = self._state[1]
         ctx = self._repo[rev]
@@ -593,10 +623,12 @@ 
         ctx = self._repo[None]
         return ctx.walk(match)
 
+    @handlessubrepoerror
     def forget(self, ui, match, prefix):
         return cmdutil.forget(ui, self._repo, match,
                               os.path.join(prefix, self._path), True)
 
+    @handlessubrepoerror
     def revert(self, ui, substate, *pats, **opts):
         # reverting a subrepo is a 2 step process:
         # 1. if the no_backup is not set, revert all modified
@@ -751,6 +783,7 @@ 
                 pass
         return rev
 
+    @handlessubrepoerror
     def commit(self, text, user, date):
         # user and date are out of our hands since svn is centralized
         changed, extchanged, missing = self._wcchanged()
@@ -778,6 +811,7 @@ 
         self._ui.status(self._svncommand(['update', '-r', newrev])[0])
         return newrev
 
+    @handlessubrepoerror
     def remove(self):
         if self.dirty():
             self._ui.warn(_('not removing repo %s because '
@@ -802,6 +836,7 @@ 
         except OSError:
             pass
 
+    @handlessubrepoerror
     def get(self, state, overwrite=False):
         if overwrite:
             self._svncommand(['revert', '--recursive'])
@@ -822,6 +857,7 @@ 
             raise util.Abort((status or err).splitlines()[-1])
         self._ui.status(status)
 
+    @handlessubrepoerror
     def merge(self, state):
         old = self._state[1]
         new = state[1]
@@ -835,6 +871,7 @@ 
         # push is a no-op for SVN
         return True
 
+    @handlessubrepoerror
     def files(self):
         output = self._svncommand(['list', '--recursive', '--xml'])[0]
         doc = xml.dom.minidom.parseString(output)
@@ -1021,6 +1058,7 @@ 
             raise util.Abort(_("revision %s does not exist in subrepo %s\n") %
                                (revision, self._relpath))
 
+    @handlessubrepoerror
     def dirty(self, ignoreupdate=False):
         if self._gitmissing():
             return self._state[1] != ''
@@ -1037,6 +1075,7 @@ 
     def basestate(self):
         return self._gitstate()
 
+    @handlessubrepoerror
     def get(self, state, overwrite=False):
         source, revision, kind = state
         if not revision:
@@ -1120,6 +1159,7 @@ 
             # a real merge would be required, just checkout the revision
             rawcheckout()
 
+    @handlessubrepoerror
     def commit(self, text, user, date):
         if self._gitmissing():
             raise util.Abort(_("subrepo %s is missing") % self._relpath)
@@ -1137,6 +1177,7 @@ 
         # circumstances
         return self._gitstate()
 
+    @handlessubrepoerror
     def merge(self, state):
         source, revision, kind = state
         self._fetch(source, revision)
@@ -1159,6 +1200,7 @@ 
         else:
             mergefunc()
 
+    @handlessubrepoerror
     def push(self, opts):
         force = opts.get('force')
 
@@ -1198,6 +1240,7 @@ 
                           (self._relpath, self._state[1]))
             return False
 
+    @handlessubrepoerror
     def remove(self):
         if self._gitmissing():
             return
@@ -1247,6 +1290,7 @@ 
         ui.progress(_('archiving (%s)') % relpath, None)
 
 
+    @handlessubrepoerror
     def status(self, rev2, **opts):
         rev1 = self._state[1]
         if self._gitmissing() or not rev1:
diff --git a/tests/test-subrepo-git.t b/tests/test-subrepo-git.t
--- a/tests/test-subrepo-git.t
+++ b/tests/test-subrepo-git.t
@@ -331,10 +331,10 @@ 
   $ hg sum | grep commit
   commit: 1 subrepos
   $ hg push -q
-  abort: subrepo s is missing
+  abort: subrepo s is missing (in subrepo s)
   [255]
   $ hg commit --subrepos -qm missing
-  abort: subrepo s is missing
+  abort: subrepo s is missing (in subrepo s)
   [255]
   $ hg update -C
   cloning subrepo s from $TESTTMP/gitroot
diff --git a/tests/test-subrepo-recursion.t b/tests/test-subrepo-recursion.t
--- a/tests/test-subrepo-recursion.t
+++ b/tests/test-subrepo-recursion.t
@@ -386,7 +386,7 @@ 
   $ echo f > foo/f
   $ hg archive --subrepos -r tip archive
   cloning subrepo foo from $TESTTMP/empty/foo
-  abort: destination '$TESTTMP/almost-empty/foo' is not empty (glob)
+  abort: destination '$TESTTMP/almost-empty/foo' is not empty (in subrepo foo) (glob)
   [255]
 
 Clone and test outgoing:
diff --git a/tests/test-subrepo-svn.t b/tests/test-subrepo-svn.t
--- a/tests/test-subrepo-svn.t
+++ b/tests/test-subrepo-svn.t
@@ -119,7 +119,7 @@ 
   $ rm s/alpha
   $ hg commit --subrepos -m 'abort on missing file'
   committing subrepository s
-  abort: cannot commit missing svn entries
+  abort: cannot commit missing svn entries (in subrepo s)
   [255]
   $ svn revert s/alpha > /dev/null
 
@@ -180,7 +180,7 @@ 
   $ echo zzz > s/externals/other
   $ hg ci --subrepos -m 'amend externals from hg'
   committing subrepository s
-  abort: cannot commit svn externals
+  abort: cannot commit svn externals (in subrepo s)
   [255]
   $ hg diff --subrepos -r 1:2 | grep -v diff
   --- a/.hgsubstate	Thu Jan 01 00:00:00 1970 +0000
@@ -202,7 +202,7 @@ 
   property 'svn:mime-type' set on 's/externals/other' (glob)
   $ hg ci --subrepos -m 'amend externals from hg'
   committing subrepository s
-  abort: cannot commit svn externals
+  abort: cannot commit svn externals (in subrepo s)
   [255]
   $ svn revert -q s/externals/other
 
diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
--- a/tests/test-subrepo.t
+++ b/tests/test-subrepo.t
@@ -320,7 +320,7 @@ 
   no changes found
   pushing subrepo s to $TESTTMP/t/s (glob)
   searching for changes
-  abort: push creates new remote head 12a213df6fa9!
+  abort: push creates new remote head 12a213df6fa9! (in subrepo s)
   (did you forget to merge? use push -f to force)
   [255]
   $ hg push -f
@@ -587,7 +587,7 @@ 
   created new head
   $ hg -R repo2 ci -m3
   $ hg -q -R repo2 push
-  abort: push creates new remote head cc505f09a8b2!
+  abort: push creates new remote head cc505f09a8b2! (in subrepo s)
   (did you forget to merge? use push -f to force)
   [255]
   $ hg -R repo update
@@ -599,7 +599,7 @@ 
   $ hg -R repo2 push -f -q
   $ hg -R repo update
   b: untracked file differs
-  abort: untracked files in working directory differ from files in requested revision
+  abort: untracked files in working directory differ from files in requested revision (in subrepo s)
   [255]
 
   $ cat repo/s/b
@@ -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 (in subrepo sub/repo) (glob)
   [255]
 
 Pull -u now doesn't help