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

login
register
mail settings
Submitter Simon Heimberg
Date Feb. 13, 2014, 11:41 p.m.
Message ID <0ed5bb67f3403b8e0655.1392334905@lapsi.heimberg.home>
Download mbox | patch
Permalink /patch/3654/
State Superseded
Headers show

Comments

Matt Mackall - Feb. 13, 2014, 11:20 p.m.
On Fri, 2014-02-14 at 00:41 +0100, Simon Heimberg wrote:
> # HG changeset patch
> # User Simon Heimberg <simohe@besonet.ch>
> # Date 1392334460 -3600
> # Node ID 0ed5bb67f3403b8e0655ee5ff2a7aeb32e307837
> # Parent  0f1ef9e9e904c18f1ac96aef3a0e0d3aa5f1190c
> rebase: do not raise an UnboundLocalError when called with wrong rev (issue4106)

Queued for stable, thanks.
Simon Heimberg - Feb. 13, 2014, 11:41 p.m.
# HG changeset patch
# User Simon Heimberg <simohe@besonet.ch>
# Date 1392334460 -3600
# Node ID 0ed5bb67f3403b8e0655ee5ff2a7aeb32e307837
# Parent  0f1ef9e9e904c18f1ac96aef3a0e0d3aa5f1190c
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, that the function was called with wrong
revisions.

Lets assign None to the variable "base" and the merge mechanism raise an
error. The error message makes sense for the problem described in issue4106.
Raising an AssertionError instead would be an alternative, because the error
only happens when the function is called with wrong revisions.
Pierre-Yves David - Feb. 14, 2014, 12:16 a.m.
On 02/13/2014 03:41 PM, Simon Heimberg wrote:
> # HG changeset patch
> # User Simon Heimberg <simohe@besonet.ch>
> # Date 1392334460 -3600
> # Node ID 0ed5bb67f3403b8e0655ee5ff2a7aeb32e307837
> # Parent  0f1ef9e9e904c18f1ac96aef3a0e0d3aa5f1190c
> rebase: do not raise an UnboundLocalError when called with wrong rev (issue4106)

Is this something happening only when other extension call rebase directly?
Simon Heimberg - Feb. 14, 2014, 12:22 a.m.
Am 14.02.2014 01:16, schrieb Pierre-Yves David:
>
>
> On 02/13/2014 03:41 PM, Simon Heimberg wrote:
>> # HG changeset patch
>> # User Simon Heimberg <simohe@besonet.ch>
>> # Date 1392334460 -3600
>> # Node ID 0ed5bb67f3403b8e0655ee5ff2a7aeb32e307837
>> # Parent  0f1ef9e9e904c18f1ac96aef3a0e0d3aa5f1190c
>> rebase: do not raise an UnboundLocalError when called with wrong rev 
>> (issue4106)
>
> Is this something happening only when other extension call rebase 
> directly?
>

Exactly.
Simon Heimberg - Feb. 14, 2014, 12:33 a.m.
Am 14.02.2014 01:16, schrieb Pierre-Yves David:
>
>
> On 02/13/2014 03:41 PM, Simon Heimberg wrote:
>> # HG changeset patch
>> # User Simon Heimberg <simohe@besonet.ch>
>> # Date 1392334460 -3600
>> # Node ID 0ed5bb67f3403b8e0655ee5ff2a7aeb32e307837
>> # Parent  0f1ef9e9e904c18f1ac96aef3a0e0d3aa5f1190c
>> rebase: do not raise an UnboundLocalError when called with wrong rev 
>> (issue4106)
>
> Is this something happening only when other extension call rebase 
> directly?
>
Exactly.
Pierre-Yves David - Feb. 14, 2014, 12:35 a.m.
On 02/13/2014 04:22 PM, Simon Heimberg wrote:
> Am 14.02.2014 01:16, schrieb Pierre-Yves David:
>>
>>
>> On 02/13/2014 03:41 PM, Simon Heimberg wrote:
>>> # HG changeset patch
>>> # User Simon Heimberg <simohe@besonet.ch>
>>> # Date 1392334460 -3600
>>> # Node ID 0ed5bb67f3403b8e0655ee5ff2a7aeb32e307837
>>> # Parent  0f1ef9e9e904c18f1ac96aef3a0e0d3aa5f1190c
>>> rebase: do not raise an UnboundLocalError when called with wrong rev
>>> (issue4106)
>>
>> Is this something happening only when other extension call rebase
>> directly?
>>
>
> Exactly.

So reading you description, we keep crashing. Was will be the error 
message in that case?

I tempted to keep crashing early, an AssertionError or ProgrammingError 
would be fine.
Simon Heimberg - Feb. 14, 2014, 6:32 a.m.
Am 14.02.2014 01:35, schrieb Pierre-Yves David:
>
>
> On 02/13/2014 04:22 PM, Simon Heimberg wrote:
>> Am 14.02.2014 01:16, schrieb Pierre-Yves David:
>>>
>>>
>>> On 02/13/2014 03:41 PM, Simon Heimberg wrote:
>>>> # HG changeset patch
>>>> # User Simon Heimberg <simohe@besonet.ch>
>>>> # Date 1392334460 -3600
>>>> # Node ID 0ed5bb67f3403b8e0655ee5ff2a7aeb32e307837
>>>> # Parent  0f1ef9e9e904c18f1ac96aef3a0e0d3aa5f1190c
>>>> rebase: do not raise an UnboundLocalError when called with wrong rev
>>>> (issue4106)
>>>
>>> Is this something happening only when other extension call rebase
>>> directly?
>>>
>>
>> Exactly.
>
> So reading you description, we keep crashing. Was will be the error 
> message in that case?
No, in this case it aborts telling it can not rebase a changeset on itself.

I should maybe rewrite the commit message.

>
> I tempted to keep crashing early, an AssertionError or 
> ProgrammingError would be fine.
>
Maybe this would be better.

Shall I ask to unqueue? And what to choose then?
Pierre-Yves David - Feb. 14, 2014, 6:35 a.m.
On 02/13/2014 10:32 PM, Simon Heimberg wrote:
> Am 14.02.2014 01:35, schrieb Pierre-Yves David:
>>
>>
>> On 02/13/2014 04:22 PM, Simon Heimberg wrote:
>>> Am 14.02.2014 01:16, schrieb Pierre-Yves David:
>>>>
>>>>
>>>> On 02/13/2014 03:41 PM, Simon Heimberg wrote:
>>>>> # HG changeset patch
>>>>> # User Simon Heimberg <simohe@besonet.ch>
>>>>> # Date 1392334460 -3600
>>>>> # Node ID 0ed5bb67f3403b8e0655ee5ff2a7aeb32e307837
>>>>> # Parent  0f1ef9e9e904c18f1ac96aef3a0e0d3aa5f1190c
>>>>> rebase: do not raise an UnboundLocalError when called with wrong rev
>>>>> (issue4106)
>>>>
>>>> Is this something happening only when other extension call rebase
>>>> directly?
>>>>
>>>
>>> Exactly.
>>
>> So reading you description, we keep crashing. Was will be the error
>> message in that case?
> No, in this case it aborts telling it can not rebase a changeset on itself.
>
> I should maybe rewrite the commit message.

Yes, my first complain is that I can't decide if the change it good from 
this patch. Adding more details in the description should fix this.

If you patch touch output related things, you want the new output (and 
possible the old one) to be int he description.

>> I tempted to keep crashing early, an AssertionError or
>> ProgrammingError would be fine.
>>
> Maybe this would be better.
>
> Shall I ask to unqueue? And what to choose then?

I already talked to Matt on IRC, I believe it has been unqueued. We'll 
decide what to do next once we have your new commit message.

Patch

diff -r 0f1ef9e9e904 -r 0ed5bb67f340 hgext/rebase.py
--- a/hgext/rebase.py	Don Feb 06 15:56:25 2014 -0800
+++ b/hgext/rebase.py	Fre 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)
+            #
+            # Let the merge mechanism find the base itself or raise an error.
+            base = None
     if base is not None:
         repo.ui.debug("   detach base %d:%s\n" % (repo[base].rev(), repo[base]))
     # When collapsing in-place, the parent is the common ancestor, we