( )⚙ 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
1314

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–1235

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–1235

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
1314

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 (43 lines)

Diff 3514

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 (see documentation for dirstatemap).""" """Return the dirstate contents (see documentation for dirstatemap)."""
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…
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):
"""Map encapsulating the dirstate's contents. """Map encapsulating the dirstate's contents.
The dirstate contains the following state: The dirstate contains the following state:
- `identity` is the identity of the dirstate file, which can be used to - `identity` is the identity of the dirstate file, which can be used to
detect when changes have occurred to the dirstate file. detect when changes have occurred to the dirstate file.
- `parents` is a pair containing the parents of the working copy. The - `parents` is a pair containing the parents of the working copy. The
parents are updated by calling `setparents`. parents are updated by calling `setparents`.
- the state map maps filenames to tuples of (state, mode, size, mtime), - the state map maps filenames to tuples of (state, mode, size, mtime),
where state is a single character representing 'normal', 'added', where state is a single character representing 'normal', 'added',
'removed', or 'merged'. It is accessed by treating the dirstate as a 'removed', or 'merged'. It is read by treating the dirstate as a
dict. dict. File state is updated by calling the `addfile`, `removefile` and
`dropfile` methods.
- `copymap` maps destination filenames to their source filename. - `copymap` maps destination filenames to their source filename.
The dirstate also provides the following views onto the state: The dirstate also provides the following views onto the state:
- `nonnormalset` is a set of the filenames that have state other - `nonnormalset` is a set of the filenames that have state other
than 'normal', or are normal but have an mtime of -1 ('normallookup'). than 'normal', or are normal but have an mtime of -1 ('normallookup').
- `otherparentset` is a set of the filenames that are marked as coming - `otherparentset` is a set of the filenames that are marked as coming
from the second parent when the dirstate is currently being merged. from the second parent when the dirstate is currently being merged.
- `dirs` is a set-like object containing all the directories that contain - `dirs` is a set-like object containing all the directories that contain
files in the dirstate, excluding any files that are marked as removed. files in the dirstate, excluding any files that are marked as removed.
- `filefoldmap` is a dict mapping normalized filenames to the denormalized - `filefoldmap` is a dict mapping normalized filenames to the denormalized
form that they appear as in the dirstate. form that they appear as in the dirstate.
- `dirfoldmap` is a dict mapping normalized directory names to the - `dirfoldmap` is a dict mapping normalized directory names to the
denormalized form that they appear as in the dirstate. denormalized form that they appear as in the dirstate.
Once instantiated, the nonnormalset, otherparentset, dirs, filefoldmap and Once instantiated, the nonnormalset, otherparentset, dirs, filefoldmap and
dirfoldmap views must be maintained by the caller. dirfoldmap views must be maintained by the caller.
""" """
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.
"""
return self._map.pop(f, None) is not None
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…
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()