Patchwork D1506: dagop: handle IndexError when using wdir() in dag range

login
register
mail settings
Submitter phabricator
Date Nov. 24, 2017, 12:06 p.m.
Message ID <differential-rev-PHID-DREV-jtg5btrlejtuimcdgnrr-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25735/
State New
Headers show

Comments

phabricator - Nov. 24, 2017, 12:06 p.m.
swhitaker created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Using wdir() in a dag range revset can crash Mercurial. For example:
  
    hg status --rev '.^::wdir()
  
  revlog.c reports an IndexError in this instance, but it isn't caught
  by the calling code. This change adds IndexError to the set of exception
  types the calling code catches. When an IndexError is caught, the code
  falls back to calling the pure Python implementation of reachableroots,
  which fails gracefully.

TEST PLAN
  $ hg status --rev '.::wdir()'
  abort: working directory revision cannot be specified

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1506

AFFECTED FILES
  mercurial/dagop.py

CHANGE DETAILS




To: swhitaker, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 25, 2017, 3:54 a.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  I think it's better to fail fast instead of trying the slow path which is supposed
  to raise WdirUnsupported exception.
  
  > $ hg status --rev '.::wdir()'
  >  abort: working directory revision cannot be specified
  
  Can you add test?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1506

To: swhitaker, #hg-reviewers, mbthomas, yuja
Cc: yuja, mercurial-devel
phabricator - Nov. 26, 2017, 9:42 a.m.
ryanmce added a comment.


  I understand that fixing the crash is a good first step, but ideally, wouldn't this be supported? Is there a reason it's particularly hard to support?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1506

To: swhitaker, #hg-reviewers, mbthomas, yuja
Cc: ryanmce, yuja, mercurial-devel
phabricator - Nov. 26, 2017, 2:01 p.m.
yuja added a comment.


  In https://phab.mercurial-scm.org/D1506#25509, @ryanmce wrote:
  
  > but ideally, wouldn't this be supported?
  
  
  Yeah, this case, `includepath=True`, won't be difficult to handle. If reachableroots() raised
  WdirUnsupportedError, drop wdir revision from `y` of `x::y`, add parents instead, rerun
  `x::(y - wdir() + parents(wdir()))`, and append wdir to the result.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1506

To: swhitaker, #hg-reviewers, mbthomas, yuja
Cc: ryanmce, yuja, mercurial-devel

Patch

diff --git a/mercurial/dagop.py b/mercurial/dagop.py
--- a/mercurial/dagop.py
+++ b/mercurial/dagop.py
@@ -230,7 +230,7 @@ 
     heads = list(heads)
     try:
         revs = repo.changelog.reachableroots(minroot, heads, roots, includepath)
-    except AttributeError:
+    except (AttributeError, IndexError):
         revs = _reachablerootspure(repo, minroot, roots, heads, includepath)
     revs = baseset(revs)
     revs.sort()