Patchwork [1,of,1,V3] rebase: do not raise an UnboundLocalError when called wrong (issue4106)

login
register
mail settings
Submitter Simon Heimberg
Date Feb. 20, 2014, 9:19 a.m.
Message ID <7433f2cb97e9ca90a720.1392887983@lapsi.heimberg.home>
Download mbox | patch
Permalink /patch/3721/
State Accepted
Commit 9155257e6330918c86c8cbeaddecbe5159f9f584
Headers show

Comments

Simon Heimberg - Feb. 20, 2014, 9:19 a.m.
# HG changeset patch
# User Simon Heimberg <simohe@besonet.ch>
# Date 1392334460 -3600
#      Fri Feb 14 00:34:20 2014 +0100
# Node ID 7433f2cb97e9ca90a72015180dc134b540245e06
# Parent  87e52e6425625ea4f7645cfe2fc491a21f9a6b51
rebase: do not raise an UnboundLocalError when called wrong (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 (probably)
called wrong.

An AssertionError is raised to highlight that the caller of the function did
something wrong.

An alternative approach is to only assign None to the variable "base" and let
the merge mechanism raise an abort message. This was the behaviour 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.
Augie Fackler - Feb. 28, 2014, 5:32 p.m.
On Thu, Feb 20, 2014 at 10:19:43AM +0100, Simon Heimberg wrote:
> # HG changeset patch
> # User Simon Heimberg <simohe@besonet.ch>
> # Date 1392334460 -3600
> #      Fri Feb 14 00:34:20 2014 +0100
> # Node ID 7433f2cb97e9ca90a72015180dc134b540245e06
> # Parent  87e52e6425625ea4f7645cfe2fc491a21f9a6b51
> rebase: do not raise an UnboundLocalError when called wrong (issue4106)

Queued, thanks. Seems like a reasonable fix, at least for now.

>
> 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 (probably)
> called wrong.
>
> An AssertionError is raised to highlight that the caller of the function did
> something wrong.
>
> An alternative approach is to only assign None to the variable "base" and let
> the merge mechanism raise an abort message. This was the behaviour 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.
>
> diff -r 87e52e642562 -r 7433f2cb97e9 hgext/rebase.py
> --- a/hgext/rebase.py	Wed Feb 19 16:46:47 2014 -0600
> +++ b/hgext/rebase.py	Fri Feb 14 00:34:20 2014 +0100
> @@ -516,6 +516,12 @@
>              if state.get(p.rev()) == repo[p1].rev():
>                  base = p.node()
>                  break
> +        else: # fallback when base not found
> +            base = None
> +
> +            # Raise because this function is called wrong (see issue 4106)
> +            raise AssertionError('no base found to rebase on '
> +                                 '(rebasenode 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
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff -r 87e52e642562 -r 7433f2cb97e9 hgext/rebase.py
--- a/hgext/rebase.py	Wed Feb 19 16:46:47 2014 -0600
+++ b/hgext/rebase.py	Fri Feb 14 00:34:20 2014 +0100
@@ -516,6 +516,12 @@ 
             if state.get(p.rev()) == repo[p1].rev():
                 base = p.node()
                 break
+        else: # fallback when base not found
+            base = None
+
+            # Raise because this function is called wrong (see issue 4106)
+            raise AssertionError('no base found to rebase on '
+                                 '(rebasenode 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