D8191: nodemap: make sure on disk change get rolled back with the transaction

[marmoute (Pierre-Yves David)]

marmoute added a comment.
marmoute updated this revision to Diff 20686.


  rebase above latest default up to landed-D8182

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D8191?vs=20583&id=20686

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8191/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8191

AFFECTED FILES
  mercurial/configitems.py
  mercurial/revlogutils/nodemap.py
  tests/test-persistent-nodemap.t
  tests/testlib/crash_transaction_late.py

CHANGE DETAILS

diff --git a/tests/testlib/crash_transaction_late.py b/tests/testlib/crash_transaction_late.py
new file mode 100644
--- /dev/null
+++ b/tests/testlib/crash_transaction_late.py
@@ -0,0 +1,32 @@
+# tiny extension to abort a transaction very late during test
+#
+# Copyright 2020 Pierre-Yves David <pierre-yves.david at octobus.net>
+#
+# This software may be used and distributed according to the terms of the
+# GNU General Public License version 2 or any later version.
+
+from __future__ import absolute_import
+
+from mercurial import (
+    error,
+    transaction,
+)
+
+
+def abort(fp):
+    raise error.Abort(b"This is a late abort")
+
+
+def reposetup(ui, repo):
+
+    transaction.postfinalizegenerators.add(b'late-abort')
+
+    class LateAbortRepo(repo.__class__):
+        def transaction(self, *args, **kwargs):
+            tr = super(LateAbortRepo, self).transaction(*args, **kwargs)
+            tr.addfilegenerator(
+                b'late-abort', [b'late-abort'], abort, order=9999999
+            )
+            return tr
+
+    repo.__class__ = LateAbortRepo
diff --git a/tests/test-persistent-nodemap.t b/tests/test-persistent-nodemap.t
--- a/tests/test-persistent-nodemap.t
+++ b/tests/test-persistent-nodemap.t
@@ -353,3 +353,25 @@
 
   $ cat output.txt
 
+Check that a failing transaction will properly revert the data
+
+  $ echo plakfe > a
+  $ f --size --sha256 .hg/store/00changelog-*.nd
+  .hg/store/00changelog-????????????????.nd: size=123584, sha256=8c6cef6fd3d3fac291968793ee19a4be6d0b8375e9508bd5c7d4a8879e8df180 (glob) (pure !)
+  .hg/store/00changelog-????????????????.nd: size=123584, sha256=eb9e9a4bcafdb5e1344bc8a0cbb3288b2106413b8efae6265fb8a7973d7e97f9 (glob) (rust !)
+  .hg/store/00changelog-????????????????.nd: size=123136, sha256=4f504f5a834db3811ced50ab3e9e80bcae3581bb0f9b13a7a9f94b7fc34bcebe (glob) (no-pure no-rust !)
+  $ hg ci -m a3 --config "extensions.abort=$RUNTESTDIR/testlib/crash_transaction_late.py"
+  transaction abort!
+  rollback completed
+  abort: This is a late abort
+  [255]
+  $ hg debugnodemap --metadata
+  uid: ???????????????? (glob)
+  tip-rev: 5005
+  tip-node: bae4d45c759e30f1cb1a40e1382cf0e0414154db
+  data-length: 123584
+  data-unused: 448
+  $ f --size --sha256 .hg/store/00changelog-*.nd
+  .hg/store/00changelog-????????????????.nd: size=123584, sha256=8c6cef6fd3d3fac291968793ee19a4be6d0b8375e9508bd5c7d4a8879e8df180 (glob) (pure !)
+  .hg/store/00changelog-????????????????.nd: size=123584, sha256=eb9e9a4bcafdb5e1344bc8a0cbb3288b2106413b8efae6265fb8a7973d7e97f9 (glob) (rust !)
+  .hg/store/00changelog-????????????????.nd: size=123136, sha256=4f504f5a834db3811ced50ab3e9e80bcae3581bb0f9b13a7a9f94b7fc34bcebe (glob) (no-pure no-rust !)
diff --git a/mercurial/revlogutils/nodemap.py b/mercurial/revlogutils/nodemap.py
--- a/mercurial/revlogutils/nodemap.py
+++ b/mercurial/revlogutils/nodemap.py
@@ -93,6 +93,15 @@
     def addpostclose(self, callback_id, callback_func):
         self._postclose[callback_id] = callback_func
 
+    def registertmp(self, *args, **kwargs):
+        pass
+
+    def addbackup(self, *args, **kwargs):
+        pass
+
+    def add(self, *args, **kwargs):
+        pass
+
 
 def update_persistent_nodemap(revlog):
     """update the persistent nodemap right now
@@ -138,6 +147,7 @@
             # EXP-TODO: if this is a cache, this should use a cache vfs, not a
             # store vfs
             new_length = target_docket.data_length + len(data)
+            tr.add(datafile, target_docket.data_length)
             with revlog.opener(datafile, b'r+') as fd:
                 fd.seek(target_docket.data_length)
                 fd.write(data)
@@ -161,6 +171,7 @@
             data = persistent_data(revlog.index)
         # EXP-TODO: if this is a cache, this should use a cache vfs, not a
         # store vfs
+        tr.add(datafile, 0)
         with revlog.opener(datafile, b'w+') as fd:
             fd.write(data)
             if feed_data:
@@ -177,6 +188,10 @@
     file_path = revlog.nodemap_file
     if pending:
         file_path += b'.a'
+        tr.registertmp(file_path)
+    else:
+        tr.addbackup(file_path)
+
     with revlog.opener(file_path, b'w', atomictemp=True) as fp:
         fp.write(target_docket.serialize())
     revlog._nodemap_docket = target_docket
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -407,7 +407,6 @@
 )
 # TODO before getting `persistent-nodemap` out of experimental
 #
-# * code/tests around aborted transaction
 # * regenerate a new nodemap when the unused/total ration is to high
 # * decide for a "status" of the persistent nodemap and associated location
 #   - part of the store next the revlog itself (new requirements)



To: marmoute, #hg-reviewers
Cc: mercurial-devel