Patchwork [06,of,17] rebase: use nullrev instead of -1

login
register
mail settings
Submitter Mads Kiilerich
Date Nov. 30, 2014, 7:08 p.m.
Message ID <09fe6c1db24cc0c3fd3d.1417374513@localhost.localdomain>
Download mbox | patch
Permalink /patch/6909/
State Changes Requested
Headers show

Comments

Mads Kiilerich - Nov. 30, 2014, 7:08 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1417374421 -3600
#      Sun Nov 30 20:07:01 2014 +0100
# Node ID 09fe6c1db24cc0c3fd3dceae4063c0a8dcbd11a5
# Parent  0cff65a0d024e31284ff6eb804ed4ac65f628a6e
rebase: use nullrev instead of -1

We have a constant. Use it consistently.
Martin von Zweigbergk - Dec. 1, 2014, 6:59 a.m.
On Sun Nov 30 2014 at 11:09:31 AM Mads Kiilerich <mads@kiilerich.com> wrote:

> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1417374421 -3600
> #      Sun Nov 30 20:07:01 2014 +0100
> # Node ID 09fe6c1db24cc0c3fd3dceae4063c0a8dcbd11a5
> # Parent  0cff65a0d024e31284ff6eb804ed4ac65f628a6e
> rebase: use nullrev instead of -1
>
> We have a constant. Use it consistently.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -23,6 +23,9 @@ from mercurial.lock import release
>  from mercurial.i18n import _
>  import os, errno
>
> +# state values:
> +# >= 0 means already rebased to this rev
> +# nullrev = -1 means 'todo'
>  nullmerge = -2
>  revignored = -3
>
> @@ -373,7 +376,7 @@ def rebase(ui, repo, **opts):
>                  desc += ' (%s)' % ' '.join(l)
>              ui.status(_('rebasing %d:%s %s\n') % (rev, short(node), desc))
>              pos += 1
> -            if state[rev] == -1:
> +            if state[rev] == nullrev:
>                  ui.progress(_("rebasing"), pos, ("%d:%s" % (rev,
> short(node))),
>                              _('changesets'), total)
>                  p1, p2 = defineparents(repo, rev, target, state,
> @@ -852,7 +855,7 @@ def abort(repo, originalwd, target, stat
>              merge.update(repo, repo[originalwd].rev(), False, True, False)
>
>          # Strip from the first rebased revision
> -        rebased = filter(lambda x: x > -1 and x != target, state.values())
> +        rebased = filter(lambda x: x > nullrev and x != target,


The other two places compare exactly to nullrev with '==' or '!='. I prefer
that. We seem to have 55 occurrences of those and only 3 occurrences of '>'
or '<'.


> state.values())
>          if rebased:
>              strippoints = [c.node()  for c in repo.set('roots(%ld)',
> rebased)]
>              # no backup of rebased cset versions needed
> @@ -1031,7 +1034,7 @@ def summaryhook(ui, repo):
>          msg = _('rebase: (use "hg rebase --abort" to clear broken
> state)\n')
>          ui.write(msg)
>          return
> -    numrebased = len([i for i in state.itervalues() if i != -1])
> +    numrebased = len([i for i in state.itervalues() if i != nullrev])
>      # i18n: column positioning for "hg summary"
>      ui.write(_('rebase: %s, %s (rebase --continue)\n') %
>               (ui.label(_('%d rebased'), 'rebase.rebased') % numrebased,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Dec. 1, 2014, 7:13 a.m.
On 11/30/2014 11:08 AM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1417374421 -3600
> #      Sun Nov 30 20:07:01 2014 +0100
> # Node ID 09fe6c1db24cc0c3fd3dceae4063c0a8dcbd11a5
> # Parent  0cff65a0d024e31284ff6eb804ed4ac65f628a6e
> rebase: use nullrev instead of -1

irk!

Using constant is good. Using unrelated constant because they happen to 
have the same value is wrong.

Can you call this "todorev" instead?
Mads Kiilerich - Dec. 1, 2014, 1:24 p.m.
On 12/01/2014 08:13 AM, Pierre-Yves David wrote:
>
>
> On 11/30/2014 11:08 AM, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1417374421 -3600
>> #      Sun Nov 30 20:07:01 2014 +0100
>> # Node ID 09fe6c1db24cc0c3fd3dceae4063c0a8dcbd11a5
>> # Parent  0cff65a0d024e31284ff6eb804ed4ac65f628a6e
>> rebase: use nullrev instead of -1
>
> irk!
>
> Using constant is good. Using unrelated constant because they happen 
> to have the same value is wrong.

The code already use the nullrev constant:
http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l783
http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l810
http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l875

I do not disagree that it probably would be better to use a different 
constant (and preferably a different value). The scope if this change is 
however just consistent use of the existing, not to introduce something new.

/Mads
Mads Kiilerich - Dec. 1, 2014, 1:26 p.m.
On 12/01/2014 07:59 AM, Martin von Zweigbergk wrote:
> On Sun Nov 30 2014 at 11:09:31 AM Mads Kiilerich <mads@kiilerich.com 
> <mailto:mads@kiilerich.com>> wrote:
>
>     @@ -852,7 +855,7 @@ def abort(repo, originalwd, target, stat
>                  merge.update(repo, repo[originalwd].rev(), False,
>     True, False)
>
>              # Strip from the first rebased revision
>     -        rebased = filter(lambda x: x > -1 and x != target,
>     state.values())
>     +        rebased = filter(lambda x: x > nullrev and x != target, 
>
>
> The other two places compare exactly to nullrev with '==' or '!='. I 
> prefer that. We seem to have 55 occurrences of those and only 3 
> occurrences of '>' or '<'.

Yes. But the existing rebase code already use -1 and nullrev 
interchangeably and do depend on the value coming "just" before the 
first revision numbers. Here I'm not trying to rewrite the code to not 
depend on ordering of constants - just making it more clear what it 
depends on.

/Mads
Pierre-Yves David - Dec. 1, 2014, 1:36 p.m.
On 12/01/2014 05:24 AM, Mads Kiilerich wrote:
> On 12/01/2014 08:13 AM, Pierre-Yves David wrote:
>>
>>
>> On 11/30/2014 11:08 AM, Mads Kiilerich wrote:
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # Date 1417374421 -3600
>>> #      Sun Nov 30 20:07:01 2014 +0100
>>> # Node ID 09fe6c1db24cc0c3fd3dceae4063c0a8dcbd11a5
>>> # Parent  0cff65a0d024e31284ff6eb804ed4ac65f628a6e
>>> rebase: use nullrev instead of -1
>>
>> irk!
>>
>> Using constant is good. Using unrelated constant because they happen
>> to have the same value is wrong.
>
> The code already use the nullrev constant:
> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l783
> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l810
> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l875
>
> I do not disagree that it probably would be better to use a different
> constant (and preferably a different value). The scope if this change is
> however just consistent use of the existing, not to introduce something
> new.

-1 for moving in the wrong direction. Lets fix the constant name 
instead. The fix is super simple lets do it while it is in our mind.
Mads Kiilerich - Dec. 2, 2014, 4:24 a.m.
On 12/01/2014 02:36 PM, Pierre-Yves David wrote:
> On 12/01/2014 05:24 AM, Mads Kiilerich wrote:
>> On 12/01/2014 08:13 AM, Pierre-Yves David wrote:
>>> On 11/30/2014 11:08 AM, Mads Kiilerich wrote:
>>>> # HG changeset patch
>>>> # User Mads Kiilerich <madski@unity3d.com>
>>>> # Date 1417374421 -3600
>>>> #      Sun Nov 30 20:07:01 2014 +0100
>>>> # Node ID 09fe6c1db24cc0c3fd3dceae4063c0a8dcbd11a5
>>>> # Parent  0cff65a0d024e31284ff6eb804ed4ac65f628a6e
>>>> rebase: use nullrev instead of -1
>>>
>>> irk!
>>>
>>> Using constant is good. Using unrelated constant because they happen
>>> to have the same value is wrong.
>>
>> The code already use the nullrev constant:
>> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l783
>> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l810
>> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l875
>>
>> I do not disagree that it probably would be better to use a different
>> constant (and preferably a different value). The scope if this change is
>> however just consistent use of the existing, not to introduce something
>> new.
>
> -1 for moving in the wrong direction. Lets fix the constant name 
> instead. The fix is super simple lets do it while it is in our mind.

If you think it is super simple then go ahead.

Make sure you test that the new constant really is independent of 
nullrev and doesn't have to be -1.

/Mads
Pierre-Yves David - Dec. 2, 2014, 8:35 p.m.
On 12/01/2014 08:24 PM, Mads Kiilerich wrote:
> On 12/01/2014 02:36 PM, Pierre-Yves David wrote:
>> On 12/01/2014 05:24 AM, Mads Kiilerich wrote:
>>> On 12/01/2014 08:13 AM, Pierre-Yves David wrote:
>>>> On 11/30/2014 11:08 AM, Mads Kiilerich wrote:
>>>>> # HG changeset patch
>>>>> # User Mads Kiilerich <madski@unity3d.com>
>>>>> # Date 1417374421 -3600
>>>>> #      Sun Nov 30 20:07:01 2014 +0100
>>>>> # Node ID 09fe6c1db24cc0c3fd3dceae4063c0a8dcbd11a5
>>>>> # Parent  0cff65a0d024e31284ff6eb804ed4ac65f628a6e
>>>>> rebase: use nullrev instead of -1
>>>>
>>>> irk!
>>>>
>>>> Using constant is good. Using unrelated constant because they happen
>>>> to have the same value is wrong.
>>>
>>> The code already use the nullrev constant:
>>> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l783
>>> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l810
>>> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l875
>>>
>>> I do not disagree that it probably would be better to use a different
>>> constant (and preferably a different value). The scope if this change is
>>> however just consistent use of the existing, not to introduce something
>>> new.
>>
>> -1 for moving in the wrong direction. Lets fix the constant name
>> instead. The fix is super simple lets do it while it is in our mind.
>
> If you think it is super simple then go ahead.

Sure, now that I've been spending my time doing your work, I dunno when 
I'll have time to review your V2.

> Make sure you test that the new constant really is independent of
> nullrev and doesn't have to be -1.

Sure, this actually caught a bug (by pure luck) in the process.
Mads Kiilerich - Dec. 3, 2014, 1:17 a.m.
On 12/02/2014 09:35 PM, Pierre-Yves David wrote:
>
>
> On 12/01/2014 08:24 PM, Mads Kiilerich wrote:
>> On 12/01/2014 02:36 PM, Pierre-Yves David wrote:
>>> On 12/01/2014 05:24 AM, Mads Kiilerich wrote:
>>>> On 12/01/2014 08:13 AM, Pierre-Yves David wrote:
>>>>> On 11/30/2014 11:08 AM, Mads Kiilerich wrote:
>>>>>> # HG changeset patch
>>>>>> # User Mads Kiilerich <madski@unity3d.com>
>>>>>> # Date 1417374421 -3600
>>>>>> #      Sun Nov 30 20:07:01 2014 +0100
>>>>>> # Node ID 09fe6c1db24cc0c3fd3dceae4063c0a8dcbd11a5
>>>>>> # Parent  0cff65a0d024e31284ff6eb804ed4ac65f628a6e
>>>>>> rebase: use nullrev instead of -1
>>>>>
>>>>> irk!
>>>>>
>>>>> Using constant is good. Using unrelated constant because they happen
>>>>> to have the same value is wrong.
>>>>
>>>> The code already use the nullrev constant:
>>>> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l783
>>>> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l810
>>>> http://selenic.com/hg/file/b913c394386f/hgext/rebase.py#l875
>>>>
>>>> I do not disagree that it probably would be better to use a different
>>>> constant (and preferably a different value). The scope if this 
>>>> change is
>>>> however just consistent use of the existing, not to introduce 
>>>> something
>>>> new.
>>>
>>> -1 for moving in the wrong direction. Lets fix the constant name
>>> instead. The fix is super simple lets do it while it is in our mind.
>>
>> If you think it is super simple then go ahead.
>
> Sure, now that I've been spending my time doing your work

It was you who wanted to redefine the scope to use another constant than 
nullrev, not me.

>
>> Make sure you test that the new constant really is independent of
>> nullrev and doesn't have to be -1.
>
> Sure, this actually caught a bug (by pure luck) in the process.

I wouldn't call it luck. The debugging of this "something" was outside 
the scope of the cleanup I was trying to do and did not match my 
definition of "super simple".

/Mads

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -23,6 +23,9 @@  from mercurial.lock import release
 from mercurial.i18n import _
 import os, errno
 
+# state values:
+# >= 0 means already rebased to this rev
+# nullrev = -1 means 'todo'
 nullmerge = -2
 revignored = -3
 
@@ -373,7 +376,7 @@  def rebase(ui, repo, **opts):
                 desc += ' (%s)' % ' '.join(l)
             ui.status(_('rebasing %d:%s %s\n') % (rev, short(node), desc))
             pos += 1
-            if state[rev] == -1:
+            if state[rev] == nullrev:
                 ui.progress(_("rebasing"), pos, ("%d:%s" % (rev, short(node))),
                             _('changesets'), total)
                 p1, p2 = defineparents(repo, rev, target, state,
@@ -852,7 +855,7 @@  def abort(repo, originalwd, target, stat
             merge.update(repo, repo[originalwd].rev(), False, True, False)
 
         # Strip from the first rebased revision
-        rebased = filter(lambda x: x > -1 and x != target, state.values())
+        rebased = filter(lambda x: x > nullrev and x != target, state.values())
         if rebased:
             strippoints = [c.node()  for c in repo.set('roots(%ld)', rebased)]
             # no backup of rebased cset versions needed
@@ -1031,7 +1034,7 @@  def summaryhook(ui, repo):
         msg = _('rebase: (use "hg rebase --abort" to clear broken state)\n')
         ui.write(msg)
         return
-    numrebased = len([i for i in state.itervalues() if i != -1])
+    numrebased = len([i for i in state.itervalues() if i != nullrev])
     # i18n: column positioning for "hg summary"
     ui.write(_('rebase: %s, %s (rebase --continue)\n') %
              (ui.label(_('%d rebased'), 'rebase.rebased') % numrebased,