This is an archive of the discontinued Mercurial Phabricator instance.

Differential D757

dirstate: move opendirstatefile to dirstatemap
ClosedPublic

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

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGc36c3fa7d35b: dirstate: move opendirstatefile to dirstatemap
rHG0407c611b7be: dirstate: move opendirstatefile to dirstatemap
Summary

As part of moving the dirstate storage logic to another class, let's move
opendirstatefile to dirstatemap. This will allow extensions to replace the
pending abstraction.

Future patches will move the consumers of _opendirstatefile into dirstatemap as
well.

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 accepted this revision.Sep 24 2017, 11:20 AM
indygreg added a subscriber: indygreg.
indygreg added inline comments.
mercurial/dirstate.py
1299–1304

Nit: I think ui should be the first argument.

1379–1380

This (pre-existing) error message is pretty bad because typical end users won't have a clue what it means or what to do if they see it.

This revision is now accepted and ready to land.Sep 24 2017, 11:20 AM
durham updated this revision to Diff 2081.Sep 26 2017, 6:58 AM
durham marked an inline comment as done.Sep 26 2017, 6:59 AM
durham added inline comments.
mercurial/dirstate.py
1379–1380

Agreed. I'm not even sure what it's actually saying as a mercurial developer...

Closed by commit rHG0407c611b7be: dirstate: move opendirstatefile to dirstatemap (authored by durham). · Explain WhySep 29 2017, 2:43 PM
This revision was automatically updated to reflect the committed changes.
foozy added a subscriber: foozy.Oct 1 2017, 5:30 AM
foozy added inline comments.
mercurial/dirstate.py
1379–1380

(sorry for late response)

With current implementation of dirstate, _pl() reads "dirstate"
file only for parents of wdir, and subsequent _read() will
read "dirstate" file again.

So, in this "changed parallelly" situation, 2 patterns are possible:

  1. previous _pl() read 'dirstate.pending', but current _read() reads 'dirstate'
  2. previous _pl() read 'dirstate', but current _read() reads 'dirstate.pending'

This should mean that:

  • current (spawned) "hg" process is running parallelly from another "hg" process, which causes current transaction, and
  • the latter process moves transaction processing forward without waiting for current "hg" process

For example:

  1. committing (or aborting) transaction between _pl() and _read() removed 'dirstate.pending' file
  2. changing "dirstate" in current (or next?) transaction newly creates 'dirstate.pending'

Spawning "hg" process from a hook process as background one can cause this.

Any hinting helps understanding ?

Revision Contents
Changeset List

PathPackages
Mmercurial/dirstate.py (40 lines)

Diff 2160

mercurial/dirstate.py

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
self._updatedfiles = set() self._updatedfiles = set()
# for consistent view between _pl() and _read() invocations
self._pendingmode = None
@contextlib.contextmanager @contextlib.contextmanager
def parentchange(self): def parentchange(self):
'''Context manager for handling dirstate parents. '''Context manager for handling dirstate parents.
If an exception occurs in the scope of the context manager, If an exception occurs in the scope of the context manager,
the incoherent dirstate won't be written when wlock is the incoherent dirstate won't be written when wlock is
released. released.
''' '''
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 @propertycache
def _pl(self): def _pl(self):
try: try:
fp = self._opendirstatefile() fp = self._map._opendirstatefile()
st = fp.read(40) st = fp.read(40)
fp.close() fp.close()
l = len(st) l = len(st)
if l == 40: if l == 40:
return st[:20], st[20:40] return st[:20], st[20:40]
elif l > 0 and l < 40: elif l > 0 and l < 40:
raise error.Abort(_('working directory state appears damaged!')) raise error.Abort(_('working directory state appears damaged!'))
except IOError as err: except IOError as err:
# replacing the underlying file # replacing the underlying file
ce = self._filecache['_branch'] ce = self._filecache['_branch']
if ce: if ce:
ce.refresh() ce.refresh()
except: # re-raises except: # re-raises
f.discard() f.discard()
raise raise
def _opendirstatefile(self):
fp, mode = txnutil.trypending(self._root, self._opener, self._filename)
if self._pendingmode is not None and self._pendingmode != mode:
fp.close()
raise error.Abort(_('working directory state may be '
'changed parallelly'))
self._pendingmode = mode
return fp
def _read(self): def _read(self):
self._map = dirstatemap() self._map = dirstatemap(self._ui, self._opener, self._root)
# ignore HG_PENDING because identity is used only for writing # ignore HG_PENDING because identity is used only for writing
self._identity = util.filestat.frompath( self._identity = util.filestat.frompath(
self._opener.join(self._filename)) self._opener.join(self._filename))
try: try:
fp = self._opendirstatefile() fp = self._map._opendirstatefile()
try: try:
st = fp.read() st = fp.read()
finally: finally:
fp.close() fp.close()
except IOError as err: except IOError as err:
if err.errno != errno.ENOENT: if err.errno != errno.ENOENT:
raise raise
return return
- version provided via command arguments - version provided via command arguments
''' '''
if self._checkcase: if self._checkcase:
return self._normalize(path, isknown, ignoremissing) return self._normalize(path, isknown, ignoremissing)
return path return path
def clear(self): def clear(self):
self._map = dirstatemap() 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._pl = [nullid, nullid]
self._lastnormaltime = 0 self._lastnormaltime = 0
self._updatedfiles.clear() self._updatedfiles.clear()
self._dirty = True self._dirty = True
filename = self._actualfilename(tr) filename = self._actualfilename(tr)
self._opener.rename(backupname, filename, checkambig=True) self._opener.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):
def __init__(self): def __init__(self, ui, opener, root):
self._ui = ui
self._opener = opener
self._root = root
self._filename = 'dirstate'
indygregUnsubmitted
Done

Nit: I think ui should be the first argument.

indygreg: Nit: I think `ui` should be the first argument.
self._map = {} self._map = {}
self.copymap = {} self.copymap = {}
# for consistent view between _pl() and _read() invocations
self._pendingmode = None
def iteritems(self): def iteritems(self):
return self._map.iteritems() return self._map.iteritems()
def __iter__(self): def __iter__(self):
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)
f['.'] = '.' # prevents useless util.fspath() invocation f['.'] = '.' # prevents useless util.fspath() invocation
return f return f
def dirs(self): def dirs(self):
"""Returns a set-like object containing all the directories in the """Returns a set-like object containing all the directories in the
current dirstate. current dirstate.
""" """
return util.dirs(self._map, 'r') return util.dirs(self._map, 'r')
def _opendirstatefile(self):
fp, mode = txnutil.trypending(self._root, self._opener, self._filename)
if self._pendingmode is not None and self._pendingmode != mode:
fp.close()
raise error.Abort(_('working directory state may be '
'changed parallelly'))
indygregUnsubmitted
Not Done

This (pre-existing) error message is pretty bad because typical end users won't have a clue what it means or what to do if they see it.

indygreg: This (pre-existing) error message is pretty bad because typical end users won't have a clue…
durhamAuthorUnsubmitted
Not Done

Agreed. I'm not even sure what it's actually saying as a mercurial developer...

durham: Agreed. I'm not even sure what it's actually saying as a mercurial developer...
foozyUnsubmitted
Not Done

(sorry for late response)

With current implementation of dirstate, _pl() reads "dirstate"
file only for parents of wdir, and subsequent _read() will
read "dirstate" file again.

So, in this "changed parallelly" situation, 2 patterns are possible:

  1. previous _pl() read 'dirstate.pending', but current _read() reads 'dirstate'
  2. previous _pl() read 'dirstate', but current _read() reads 'dirstate.pending'

This should mean that:

  • current (spawned) "hg" process is running parallelly from another "hg" process, which causes current transaction, and
  • the latter process moves transaction processing forward without waiting for current "hg" process

For example:

  1. committing (or aborting) transaction between _pl() and _read() removed 'dirstate.pending' file
  2. changing "dirstate" in current (or next?) transaction newly creates 'dirstate.pending'

Spawning "hg" process from a hook process as background one can cause this.

Any hinting helps understanding ?

foozy: (sorry for late response) With current implementation of dirstate, _pl() reads "dirstate" file…
self._pendingmode = mode
return fp