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

[Mads Kiilerich]

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 at 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 at selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel