This is an archive of the discontinued Mercurial Phabricator instance.

Differential D759

dirstate: move parents source of truth to dirstatemap
ClosedPublic

Authored by durham on Sep 20 2017, 8:03 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGec769bba34d3: dirstate: move parents source of truth to dirstatemap
rHG5fabba9b3d9c: dirstate: move parents source of truth to dirstatemap
Summary

As part of moving dirstate storage to its own class, let's move the source of
truth for the dirstate parents to dirstatemap. This requires that dirstate._pl
no longer be a cache, and that all sets go through dirstatemap.setparents.

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

durham created this revision.Sep 20 2017, 8:03 PM
indygreg requested changes to this revision.Sep 24 2017, 11:33 AM
indygreg added a subscriber: indygreg.

The logic here isn't readable enough that IMO we should fix it in flight.

mercurial/dirstate.py
1401–1402

This code doesn't read very cleanly.

The only cases where self._parents won't be set here are if the file exists but is empty or the file doesn't exist. I'd rather make that logic explicit and assert self._parents here.

Note that if the code were changed so an empty string represented both missing and empty files, the resulting code would be pretty clear via a switch statement against the content length.

This revision now requires changes to proceed.Sep 24 2017, 11:33 AM
durham updated this revision to Diff 2083.Sep 26 2017, 6:59 AM
durham marked an inline comment as done.Sep 26 2017, 7:00 AM
indygreg accepted this revision.Sep 29 2017, 2:05 PM
This revision is now accepted and ready to land.Sep 29 2017, 2:05 PM
Closed by commit rHG5fabba9b3d9c: dirstate: move parents source of truth to dirstatemap (authored by durham). · Explain WhySep 29 2017, 2:43 PM
This revision was automatically updated to reflect the committed changes.
mbolin added a subscriber: mbolin.Oct 7 2017, 8:09 PM
mbolin added inline comments.
mercurial/dirstate.py
320

Can/should parents(), p1(), and p2() here all be updated to go through map.parents()? We should be moving in the direction of removing _pl, no?

Revision Contents
Changeset List

PathPackages
Mmercurial/dirstate.py (55 lines)

Diff 2162

mercurial/dirstate.py

self._opener = opener self._opener = opener
self._validate = validate self._validate = validate
self._root = root self._root = root
self._sparsematchfn = sparsematchfn self._sparsematchfn = sparsematchfn
# ntpath.join(root, '') of Python 2.7.9 does not add sep if root is # ntpath.join(root, '') of Python 2.7.9 does not add sep if root is
# UNC path pointing to root share (issue4557) # UNC path pointing to root share (issue4557)
self._rootdir = pathutil.normasprefix(root) self._rootdir = pathutil.normasprefix(root)
self._dirty = False self._dirty = False
self._dirtypl = False
self._lastnormaltime = 0 self._lastnormaltime = 0
self._ui = ui self._ui = ui
self._filecache = {} self._filecache = {}
self._parentwriters = 0 self._parentwriters = 0
self._filename = 'dirstate' self._filename = 'dirstate'
self._pendingfilename = '%s.pending' % self._filename self._pendingfilename = '%s.pending' % self._filename
self._plchangecallbacks = {} self._plchangecallbacks = {}
self._origpl = None self._origpl = None
def _branch(self): def _branch(self):
try: try:
return self._opener.read("branch").strip() or "default" return self._opener.read("branch").strip() or "default"
except IOError as inst: except IOError as inst:
if inst.errno != errno.ENOENT: if inst.errno != errno.ENOENT:
raise raise
return "default" return "default"
@propertycache @property
def _pl(self): def _pl(self):
return self._map.parents() return self._map.parents()
@propertycache @propertycache
def _dirs(self): def _dirs(self):
return self._map.dirs() return self._map.dirs()
def dirs(self): def dirs(self):
def __iter__(self): def __iter__(self):
return iter(sorted(self._map)) return iter(sorted(self._map))
def items(self): def items(self):
return self._map.iteritems() return self._map.iteritems()
iteritems = items iteritems = items
def parents(self): def parents(self):
mbolinUnsubmitted
Not Done

Can/should parents(), p1(), and p2() here all be updated to go through map.parents()? We should be moving in the direction of removing _pl, no?

mbolin: Can/should `parents()`, `p1()`, and `p2()` here all be updated to go through `map.parents()`?
return [self._validate(p) for p in self._pl] return [self._validate(p) for p in self._pl]
def p1(self): def p1(self):
return self._validate(self._pl[0]) return self._validate(self._pl[0])
def p2(self): def p2(self):
return self._validate(self._pl[1]) return self._validate(self._pl[1])
def branch(self): def branch(self):
return encoding.tolocal(self._branch) return encoding.tolocal(self._branch)
def setparents(self, p1, p2=nullid): def setparents(self, p1, p2=nullid):
"""Set dirstate parents to p1 and p2. """Set dirstate parents to p1 and p2.
When moving from two parents to one, 'm' merged entries a When moving from two parents to one, 'm' merged entries a
adjusted to normal and previous copy records discarded and adjusted to normal and previous copy records discarded and
returned by the call. returned by the call.
See localrepo.setparents() See localrepo.setparents()
""" """
if self._parentwriters == 0: if self._parentwriters == 0:
raise ValueError("cannot set dirstate parent without " raise ValueError("cannot set dirstate parent without "
"calling dirstate.beginparentchange") "calling dirstate.beginparentchange")
self._dirty = self._dirtypl = True self._dirty = True
oldp2 = self._pl[1] oldp2 = self._pl[1]
if self._origpl is None: if self._origpl is None:
self._origpl = self._pl self._origpl = self._pl
self._pl = p1, p2 self._map.setparents(p1, p2)
copies = {} copies = {}
if oldp2 != nullid and p2 == nullid: if oldp2 != nullid and p2 == nullid:
candidatefiles = self._nonnormalset.union(self._otherparentset) candidatefiles = self._nonnormalset.union(self._otherparentset)
for f in candidatefiles: for f in candidatefiles:
s = self._map.get(f) s = self._map.get(f)
if s is None: if s is None:
continue continue
# into. This means that O(number of files) GCs are unavoidable. # into. This means that O(number of files) GCs are unavoidable.
# Depending on when in the process's lifetime the dirstate is parsed, # Depending on when in the process's lifetime the dirstate is parsed,
# this can get very expensive. As a workaround, disable GC while # this can get very expensive. As a workaround, disable GC while
# parsing the dirstate. # parsing the dirstate.
# #
# (we cannot decorate the function directly since it is in a C module) # (we cannot decorate the function directly since it is in a C module)
parse_dirstate = util.nogc(parsers.parse_dirstate) parse_dirstate = util.nogc(parsers.parse_dirstate)
p = parse_dirstate(self._map._map, self._map.copymap, st) p = parse_dirstate(self._map._map, self._map.copymap, st)
if not self._dirtypl: if not self._map._dirtyparents:
self._pl = p self._map.setparents(*p)
def invalidate(self): def invalidate(self):
'''Causes the next access to reread the dirstate. '''Causes the next access to reread the dirstate.
This is different from localrepo.invalidatedirstate() because it always This is different from localrepo.invalidatedirstate() because it always
rereads the dirstate. Use localrepo.invalidatedirstate() if you want to rereads the dirstate. Use localrepo.invalidatedirstate() if you want to
check whether the dirstate has changed before rereading it.''' check whether the dirstate has changed before rereading it.'''
for a in ("_map", "_identity", for a in ("_map", "_identity",
"_filefoldmap", "_dirfoldmap", "_branch", "_filefoldmap", "_dirfoldmap", "_branch",
"_pl", "_dirs", "_ignore", "_nonnormalset", "_dirs", "_ignore", "_nonnormalset",
"_otherparentset"): "_otherparentset"):
if a in self.__dict__: if a in self.__dict__:
delattr(self, a) delattr(self, a)
self._lastnormaltime = 0 self._lastnormaltime = 0
self._dirty = False self._dirty = False
self._updatedfiles.clear() self._updatedfiles.clear()
self._parentwriters = 0 self._parentwriters = 0
self._origpl = None self._origpl = None
return path return path
def clear(self): def clear(self):
self._map = dirstatemap(self._ui, self._opener, self._root) self._map = dirstatemap(self._ui, self._opener, self._root)
self._nonnormalset = set() self._nonnormalset = set()
self._otherparentset = set() self._otherparentset = set()
if "_dirs" in self.__dict__: if "_dirs" in self.__dict__:
delattr(self, "_dirs") delattr(self, "_dirs")
self._pl = [nullid, nullid] self._map.setparents(nullid, nullid)
self._lastnormaltime = 0 self._lastnormaltime = 0
self._updatedfiles.clear() self._updatedfiles.clear()
self._dirty = True self._dirty = True
def rebuild(self, parent, allfiles, changedfiles=None): def rebuild(self, parent, allfiles, changedfiles=None):
if changedfiles is None: if changedfiles is None:
# Rebuild entire dirstate # Rebuild entire dirstate
changedfiles = allfiles changedfiles = allfiles
lastnormaltime = self._lastnormaltime lastnormaltime = self._lastnormaltime
self.clear() self.clear()
self._lastnormaltime = lastnormaltime self._lastnormaltime = lastnormaltime
if self._origpl is None: if self._origpl is None:
self._origpl = self._pl self._origpl = self._pl
self._pl = (parent, nullid) self._map.setparents(parent, nullid)
for f in changedfiles: for f in changedfiles:
if f in allfiles: if f in allfiles:
self.normallookup(f) self.normallookup(f)
else: else:
self.drop(f) self.drop(f)
self._dirty = True self._dirty = True
now = end # trust our estimate that the end is near now now = end # trust our estimate that the end is near now
break break
st.write(parsers.pack_dirstate(self._map._map, self._map.copymap, st.write(parsers.pack_dirstate(self._map._map, self._map.copymap,
self._pl, now)) self._pl, now))
self._nonnormalset, self._otherparentset = self._map.nonnormalentries() self._nonnormalset, self._otherparentset = self._map.nonnormalentries()
st.close() st.close()
self._lastnormaltime = 0 self._lastnormaltime = 0
self._dirty = self._dirtypl = False self._dirty = self._map._dirtyparents = False
def _dirignore(self, f): def _dirignore(self, f):
if f == '.': if f == '.':
return False return False
if self._ignore(f): if self._ignore(f):
return True return True
for p in util.finddirs(f): for p in util.finddirs(f):
if self._ignore(p): if self._ignore(p):
def __init__(self, ui, opener, root): def __init__(self, ui, opener, root):
self._ui = ui self._ui = ui
self._opener = opener self._opener = opener
self._root = root self._root = root
self._filename = 'dirstate' self._filename = 'dirstate'
self._map = {} self._map = {}
self.copymap = {} self.copymap = {}
self._parents = None
self._dirtyparents = False
# for consistent view between _pl() and _read() invocations # for consistent view between _pl() and _read() invocations
self._pendingmode = None self._pendingmode = None
def iteritems(self): def iteritems(self):
return self._map.iteritems() return self._map.iteritems()
def __iter__(self): def __iter__(self):
if self._pendingmode is not None and self._pendingmode != mode: if self._pendingmode is not None and self._pendingmode != mode:
fp.close() fp.close()
raise error.Abort(_('working directory state may be ' raise error.Abort(_('working directory state may be '
'changed parallelly')) 'changed parallelly'))
self._pendingmode = mode self._pendingmode = mode
return fp return fp
def parents(self): def parents(self):
if not self._parents:
try: try:
fp = self._opendirstatefile() fp = self._opendirstatefile()
st = fp.read(40) st = fp.read(40)
fp.close() fp.close()
l = len(st)
if l == 40:
return st[:20], st[20:40]
elif l > 0 and l < 40:
raise error.Abort(_('working directory state appears damaged!'))
except IOError as err: except IOError as err:
if err.errno != errno.ENOENT: if err.errno != errno.ENOENT:
raise raise
return [nullid, nullid] # File doesn't exist, so the current state is empty
st = ''
l = len(st)
if l == 40:
self._parents = st[:20], st[20:40]
elif l == 0:
self._parents = [nullid, nullid]
else:
raise error.Abort(_('working directory state appears '
'damaged!'))
return self._parents
def setparents(self, p1, p2):
self._parents = (p1, p2)
self._dirtyparents = True
indygregUnsubmitted
Done

This code doesn't read very cleanly.

The only cases where self._parents won't be set here are if the file exists but is empty or the file doesn't exist. I'd rather make that logic explicit and assert self._parents here.

Note that if the code were changed so an empty string represented both missing and empty files, the resulting code would be pretty clear via a switch statement against the content length.

indygreg: This code doesn't read very cleanly. The only cases where `self._parents` won't be set here…