This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3140

rebase: remove unnecessary and incorrect handling of nullid
ClosedPublic

Authored by martinvonz on Apr 5 2018, 1:46 PM.

Details

Reviewers
quark
pulkit
Group Reviewers
hg-reviewers
Commits
rHG953db9e00eeb: rebase: remove unnecessary and incorrect handling of nullid
Summary

We used to write a nullid as hex to the rebase state file and
interpret it as the "todo" state (value -1). However, when reading it,
we compared the string value to (binary) nullid, which would of course
not match. AFAICT, it still worked because when the read nodeid did
not match nullid (which, again, it didn't), we'd use the normal path
which did repo[<hex nullid>].rev(), and that also happens to return
-1. It seems to have been this way ever since 9972758ab4c5 (rebase:
handle revtodo as a special value when storing/restoring state,
2014-12-02).

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Apr 5 2018, 1:46 PM
quark accepted this revision.Apr 5 2018, 1:55 PM
quark added a subscriber: quark.

Hmm... maybe just remove it.

martinvonz edited the summary of this revision. (Show Details)Apr 5 2018, 2:02 PM
martinvonz retitled this revision from rebase: fix explicit handling of nullid in rebase state to rebase: remove unnecessary and incorrect handling of nullid.
martinvonz updated this revision to Diff 7742.
martinvonz added a comment.Apr 5 2018, 2:02 PM
In D3140#50376, @quark wrote:

Hmm... maybe just remove it.

Done.

pulkit accepted this revision.Apr 5 2018, 3:07 PM
Closed by commit rHG953db9e00eeb: rebase: remove unnecessary and incorrect handling of nullid (authored by martinvonz). · Explain WhyApr 5 2018, 3:17 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (3 lines)

Diff 7750

hgext/rebase.py

from __future__ import absolute_import from __future__ import absolute_import
import errno import errno
import os import os
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial.node import ( from mercurial.node import (
nullid,
nullrev, nullrev,
short, short,
) )
from mercurial import ( from mercurial import (
bookmarks, bookmarks,
cmdutil, cmdutil,
commands, commands,
copies, copies,
newrev = args[1] newrev = args[1]
if newrev in legacystates: if newrev in legacystates:
continue continue
if len(args) > 2: if len(args) > 2:
destrev = repo[args[2]].rev() destrev = repo[args[2]].rev()
else: else:
destrev = legacydest destrev = legacydest
destmap[oldrev] = destrev destmap[oldrev] = destrev
if newrev in (nullid, revtodostr): if newrev == revtodostr:
state[oldrev] = revtodo state[oldrev] = revtodo
# Legacy compat special case # Legacy compat special case
else: else:
state[oldrev] = repo[newrev].rev() state[oldrev] = repo[newrev].rev()
except IOError as err: except IOError as err:
if err.errno != errno.ENOENT: if err.errno != errno.ENOENT:
raise raise