Patchwork [V2] rebase: do not raise an UnboundLocalError when called with wrong rev (issue4106)

# HG changeset patch
# User Simon Heimberg <simohe@besonet.ch>
# Date 1392334460 -3600
#      Fri Feb 14 00:34:20 2014 +0100
# Node ID 2554f90793b6645be68348729bedda8f695b979f
# Parent  0e2877f8605dcaf4fdf2ab7e0046f1f6f80161dd
rebase: do not raise an UnboundLocalError when called with wrong rev (issue4106)

When the base is not found, we should not raise a traceback about a not defined
variable. This hides the real problem. The function rebasenode was probaly
called with wrong revisions.

An AssertionError is raised to highlight that the caller of the function did
something wrong.
An alternative approach is to assign None to the variable "base" and let the
merge mechanism raise an abort message. This was the behavior for this case
before ad9db007656f. But the only known case for this problem is when an
extension calls this function wrong. An AssertionError makes this clearer than
an abort message. When a different case is detected, the behaviour can be
improved then.

On 2/19/14, 2:55 AM, "Simon Heimberg" <simohe@besonet.ch> wrote:

>diff -r 0e2877f8605d -r 2554f90793b6 hgext/rebase.py
>--- a/hgext/rebase.py	Sat Feb 15 22:09:32 2014 -0600
>+++ b/hgext/rebase.py	Fri Feb 14 00:34:20 2014 +0100
>@@ -513,6 +513,11 @@
>             if state.get(p.rev()) == repo[p1].rev():
>                 base = p.node()
>                 break
>+        else:
>+            # fallback when base not found (unnormal, see issue 4106)
>+            #
>+            # Raise because this function is (probaby) called wrong
>+            raise AssertionError('function is called wrong')

1. I think we could do better as Œfunction is called wrong¹. Maybe Œbase
must be defined¹. 
2. I personally would have an invariant before the
 assert Œbase¹ in locals()
 if base is not None:
making it clear that base needs to be defined after the whole if-else
statement.

- David

I will send V3 soon. More details below.

On 2014-02-19 20:55, David Soria Parra wrote:
> On 2/19/14, 2:55 AM, "Simon Heimberg" <simohe@besonet.ch> wrote:
>> diff -r 0e2877f8605d -r 2554f90793b6 hgext/rebase.py
>> --- a/hgext/rebase.py	Sat Feb 15 22:09:32 2014 -0600
>> +++ b/hgext/rebase.py	Fri Feb 14 00:34:20 2014 +0100
>> @@ -513,6 +513,11 @@
>>              if state.get(p.rev()) == repo[p1].rev():
>>                  base = p.node()
>>                  break
>> +        else:
>> +            # fallback when base not found (unnormal, see issue 4106)
>> +            #
>> +            # Raise because this function is (probaby) called wrong
>> +            raise AssertionError('function is called wrong')
> 1. I think we could do better as Œfunction is called wrong¹. Maybe Œbase
> must be defined¹.
We can not mention the variable "base", the caller can not define it. 
Maybe "no base found to rebase on (rebasenode called wrong)"?
> 2. I personally would have an invariant before the
>   assert Œbase¹ in locals()
>   if base is not None:
> making it clear that base needs to be defined after the whole if-else
> statement.
??
We can add "base = None" in else, it is always undefined there. Then we 
could just run on, the problem of not finding a valid base would be 
reported later (somehow).
By raising an AssertionError, we make clear that the caller of this 
function did something wrong. (And raising AssertionError is never 
optimized away, unlike assert.) The patch description tells this case 
only happens when an extension calls rebasenode() wrong.
> - David