| 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
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 >
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?
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
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
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.
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
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.
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,