( )⚙ D1341 dirstate: move management of nonnormal sets into dirstate map

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1341

dirstate: move management of nonnormal sets into dirstate map
ClosedPublic

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

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG19b75779b7c3: dirstate: move management of nonnormal sets into dirstate map
rHGfd745bbf00c5: dirstate: move management of nonnormal sets into dirstate map
Summary

The dirstate map owns the nonnormal sets, and so should be the class to update
them. A future implementation of the dirstate will manage these maps
differently.

The action of clearing ambiguous times is now entirely controlled by the
dirstate map, so it moves there too.

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

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

I would prefer to see all mutations to self._map go through a common code path so that we can override this behavior easier in Eden.

As it stands, when this logic is conflated, it makes it much harder for us to safely subclass dirstatemap in Eden. For reference, here's what we're doing today:

https://github.com/facebookexperimental/eden-hg/blob/master/eden/hg/eden/eden_dirstate_map.py

durin42 added a subscriber: durin42.Nov 10 2017, 5:08 PM

I strongly suspect that (if you agree with my feedback on D1340) this will need some adjustments to document the expanded behavior of dirstatemap WRT nonnormalset (whatever a nonnormalset is)

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

I think what you want is a common tuple constructor hook point - eden_dirstate_map does build a different tuple, but it just sets it as a property of the map in the same way. I think we want to avoid another layer of indirection if we can.

Arguably dirstatetuple is already one of these, as it can be either a native tuple type, or the fast C-based one from parses.c, but maybe that's the wrong place for eden to hook as it's global to the module. Instead, we can do it by setting dirstatemap._tuplecls to dirstatetuple in the constructor, so the code here becomes more like:

self._map[f] = self._tuplecls(state, mode, size, mtime)

In eden you can make your own function and assign that to self._tuplecls in eden_dirstate_map.__init__.

mbthomas updated this revision to Diff 3438.Nov 13 2017, 7:18 AM
durin42 accepted this revision.Nov 13 2017, 5:43 PM
This revision is now accepted and ready to land.Nov 13 2017, 5:43 PM
mbthomas updated this revision to Diff 3515.Nov 15 2017, 4:08 AM
durin42 accepted this revision.Nov 17 2017, 5:22 PM
Closed by commit rHGfd745bbf00c5: dirstate: move management of nonnormal sets into dirstate map (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 (34 lines)

Diff 3614

mercurial/dirstate.py

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)
if state != 'n' or mtime == -1:
self._map.nonnormalset.add(f)
if size == -2:
self._map.otherparentset.add(f)
self._map.addfile(f, state, mode, size, mtime) 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)
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.nonnormalset.add(f)
self._map.removefile(f, size) 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 self._map.dropfile(f): if self._map.dropfile(f):
self._dirty = True self._dirty = True
self._droppath(f) self._droppath(f)
if f in self._map.nonnormalset:
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:
# Maybe a path component exists # Maybe a path component exists
if not ignoremissing and '/' in path: if not ignoremissing and '/' in path:
# 'dirstate.write()' is not only for writing in-memory # 'dirstate.write()' is not only for writing in-memory
# changes out, but also for dropping ambiguous timestamp. # changes out, but also for dropping ambiguous timestamp.
# delayed writing re-raise "ambiguous timestamp issue". # delayed writing re-raise "ambiguous timestamp issue".
# See also the wiki page below for detail: # See also the wiki page below for detail:
# 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 self._map.clearambiguoustimes(self._updatedfiles, now)
for f in self._updatedfiles:
e = dmap.get(f)
if e is not None and e[0] == 'n' and e[3] == now:
dmap.addfile(f, e[0], e[1], e[2], -1)
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,),
self._writedirstate, location='plain') self._writedirstate, location='plain')
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 dirs, filefoldmap and dirfoldmap views must be
dirfoldmap views must be maintained by the caller. maintained by the caller.
""" """
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'
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): def addfile(self, f, state, mode, size, mtime):
"""Add a tracked file to the dirstate.""" """Add a tracked file to the dirstate."""
self._map[f] = dirstatetuple(state, mode, size, mtime) self._map[f] = dirstatetuple(state, mode, size, mtime)
if state != 'n' or mtime == -1:
mbolinUnsubmitted
Not Done

I would prefer to see all mutations to self._map go through a common code path so that we can override this behavior easier in Eden.

As it stands, when this logic is conflated, it makes it much harder for us to safely subclass dirstatemap in Eden. For reference, here's what we're doing today:

https://github.com/facebookexperimental/eden-hg/blob/master/eden/hg/eden/eden_dirstate_map.py

mbolin: I would prefer to see all mutations to `self._map` go through a common code path so that we can…
mbthomasAuthorUnsubmitted
Not Done

I think what you want is a common tuple constructor hook point - eden_dirstate_map does build a different tuple, but it just sets it as a property of the map in the same way. I think we want to avoid another layer of indirection if we can.

Arguably dirstatetuple is already one of these, as it can be either a native tuple type, or the fast C-based one from parses.c, but maybe that's the wrong place for eden to hook as it's global to the module. Instead, we can do it by setting dirstatemap._tuplecls to dirstatetuple in the constructor, so the code here becomes more like:

self._map[f] = self._tuplecls(state, mode, size, mtime)

In eden you can make your own function and assign that to self._tuplecls in eden_dirstate_map.__init__.

mbthomas: I think what you want is a common tuple constructor hook point - eden_dirstate_map does build a…
self.nonnormalset.add(f)
if size == -2:
self.otherparentset.add(f)
def removefile(self, f, size): def removefile(self, f, size):
""" """
Mark a file as removed in the dirstate. Mark a file as removed in the dirstate.
The `size` parameter is used to store sentinel values that indicate The `size` parameter is used to store sentinel values that indicate
the file's previous state. In the future, we should refactor this the file's previous state. In the future, we should refactor this
to be more explicit about what that state is. to be more explicit about what that state is.
""" """
self._map[f] = dirstatetuple('r', 0, size, 0) self._map[f] = dirstatetuple('r', 0, size, 0)
self.nonnormalset.add(f)
def dropfile(self, f): def dropfile(self, f):
""" """
Remove a file from the dirstate. Returns True if the file was Remove a file from the dirstate. Returns True if the file was
previously recorded. previously recorded.
""" """
return self._map.pop(f, None) is not None exists = self._map.pop(f, None) is not None
self.nonnormalset.discard(f)
return exists
def clearambiguoustimes(self, files, now):
for f in files:
e = self.get(f)
if e is not None and e[0] == 'n' and e[3] == now:
self._map[f] = dirstatetuple(e[0], e[1], e[2], -1)
self.nonnormalset.add(f)
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()