Patchwork [5,of,6,misc] transplant: assert instead of UnboundLocalError on parentrev in .applied()

login
register
mail settings
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

Mads Kiilerich - April 13, 2014, 5:13 p.m.
# 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.
Augie Fackler - April 14, 2014, 2:36 a.m.
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
Mads Kiilerich - April 14, 2014, 5:46 p.m.
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