Submitter | Mads Kiilerich |
---|---|
Date | April 13, 2014, 5:13 p.m. |
Message ID | <52bdb665e6938025c57a.1397409229@localhost.localdomain> |
Download | mbox | patch |
Permalink | /patch/4333/ |
State | Deferred |
Headers | show |
Comments
On Apr 13, 2014, at 1:13 PM, Mads Kiilerich <mads@kiilerich.com> wrote: > # HG changeset patch > # User Mads Kiilerich <madski@unity3d.com> > # Date 1366133616 -7200 > # Tue Apr 16 19:33:36 2013 +0200 > # Node ID 52bdb665e6938025c57a2d745de1306aa9439a9f > # Parent 8674a2e7d16fb130b0af9c541c663462a54bc454 > transplant: assert instead of UnboundLocalError on parentrev in .applied() > > There might be implicit reasons it can't happen, but better be more explicit > and make it fail with a more useful assertion ... if it ever should happen. > > diff --git a/hgext/transplant.py b/hgext/transplant.py > --- a/hgext/transplant.py > +++ b/hgext/transplant.py > @@ -91,10 +91,12 @@ class transplanter(object): > def applied(self, repo, node, parent): > '''returns True if a node is already an ancestor of parent > or is parent or has already been transplanted''' > + parentrev = None > if hasnode(repo, parent): > parentrev = repo.changelog.rev(parent) > if hasnode(repo, node): > rev = repo.changelog.rev(node) > + assert parentrev is not None, (node, parent) Can you put a more descriptive message in the assertion? Eg "missing parent revision for %r" %(node, parent) or something? The message as stated won't help anyone figure out what the invariant we're enforcing is. > reachable = repo.changelog.ancestors([parentrev], rev, > inclusive=True) > if rev in reachable: > @@ -105,6 +107,7 @@ class transplanter(object): > self.transplants.remove(t) > return False > lnoderev = repo.changelog.rev(t.lnode) > + assert parentrev is not None, (node, parent) > if lnoderev in repo.changelog.ancestors([parentrev], lnoderev, > inclusive=True): > return True > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel
On 04/14/2014 04:36 AM, Augie Fackler wrote: > On Apr 13, 2014, at 1:13 PM, Mads Kiilerich <mads@kiilerich.com> wrote: > >> # HG changeset patch >> # User Mads Kiilerich <madski@unity3d.com> >> # Date 1366133616 -7200 >> # Tue Apr 16 19:33:36 2013 +0200 >> # Node ID 52bdb665e6938025c57a2d745de1306aa9439a9f >> # Parent 8674a2e7d16fb130b0af9c541c663462a54bc454 >> transplant: assert instead of UnboundLocalError on parentrev in .applied() >> >> There might be implicit reasons it can't happen, but better be more explicit >> and make it fail with a more useful assertion ... if it ever should happen. >> >> diff --git a/hgext/transplant.py b/hgext/transplant.py >> --- a/hgext/transplant.py >> +++ b/hgext/transplant.py >> @@ -91,10 +91,12 @@ class transplanter(object): >> def applied(self, repo, node, parent): >> '''returns True if a node is already an ancestor of parent >> or is parent or has already been transplanted''' >> + parentrev = None >> if hasnode(repo, parent): >> parentrev = repo.changelog.rev(parent) >> if hasnode(repo, node): >> rev = repo.changelog.rev(node) >> + assert parentrev is not None, (node, parent) > Can you put a more descriptive message in the assertion? Eg "missing parent revision for %r" %(node, parent) or something? > > The message as stated won't help anyone figure out what the invariant we're enforcing is. I agree that the assert without any message would not be very helpful. It will just make it explicit to people reading the code that there is an assumption. And _if_ anyone ever triggers the assertion, it will provide a bit of info that can make us understand how it happened. I haven't spend enough time to understand this code (if I had I would fix/refactor it) and I'm afraid that a nice error message would miss the point and be more confusing than helpful. /Mads
Patch
diff --git a/hgext/transplant.py b/hgext/transplant.py --- a/hgext/transplant.py +++ b/hgext/transplant.py @@ -91,10 +91,12 @@ class transplanter(object): def applied(self, repo, node, parent): '''returns True if a node is already an ancestor of parent or is parent or has already been transplanted''' + parentrev = None if hasnode(repo, parent): parentrev = repo.changelog.rev(parent) if hasnode(repo, node): rev = repo.changelog.rev(node) + assert parentrev is not None, (node, parent) reachable = repo.changelog.ancestors([parentrev], rev, inclusive=True) if rev in reachable: @@ -105,6 +107,7 @@ class transplanter(object): self.transplants.remove(t) return False lnoderev = repo.changelog.rev(t.lnode) + assert parentrev is not None, (node, parent) if lnoderev in repo.changelog.ancestors([parentrev], lnoderev, inclusive=True): return True