Submitter | Simon Heimberg |
---|---|
Date | Feb. 19, 2014, 10:55 a.m. |
Message ID | <2554f90793b6645be683.1392807341@lapsi.heimberg.home> |
Download | mbox | patch |
Permalink | /patch/3694/ |
State | Accepted |
Headers | show |
Comments
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
Patch
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') 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