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

login
register
mail settings
Submitter Mads Kiilerich
Date April 16, 2013, 5:49 p.m.
Message ID <8e823f137a686b159fb6.1366134550@mk-desktop>
Download mbox | patch
Permalink /patch/1355/
State Rejected, archived
Headers show

Comments

Mads Kiilerich - April 16, 2013, 5:49 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1366133616 -7200
#      Tue Apr 16 19:33:36 2013 +0200
# Node ID 8e823f137a686b159fb67633d161032e53f0dc3c
# Parent  3f621c427c998d049888ab60e6f40b7258f9f65f
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 17, 2013, 2 p.m.
On Tue, Apr 16, 2013 at 07:49:10PM +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1366133616 -7200
> #      Tue Apr 16 19:33:36 2013 +0200
> # Node ID 8e823f137a686b159fb67633d161032e53f0dc3c
> # Parent  3f621c427c998d049888ab60e6f40b7258f9f65f
> 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.

If it's really a barrier condition, don't use assert, because that
gets dropped by "python -O". Consider using an if/raise block instead?

>
> diff --git a/hgext/transplant.py b/hgext/transplant.py
> --- a/hgext/transplant.py
> +++ b/hgext/transplant.py
> @@ -91,10 +91,12 @@
>      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, (revlog.hex(parent), rev)
>              reachable = repo.changelog.ancestors([parentrev], rev,
>                                                   inclusive=True)
>              if rev in reachable:
> @@ -105,6 +107,7 @@
>                  self.transplants.remove(t)
>                  return False
>              lnoderev = repo.changelog.rev(t.lnode)
> +            assert parentrev is not None, (revlog.hex(parent), lnoderev)
>              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 17, 2013, 2:25 p.m.
On 04/17/2013 04:00 PM, Augie Fackler wrote:
> On Tue, Apr 16, 2013 at 07:49:10PM +0200, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1366133616 -7200
>> #      Tue Apr 16 19:33:36 2013 +0200
>> # Node ID 8e823f137a686b159fb67633d161032e53f0dc3c
>> # Parent  3f621c427c998d049888ab60e6f40b7258f9f65f
>> 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.
> If it's really a barrier condition, don't use assert, because that
> gets dropped by "python -O". Consider using an if/raise block instead?

Yes, that would be the general argument against using assertions both in 
general, in Python and in Mercurial.

The goal here was to make the assumption explicit. Looking at the 
function without considering these assumptions would be an error that 
would be caught by a sufficiently clever (and stupid) static code 
analysis tool. Making the assumption explicit helps users and hackers of 
the function and help debugging violations of the assumptions. I assume 
it must be a programming error if anyone ever hits this case, so a 
friendly and verbose runtime check would be overkill.

Removing these assertions will in worst case only make the program crash 
in a less useful way than it would with these assertions.

Assertions thus seems like just the right hammer for this nail.

/Mads


>
>> diff --git a/hgext/transplant.py b/hgext/transplant.py
>> --- a/hgext/transplant.py
>> +++ b/hgext/transplant.py
>> @@ -91,10 +91,12 @@
>>       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, (revlog.hex(parent), rev)
>>               reachable = repo.changelog.ancestors([parentrev], rev,
>>                                                    inclusive=True)
>>               if rev in reachable:
>> @@ -105,6 +107,7 @@
>>                   self.transplants.remove(t)
>>                   return False
>>               lnoderev = repo.changelog.rev(t.lnode)
>> +            assert parentrev is not None, (revlog.hex(parent), lnoderev)
>>               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

Patch

diff --git a/hgext/transplant.py b/hgext/transplant.py
--- a/hgext/transplant.py
+++ b/hgext/transplant.py
@@ -91,10 +91,12 @@ 
     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, (revlog.hex(parent), rev)
             reachable = repo.changelog.ancestors([parentrev], rev,
                                                  inclusive=True)
             if rev in reachable:
@@ -105,6 +107,7 @@ 
                 self.transplants.remove(t)
                 return False
             lnoderev = repo.changelog.rev(t.lnode)
+            assert parentrev is not None, (revlog.hex(parent), lnoderev)
             if lnoderev in repo.changelog.ancestors([parentrev], lnoderev,
                                                     inclusive=True):
                 return True