# HG changeset patch # User Boris Feld # Date 2017-07-11 10:38:17 # Node ID 9bb4decd43b0c9fd0e1902ccaae550126fe415ce # Parent 456626e9c3d1b93cd27a0a7a46536b67cae29574 repovfs: add a ward to check if locks are properly taken When the appropriate developer warnings are enabled, We wrap 'repo.vfs.audit' to check for locks when accessing file in '.hg' for writing. Another changeset will add a 'ward' for the store vfs (svfs). This check system has caught a handful of locking issues that have been fixed in previous series (mostly in 4.0). I expect another batch to be caught in third party extensions. We introduce two real exceptions from extensions 'blackbox.log' (because a lot of read-only operations add entry to it), and 'last-email.txt' (because 'hg email' is currently a read only operation and there is value to keep it this way). In addition we are currently allowing bisect to operate outside of the lock because the current code is a bit hard to get properly locked for now. Multiple clean up have been made but there is still a couple of them to do and the freeze is coming. diff --git a/hgext/blackbox.py b/hgext/blackbox.py --- a/hgext/blackbox.py +++ b/hgext/blackbox.py @@ -234,6 +234,7 @@ def reposetup(ui, repo): if util.safehasattr(ui, 'setrepo'): ui.setrepo(repo) + repo._wlockfreeprefix.add('blackbox.log') @command('^blackbox', [('l', 'limit', 10, _('the number of events to show')), diff --git a/hgext/journal.py b/hgext/journal.py --- a/hgext/journal.py +++ b/hgext/journal.py @@ -69,6 +69,7 @@ def extsetup(ui): def reposetup(ui, repo): if repo.local(): repo.journal = journalstorage(repo) + repo._wlockfreeprefix.add('namejournal') dirstate, cached = localrepo.isfilecached(repo, 'dirstate') if cached: diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py --- a/hgext/patchbomb.py +++ b/hgext/patchbomb.py @@ -122,6 +122,10 @@ def uisetup(ui): cmdutil.extraexport.append('pullurl') cmdutil.extraexportmap['pullurl'] = _addpullheader +def reposetup(ui, repo): + if not repo.local(): + return + repo._wlockfreeprefix.add('last-email.txt') def prompt(ui, prompt, default=None, rest=':'): if default: diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -300,6 +300,26 @@ class localrepository(object): # only functions defined in module of enabled extensions are invoked featuresetupfuncs = set() + # list of prefix for file which can be written without 'wlock' + # Extensions should extend this list when needed + _wlockfreeprefix = { + # We migh consider requiring 'wlock' for the next + # two, but pretty much all the existing code assume + # wlock is not needed so we keep them excluded for + # now. + 'hgrc', + 'requires', + # XXX cache is a complicatged business someone + # should investigate this in depth at some point + 'cache/', + # XXX shouldn't be dirstate covered by the wlock? + 'dirstate', + # XXX bisect was still a bit too messy at the time + # this changeset was introduced. Someone should fix + # the remainig bit and drop this line + 'bisect.state', + } + def __init__(self, baseui, path, create=False): self.requirements = set() self.filtername = None @@ -319,10 +339,13 @@ class localrepository(object): self.auditor = pathutil.pathauditor(self.root, self._checknested) self.nofsauditor = pathutil.pathauditor(self.root, self._checknested, realfs=False) - self.vfs = vfsmod.vfs(self.path) self.baseui = baseui self.ui = baseui.copy() self.ui.copy = baseui.copy # prevent copying repo configuration + self.vfs = vfsmod.vfs(self.path) + if (self.ui.configbool('devel', 'all-warnings') or + self.ui.configbool('devel', 'check-locks')): + self.vfs.audit = self._getvfsward(self.vfs.audit) # A list of callback to shape the phase if no data were found. # Callback are in the form: func(repo, roots) --> processed root. # This list it to be filled by extension during repo setup @@ -441,6 +464,38 @@ class localrepository(object): # Signature to cached matcher instance. self._sparsematchercache = {} + def _getvfsward(self, origfunc): + """build a ward for self.vfs""" + rref = weakref.ref(self) + def checkvfs(path, mode=None): + ret = origfunc(path, mode=mode) + repo = rref() + if (repo is None + or not util.safehasattr(repo, '_wlockref') + or not util.safehasattr(repo, '_lockref')): + return + if mode in (None, 'r', 'rb'): + return + if path.startswith(repo.path): + # truncate name relative to the repository (.hg) + path = path[len(repo.path) + 1:] + if path.startswith('journal.'): + # journal is covered by 'lock' + if repo._currentlock(repo._lockref) is None: + repo.ui.develwarn('write with no lock: "%s"' % path, + stacklevel=2) + elif repo._currentlock(repo._wlockref) is None: + # rest of vfs files are covered by 'wlock' + # + # exclude special files + for prefix in self._wlockfreeprefix: + if path.startswith(prefix): + return + repo.ui.develwarn('write with no wlock: "%s"' % path, + stacklevel=2) + return ret + return checkvfs + def close(self): self._writecaches() diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t --- a/tests/test-devel-warnings.t +++ b/tests/test-devel-warnings.t @@ -44,6 +44,11 @@ > wl.release() > lo.release() > + > @command(b'no-wlock-write', [], '') + > def nowlockwrite(ui, repo): + > with repo.vfs(b'branch', 'a'): + > pass + > > @command(b'stripintr', [], '') > def stripintr(ui, repo): > lo = repo.lock() @@ -104,6 +109,11 @@ $ hg properlocking $ hg nowaitlocking +Writing without lock + + $ hg no-wlock-write + devel-warn: write with no wlock: "branch" at: $TESTTMP/buggylocking.py:* (nowlockwrite) (glob) + Stripping from a transaction $ echo a > a