Patchwork [2,of,2] localrepo: refresh filecache stats only if transaction finished successfully

login
register
mail settings
Submitter Yuya Nishihara
Date Sept. 15, 2015, 1:55 p.m.
Message ID <a2c7d936a237ea7548b7.1442325304@mimosa>
Download mbox | patch
Permalink /patch/10509/
State Accepted
Headers show

Comments

Yuya Nishihara - Sept. 15, 2015, 1:55 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1442244759 -32400
#      Tue Sep 15 00:32:39 2015 +0900
# Node ID a2c7d936a237ea7548b7fc807fa6227bddde80fa
# Parent  02d72254dc69b88c60c59234e447572ae63ed41e
localrepo: refresh filecache stats only if transaction finished successfully

If commit is aborted by pretxncommit hook, in-memory changelog and manifest
have entries that would be added. So they must be discarded on invalidate().

But the mechanism introduced by a710936c3037 doesn't handle this case well.
It tries to mitigate the penalty of invalidate() by marking in-memory cache
as "clean" on unlock assuming that they are identical to the stored data.
But this assumption is wrong if stored data are rolled back by tr.abort().

This patch moves the hook to post-close action so that it will never be
triggered on abort.

This bug was originally reported to thg, which is only reproducible in
command-server process on unix, evolve disabled.

https://bitbucket.org/tortoisehg/thg/issues/4285/
Pierre-Yves David - Sept. 15, 2015, 8:35 p.m.
On 09/15/2015 06:55 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1442244759 -32400
> #      Tue Sep 15 00:32:39 2015 +0900
> # Node ID a2c7d936a237ea7548b7fc807fa6227bddde80fa
> # Parent  02d72254dc69b88c60c59234e447572ae63ed41e
> localrepo: refresh filecache stats only if transaction finished successfully

pretty nice catch, pushed to the clowncopter.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1021,6 +1021,9 @@  class localrepository(object):
             reporef().hook('txnabort', throw=False, txnname=desc,
                            **tr2.hookargs)
         tr.addabort('txnabort-hook', txnaborthook)
+        # avoid eager cache invalidation. in-memory data should be identical
+        # to stored data if transaction has no error.
+        tr.addpostclose('refresh-filecachestats', self._refreshfilecachestats)
         self._transref = weakref.ref(tr)
         return tr
 
@@ -1198,7 +1201,7 @@  class localrepository(object):
         self.invalidate()
         self.invalidatedirstate()
 
-    def _refreshfilecachestats(self):
+    def _refreshfilecachestats(self, tr):
         '''Reload stats of cached files so that they are flagged as valid'''
         for k, ce in self._filecache.items():
             if k == 'dirstate' or k not in self.__dict__:
@@ -1247,7 +1250,7 @@  class localrepository(object):
             l.lock()
             return l
 
-        l = self._lock(self.svfs, "lock", wait, self._refreshfilecachestats,
+        l = self._lock(self.svfs, "lock", wait, None,
                        self.invalidate, _('repository %s') % self.origroot)
         self._lockref = weakref.ref(l)
         return l
diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t
--- a/tests/test-commandserver.t
+++ b/tests/test-commandserver.t
@@ -415,6 +415,30 @@  check that local configs for the cached 
   *** runcommand branches
   default                        1:731265503d86
 
+in-memory cache must be reloaded if transaction is aborted. otherwise
+changelog and manifest would have invalid node:
+
+  $ echo a >> a
+  >>> from hgclient import readchannel, runcommand, check
+  >>> @check
+  ... def txabort(server):
+  ...     readchannel(server)
+  ...     runcommand(server, ['commit', '--config', 'hooks.pretxncommit=false',
+  ...                         '-mfoo'])
+  ...     runcommand(server, ['verify'])
+  *** runcommand commit --config hooks.pretxncommit=false -mfoo
+  transaction abort!
+  rollback completed
+  abort: pretxncommit hook exited with status 1
+   [255]
+  *** runcommand verify
+  checking changesets
+  checking manifests
+  crosschecking files in changesets and manifests
+  checking files
+  1 files, 2 changesets, 2 total revisions
+  $ hg revert --no-backup -aq
+
   $ cat >> .hg/hgrc << EOF
   > [experimental]
   > evolution=createmarkers