Patchwork [5,of,6,V2] changelog: avoid redundant truncation of 00changelog.i at failure

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Dec. 16, 2015, 5:50 p.m.
Message ID <e979be8dbd88dec14bd9.1450288253@feefifofum>
Download mbox | patch
Permalink /patch/12073/
State Superseded
Headers show

Comments

Katsunori FUJIWARA - Dec. 16, 2015, 5:50 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1450287754 -32400
#      Thu Dec 17 02:42:34 2015 +0900
# Node ID e979be8dbd88dec14bd97729a9f837b6d91ba0ed
# Parent  2f1413cd29a754d41c65f14d162605fc24bdd45c
changelog: avoid redundant truncation of 00changelog.i at failure

Before this patch, '00changelog.i' is always truncated at transaction
failure, even though actual changes are written not into it but into
'00changelog.i.a' (aka "pending file").

This truncation changes behavior depending on timestamp of the file
(e.g. filecache). and may cause issues, which are difficult to
be certainly reproduced, (e.g. issue4876).

On the other hand, if once changes are written into '00changelog.i' by
'_finalize()', it should be truncated at failure.

To avoid such redundant truncation of '00changelog.i' at failure
correctly, this patch schedules invoking own '_avoidrollback()' at
failure by 'transaction.addabort()' in 'delayupdate()'.

It implies 'transaction._avoidrollback()' to avoid redundant
truncation of '00changelog.i' at failure

This patch introduces new status field '_finalized' to detect whether
truncation is needed or not, because combination of current internal
status fields '_divert' and '_delayed' can't distinguish cases below.

  - transaction is aborted before any change (truncation not needed)
  - changelog is finalized successfully (truncation needed)

Because changelog object is discarded from 'repo._filecache' at
failure of transaction since 0a7610758c42, changelog object is reused
only if previous transaction is closed successfully.

If changelog is reused, 'changelog._finalized' may be unintentionally
True at aborting transaction in the case below.

  1. tr.close()
  2. changelog._finalized = True
  3. transaction is closed successfully
  4. start next transaction
  5. transaction is aborted before any changelog.delayupdate()

But in this case, '_avoidrollback()' isn't registered via
'transaction.addabort()', and isn't executed at aborting. Therefore,
'changelog._finalized' value has no effect.

This is reason why resetting '_finalized' by False before
'transaction.addabort()' in 'delayupdate()' is enough for consistent
reusing changelog.

If changelog object in 'repo._filecache' isn't discarded as expected
at failure, this patch can reproduce problems like issue4876 more
frequently: in fact, "invalid changelog cache of repoview" problem,
which is fixed by previous patch, can be certainly reproduced by this
patch.

This patch adds timestamp examination to test-commandserver.t, to
confirm that '00changelog.i' isn't truncated if transaction is aborted
before finalization.

Patch

diff --git a/mercurial/changelog.py b/mercurial/changelog.py
--- a/mercurial/changelog.py
+++ b/mercurial/changelog.py
@@ -147,6 +147,7 @@  class changelog(revlog.revlog):
         self._delayed = False
         self._delaybuf = None
         self._divert = False
+        self._finalized = False
         self.filteredrevs = frozenset()
 
     def tip(self):
@@ -252,6 +253,8 @@  class changelog(revlog.revlog):
         self._delayed = True
         tr.addpending('cl-%i' % id(self), self._writepending)
         tr.addfinalize('cl-%i' % id(self), self._finalize)
+        self._finalized = False
+        tr.addabort('cl-%i' % id(self), self._avoidrollback)
 
     def _finalize(self, tr):
         "finalize index updates"
@@ -270,6 +273,7 @@  class changelog(revlog.revlog):
             fp.close()
             self._delaybuf = None
         self._divert = False
+        self._finalized = True
         # split when we're done
         self.checkinlinesize(tr)
 
@@ -318,6 +322,27 @@  class changelog(revlog.revlog):
 
         return False
 
+    def _avoidrollback(self, tr):
+        """Avoid redundant truncation of '00changelog.i' at failure
+
+        Without invocation of this function, '00changelog.i' is always
+        truncated at transaction failure, even though actual changes
+        are written not into it but into '00changelog.i.a'.
+
+        This truncation changes behavior depending on timestamp of
+        the file (e.g. filecache), and may cause issues, which are
+        difficult to be certainly reproduced.
+
+        On the other hand, if once changes are written into
+        '00changelog.i' (= finalized), it should be truncated at
+        failure.
+
+        This function is invoked as an abort hook of transaction and
+        avoids such truncation according to finalization status.
+        """
+        if not self._finalized:
+            tr._avoidrollback(self.indexfile)
+
     def checkinlinesize(self, tr, fp=None):
         if not self._delayed:
             revlog.revlog.checkinlinesize(self, tr, fp)
diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t
--- a/tests/test-commandserver.t
+++ b/tests/test-commandserver.t
@@ -756,6 +756,12 @@  cases.
   >     repo.__class__ = failrepo
   > EOF
 
+('timestampbase' is used to examine whether 00changelog.i is truncated
+at failure of transaction. This should be certainly newer than
+expected timestamp of 00changelog.i, because of "f --newer" specification)
+
+  $ touch -t 200001010001 $TESTTMP/timestampbase
+
   $ hg init repo3
   $ cd repo3
 
@@ -774,6 +780,8 @@  cases.
 
 (failuer before finalization)
 
+  $ touch -t 200001010000 .hg/store/00changelog.i
+
   >>> from hgclient import readchannel, runcommand, check
   >>> @check
   ... def abort(server):
@@ -791,6 +799,9 @@  cases.
   *** runcommand log
   *** runcommand verify -q
 
+  $ f --newer $TESTTMP/timestampbase .hg/store/00changelog.i
+  .hg/store/00changelog.i: older than $TESTTMP/timestampbase
+
 (failuer after finalization)
 
   >>> from hgclient import readchannel, runcommand, check
@@ -818,6 +829,8 @@  cases.
 
 (failure before finalization)
 
+  $ touch -t 200001010000 .hg/store/00changelog.i
+
   >>> from hgclient import readchannel, runcommand, check
   >>> @check
   ... def abort(server):
@@ -836,6 +849,9 @@  cases.
   0 bar (bar)
   *** runcommand verify -q
 
+  $ f --newer $TESTTMP/timestampbase .hg/store/00changelog.i
+  .hg/store/00changelog.i: older than $TESTTMP/timestampbase
+
 (failure after finalization)
 
   >>> from hgclient import readchannel, runcommand, check