| Submitter | Angel Ezquerra |
|---|---|
| Date | Dec. 16, 2012, 11:24 p.m. |
| Message ID | <964686dbc23ea2e66c5f.1355700247@Lucia-PC> |
| Download | mbox | patch |
| Permalink | /patch/149/ |
| State | Superseded |
| Commit | 9e3910db4e787787c315808f37ceecf695cb18c9 |
| Headers | show |
Comments
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've tried to decorate those subrepo methods were I think it would make sense to handle errors in this manner. I've also had to chagne an existing error message to accommodate the way we append the subrepo that is in error to all error messages. Cheers, Angel
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
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
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
Patch
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py --- a/mercurial/subrepo.py +++ b/mercurial/subrepo.py @@ -14,6 +14,16 @@ nullstate = ('', '', 'empty') +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 + 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 +254,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 +411,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 +432,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 +449,7 @@ % (inst, subrelpath(self))) return [], [], [], [], [], [], [] + @handlessubrepoerror def diff(self, ui, diffopts, node2, match, prefix, **opts): try: node1 = node.bin(self._state[1]) @@ -452,6 +465,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 +477,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 +494,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 +506,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 +536,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 +570,7 @@ else: mergefunc() + @handlessubrepoerror def push(self, opts): force = opts.get('force') newbranch = opts.get('new_branch') @@ -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 + @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 +617,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 +777,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 +805,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 +830,7 @@ except OSError: pass + @handlessubrepoerror def get(self, state, overwrite=False): if overwrite: self._svncommand(['revert', '--recursive']) @@ -822,6 +851,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 +865,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 +1052,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 +1069,7 @@ def basestate(self): return self._gitstate() + @handlessubrepoerror def get(self, state, overwrite=False): source, revision, kind = state if not revision: @@ -1120,6 +1153,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 +1171,7 @@ # circumstances return self._gitstate() + @handlessubrepoerror def merge(self, state): source, revision, kind = state self._fetch(source, revision) @@ -1159,6 +1194,7 @@ else: mergefunc() + @handlessubrepoerror def push(self, opts): force = opts.get('force') @@ -1198,6 +1234,7 @@ (self._relpath, self._state[1])) return False + @handlessubrepoerror def remove(self): if self._gitmissing(): return @@ -1247,6 +1284,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.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! (on 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! (on 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 (on 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 (on subrepo sub\repo) [255] Pull -u now doesn't help