( )⚙ D1340 dirstate: add explicit methods for modifying dirstate

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1340

dirstate: add explicit methods for modifying dirstate
ClosedPublic

Authored by mbthomas on Nov 8 2017, 12:31 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG853b7c41d19c: dirstate: add explicit methods for modifying dirstate
rHGd85f598b24e0: dirstate: add explicit methods for modifying dirstate
Summary

Instead of assigning dirstatetuple objects to entries in the dirstate, move
responsibility for creating tuples into the dirstatemap.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mbthomas created this revision.Nov 8 2017, 12:31 PM
mbolin added a subscriber: mbolin.Nov 9 2017, 8:00 PM
mbolin added inline comments.
mercurial/dirstate.py
1275

To avoid doing two lookups in self._map:

try:
    self._map.pop(f)
    return True
except KeyError:
    return False
durin42 requested changes to this revision.Nov 10 2017, 5:06 PM
durin42 added a subscriber: durin42.
durin42 added inline comments.
mercurial/dirstate.py
130

Why is ~all the interesting content of this docstring removed? Where is the meaning of the dirstate map documented other than this docstring?

1196

Per my comment above, let's insert a patch before this one that documents in more detail the API that dirstatemap is supposed to be providing, probably in a docstring on this class. Sound reasonable?

(Then the above docstring I complained about could point to this docstring.)

This revision now requires changes to proceed.Nov 10 2017, 5:06 PM
mbthomas added inline comments.Nov 11 2017, 7:59 AM
mercurial/dirstate.py
1196

Agreed. I removed the docstring above because it became a lie (we no longer implement __setitem__ or __delitem__, so it's not really a dict any more). I will add a docstring here on Monday.

mbthomas added inline comments.Nov 11 2017, 8:04 AM
mercurial/dirstate.py
1275

Good idea. I think return self._map.pop(f, None) is not None would avoid two lookups and also the exception. This function's going to be getting more complex in later diffs, so I'd like to avoid an exception handler if possible.

mbthomas updated this revision to Diff 3437.Nov 13 2017, 7:18 AM
durin42 accepted this revision.Nov 13 2017, 5:42 PM
This revision is now accepted and ready to land.Nov 13 2017, 5:42 PM
mbthomas updated this revision to Diff 3514.Nov 15 2017, 4:08 AM
durin42 accepted this revision.Nov 17 2017, 5:22 PM
Closed by commit rHGd85f598b24e0: dirstate: add explicit methods for modifying dirstate (authored by mbthomas). · Explain WhyNov 17 2017, 5:28 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/dirstate.py (44 lines)

Diff 3340

mercurial/dirstate.py

def pendingparentchange(self): def pendingparentchange(self):
'''Returns true if the dirstate is in the middle of a set of changes '''Returns true if the dirstate is in the middle of a set of changes
that modify the dirstate parent. that modify the dirstate parent.
''' '''
return self._parentwriters > 0 return self._parentwriters > 0
@propertycache @propertycache
def _map(self): def _map(self):
'''Return the dirstate contents as a map from filename to '''Returns the dirstate map.'''
durin42Unsubmitted
Not Done

Why is ~all the interesting content of this docstring removed? Where is the meaning of the dirstate map documented other than this docstring?

durin42: Why is ~all the interesting content of this docstring removed? Where is the meaning of the…
(state, mode, size, time).'''
self._map = dirstatemap(self._ui, self._opener, self._root) self._map = dirstatemap(self._ui, self._opener, self._root)
return self._map return self._map
@property @property
def _sparsematcher(self): def _sparsematcher(self):
"""The matcher for the sparse checkout. """The matcher for the sparse checkout.
The working directory may not include every file from a manifest. The The working directory may not include every file from a manifest. The
entry = self._map.get(d) entry = self._map.get(d)
if entry is not None and entry[0] != 'r': if entry is not None and entry[0] != 'r':
raise error.Abort( raise error.Abort(
_('file %r in dirstate clashes with %r') % (d, f)) _('file %r in dirstate clashes with %r') % (d, f))
if oldstate in "?r" and "dirs" in self._map.__dict__: if oldstate in "?r" and "dirs" in self._map.__dict__:
self._map.dirs.addpath(f) self._map.dirs.addpath(f)
self._dirty = True self._dirty = True
self._updatedfiles.add(f) self._updatedfiles.add(f)
self._map[f] = dirstatetuple(state, mode, size, mtime)
if state != 'n' or mtime == -1: if state != 'n' or mtime == -1:
self._map.nonnormalset.add(f) self._map.nonnormalset.add(f)
if size == -2: if size == -2:
self._map.otherparentset.add(f) self._map.otherparentset.add(f)
self._map.addfile(f, state, mode, size, mtime)
def normal(self, f): def normal(self, f):
'''Mark a file normal and clean.''' '''Mark a file normal and clean.'''
s = os.lstat(self._join(f)) s = os.lstat(self._join(f))
mtime = s.st_mtime mtime = s.st_mtime
self._addpath(f, 'n', s.st_mode, self._addpath(f, 'n', s.st_mode,
s.st_size & _rangemask, mtime & _rangemask) s.st_size & _rangemask, mtime & _rangemask)
self._map.copymap.pop(f, None) self._map.copymap.pop(f, None)
entry = self._map.get(f) entry = self._map.get(f)
if entry is not None: if entry is not None:
# backup the previous state # backup the previous state
if entry[0] == 'm': # merge if entry[0] == 'm': # merge
size = -1 size = -1
elif entry[0] == 'n' and entry[2] == -2: # other parent elif entry[0] == 'n' and entry[2] == -2: # other parent
size = -2 size = -2
self._map.otherparentset.add(f) self._map.otherparentset.add(f)
self._map[f] = dirstatetuple('r', 0, size, 0)
self._map.nonnormalset.add(f) self._map.nonnormalset.add(f)
self._map.removefile(f, size)
if size == 0: if size == 0:
self._map.copymap.pop(f, None) self._map.copymap.pop(f, None)
def merge(self, f): def merge(self, f):
'''Mark a file merged.''' '''Mark a file merged.'''
if self._pl[1] == nullid: if self._pl[1] == nullid:
return self.normallookup(f) return self.normallookup(f)
return self.otherparent(f) return self.otherparent(f)
def drop(self, f): def drop(self, f):
'''Drop a file from the dirstate''' '''Drop a file from the dirstate'''
if f in self._map: if self._map.dropfile(f):
self._dirty = True self._dirty = True
self._droppath(f) self._droppath(f)
del self._map[f]
if f in self._map.nonnormalset: if f in self._map.nonnormalset:
self._map.nonnormalset.remove(f) self._map.nonnormalset.remove(f)
self._map.copymap.pop(f, None) self._map.copymap.pop(f, None)
def _discoverpath(self, path, normed, ignoremissing, exists, storemap): def _discoverpath(self, path, normed, ignoremissing, exists, storemap):
if exists is None: if exists is None:
exists = os.path.lexists(os.path.join(self._root, path)) exists = os.path.lexists(os.path.join(self._root, path))
if not exists: if not exists:
# https://www.mercurial-scm.org/wiki/DirstateTransactionPlan # https://www.mercurial-scm.org/wiki/DirstateTransactionPlan
# emulate dropping timestamp in 'parsers.pack_dirstate' # emulate dropping timestamp in 'parsers.pack_dirstate'
now = _getfsnow(self._opener) now = _getfsnow(self._opener)
dmap = self._map dmap = self._map
for f in self._updatedfiles: for f in self._updatedfiles:
e = dmap.get(f) e = dmap.get(f)
if e is not None and e[0] == 'n' and e[3] == now: if e is not None and e[0] == 'n' and e[3] == now:
dmap[f] = dirstatetuple(e[0], e[1], e[2], -1) dmap.addfile(f, e[0], e[1], e[2], -1)
self._map.nonnormalset.add(f) self._map.nonnormalset.add(f)
# emulate that all 'dirstate.normal' results are written out # emulate that all 'dirstate.normal' results are written out
self._lastnormaltime = 0 self._lastnormaltime = 0
self._updatedfiles.clear() self._updatedfiles.clear()
# delay writing in-memory changes out # delay writing in-memory changes out
tr.addfilegenerator('dirstate', (self._filename,), tr.addfilegenerator('dirstate', (self._filename,),
o.unlink(backupname) o.unlink(backupname)
else: else:
o.rename(backupname, filename, checkambig=True) o.rename(backupname, filename, checkambig=True)
def clearbackup(self, tr, backupname): def clearbackup(self, tr, backupname):
'''Clear backup file''' '''Clear backup file'''
self._opener.unlink(backupname) self._opener.unlink(backupname)
class dirstatemap(object): class dirstatemap(object):
durin42Unsubmitted
Not Done

Per my comment above, let's insert a patch before this one that documents in more detail the API that dirstatemap is supposed to be providing, probably in a docstring on this class. Sound reasonable?

(Then the above docstring I complained about could point to this docstring.)

durin42: Per my comment above, let's insert a patch before this one that documents in more detail the…
mbthomasAuthorUnsubmitted
Not Done

Agreed. I removed the docstring above because it became a lie (we no longer implement __setitem__ or __delitem__, so it's not really a dict any more). I will add a docstring here on Monday.

mbthomas: Agreed. I removed the docstring above because it became a lie (we no longer implement…
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._parents = None self._parents = None
self._dirtyparents = False self._dirtyparents = False
return iter(self._map) return iter(self._map)
def get(self, key, default=None): def get(self, key, default=None):
return self._map.get(key, default) return self._map.get(key, default)
def __contains__(self, key): def __contains__(self, key):
return key in self._map return key in self._map
def __setitem__(self, key, value):
self._map[key] = value
def __getitem__(self, key): def __getitem__(self, key):
return self._map[key] return self._map[key]
def __delitem__(self, key):
del self._map[key]
def keys(self): def keys(self):
return self._map.keys() return self._map.keys()
def preload(self): def preload(self):
"""Loads the underlying data, if it's not already loaded""" """Loads the underlying data, if it's not already loaded"""
self._map self._map
def addfile(self, f, state, mode, size, mtime):
"""Add a tracked file to the dirstate."""
self._map[f] = dirstatetuple(state, mode, size, mtime)
def removefile(self, f, size):
"""
Mark a file as removed in the dirstate.
The `size` parameter is used to store sentinel values that indicate
the file's previous state. In the future, we should refactor this
to be more explicit about what that state is.
"""
self._map[f] = dirstatetuple('r', 0, size, 0)
def dropfile(self, f):
"""
Remove a file from the dirstate. Returns True if the file was
previously recorded.
"""
exists = f in self._map
mbolinUnsubmitted
Not Done

To avoid doing two lookups in self._map:

try:
    self._map.pop(f)
    return True
except KeyError:
    return False
mbolin: To avoid doing two lookups in `self._map`: ``` try: self._map.pop(f)…
mbthomasAuthorUnsubmitted
Not Done

Good idea. I think return self._map.pop(f, None) is not None would avoid two lookups and also the exception. This function's going to be getting more complex in later diffs, so I'd like to avoid an exception handler if possible.

mbthomas: Good idea. I think `return self._map.pop(f, None) is not None` would avoid two lookups and…
if exists:
del self._map[f]
return exists
def nonnormalentries(self): def nonnormalentries(self):
'''Compute the nonnormal dirstate entries from the dmap''' '''Compute the nonnormal dirstate entries from the dmap'''
try: try:
return parsers.nonnormalotherparententries(self._map) return parsers.nonnormalotherparententries(self._map)
except AttributeError: except AttributeError:
nonnorm = set() nonnorm = set()
otherparent = set() otherparent = set()
for fname, e in self._map.iteritems(): for fname, e in self._map.iteritems():
parse_dirstate = util.nogc(parsers.parse_dirstate) parse_dirstate = util.nogc(parsers.parse_dirstate)
p = parse_dirstate(self._map, self.copymap, st) p = parse_dirstate(self._map, self.copymap, st)
if not self._dirtyparents: if not self._dirtyparents:
self.setparents(*p) self.setparents(*p)
# Avoid excess attribute lookups by fast pathing certain checks # Avoid excess attribute lookups by fast pathing certain checks
self.__contains__ = self._map.__contains__ self.__contains__ = self._map.__contains__
self.__getitem__ = self._map.__getitem__ self.__getitem__ = self._map.__getitem__
self.__setitem__ = self._map.__setitem__
self.__delitem__ = self._map.__delitem__
self.get = self._map.get self.get = self._map.get
def write(self, st, now): def write(self, st, now):
st.write(parsers.pack_dirstate(self._map, self.copymap, st.write(parsers.pack_dirstate(self._map, self.copymap,
self.parents(), now)) self.parents(), now))
st.close() st.close()
self._dirtyparents = False self._dirtyparents = False
self.nonnormalset, self.otherparentset = self.nonnormalentries() self.nonnormalset, self.otherparentset = self.nonnormalentries()