changeset 17126:8fa8717b47b6

obsolete: write obsolete marker inside a transaction

Marker are now written as soon as possible but within a transaction. Using a
transaction ensure a proper behavior on error and rollback compatibility.

Flush logic are not necessary anymore and are dropped from lock release.

With this changeset, the obsstore is open, written and closed for every single
added marker. This is expected to be highly inefficient and batched write should
be implemented "quickly".

Another issue is that every flush of the file will invalidate the obsstore
filecache and trigger a full re instantiation of the repo.obsstore attribute
(including, reading and parsing entry). This is also expected to be highly
inefficient and proper filecache operation should be implemented "quickly" too.

A side benefit of the filecache issue is that repo.obsstore  object is properly
invalidated on transaction abortion.

--- a/mercurial/commands.py	Wed Jul 04 02:02:48 2012 +0200
+++ b/mercurial/commands.py	Wed Jul 04 02:21:04 2012 +0200
@@ -2061,7 +2061,12 @@
         succs = tuple(bin(succ) for succ in successors)
         l = repo.lock()
         try:
-            repo.obsstore.create(bin(precursor), succs, 0, metadata)
+            tr = repo.transaction('debugobsolete')
+            try:
+                repo.obsstore.create(tr, bin(precursor), succs, 0, metadata)
+                tr.close()
+            finally:
+                tr.release()
         finally:
             l.release()
     else:

--- a/mercurial/localrepo.py	Wed Jul 04 02:02:48 2012 +0200
+++ b/mercurial/localrepo.py	Wed Jul 04 02:21:04 2012 +0200
@@ -987,8 +987,6 @@
             self.store.write()
             if '_phasecache' in vars(self):
                 self._phasecache.write()
-            if 'obsstore' in vars(self):
-                self.obsstore.flushmarkers()
             for k, ce in self._filecache.items():
                 if k == 'dirstate':
                     continue
@@ -1607,6 +1605,10 @@
         return r
 
     def pull(self, remote, heads=None, force=False):
+        # don't open transaction for nothing or you break future useful
+        # rollback call
+        tr = None
+        trname = 'pull\n' + util.hidepassword(remote.url())
         lock = self.lock()
         try:
             tmp = discovery.findcommonincoming(self, remote, heads=heads,
@@ -1617,6 +1619,7 @@
                 added = []
                 result = 0
             else:
+                tr = self.transaction(trname)
                 if heads is None and list(common) == [nullid]:
                     self.ui.status(_("requesting all changes\n"))
                 elif heads is None and remote.capable('changegroupsubset'):
@@ -1665,9 +1668,15 @@
 
             remoteobs = remote.listkeys('obsolete')
             if 'dump' in remoteobs:
+                if tr is None:
+                    tr = self.transaction(trname)
                 data = base85.b85decode(remoteobs['dump'])
-                self.obsstore.mergemarkers(data)
+                self.obsstore.mergemarkers(tr, data)
+            if tr is not None:
+                tr.close()
         finally:
+            if tr is not None:
+                tr.release()
             lock.release()
 
         return result

--- a/mercurial/obsolete.py	Wed Jul 04 02:02:48 2012 +0200
+++ b/mercurial/obsolete.py	Wed Jul 04 02:21:04 2012 +0200
@@ -159,7 +159,6 @@
     def __init__(self, sopener):
         self._all = []
         # new markers to serialize
-        self._new = []
         self.precursors = {}
         self.successors = {}
         self.sopener = sopener
@@ -174,7 +173,7 @@
     def __nonzero__(self):
         return bool(self._all)
 
-    def create(self, prec, succs=(), flag=0, metadata=None):
+    def create(self, transaction, prec, succs=(), flag=0, metadata=None):
         """obsolete: add a new obsolete marker
 
         * ensuring it is hashable
@@ -189,39 +188,33 @@
             if len(succ) != 20:
                 raise ValueError(succ)
         marker = (str(prec), tuple(succs), int(flag), encodemeta(metadata))
-        self.add(marker)
-
-    def add(self, marker):
-        """Add a new marker to the store
-
-        This marker still needs to be written to disk"""
-        self._new.append(marker)
-        self._load(marker)
+        self.add(transaction, marker)
 
-    def mergemarkers(self, data):
-        other = set(_readmarkers(data))
-        local = set(self._all)
-        new = other - local
-        for marker in new:
-            self.add(marker)
-
-    def flushmarkers(self):
-        """Write all markers on disk
-
-        After this operation, "new" markers are considered "known"."""
-        # XXX: transaction logic should be used
-        if self._new:
+    def add(self, transaction, marker):
+        """Add a new marker to the store"""
+        if marker not in self._all:
             f = self.sopener('obsstore', 'ab')
             try:
-                if f.tell() == 0:
-                    # plain new obsstore
+                offset = f.tell()
+                transaction.add('obsstore', offset)
+                if offset == 0:
+                    # new file add version header
                     f.write(_pack('>B', _fmversion))
-                _writemarkers(f.write, self._new)
+                _writemarkers(f.write, [marker])
+            finally:
+                # XXX: f.close() == filecache invalidation == obsstore rebuilt.
+                # call 'filecacheentry.refresh()'  here
                 f.close()
-                self._new[:] = []
-            except: # re-raises
-                f.discard()
-                raise
+            self._load(marker)
+
+    def mergemarkers(self, transation, data):
+        other = _readmarkers(data)
+        local = set(self._all)
+        new = [m for m in other if m not in local]
+        for marker in new:
+            # XXX: N marker == N x (open, write, close)
+            # we should write them all at once
+            self.add(transation, marker)
 
     def _load(self, marker):
         self._all.append(marker)
@@ -261,8 +254,13 @@
     data = base85.b85decode(new)
     lock = repo.lock()
     try:
-        repo.obsstore.mergemarkers(data)
-        return 1
+        tr = repo.transaction('pushkey: obsolete markers')
+        try:
+            repo.obsstore.mergemarkers(tr, data)
+            tr.close()
+            return 1
+        finally:
+            tr.release()
     finally:
         lock.release()
 

--- a/tests/test-obsolete.t	Wed Jul 04 02:02:48 2012 +0200
+++ b/tests/test-obsolete.t	Wed Jul 04 02:21:04 2012 +0200
@@ -158,9 +158,28 @@
   added 6 changesets with 6 changes to 6 files (+3 heads)
   (run 'hg heads' to see heads, 'hg merge' to merge)
   $ hg debugobsolete
+  245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
+  cdbce2fbb16313928851e97e0d85413f3f7eb77f ca819180edb99ed25ceafb3e9584ac287e240b00 0 {'date': '1337 0', 'user': 'test'}
   ca819180edb99ed25ceafb3e9584ac287e240b00 1337133713371337133713371337133713371337 0 {'date': '1338 0', 'user': 'test'}
+  1337133713371337133713371337133713371337 5601fb93a350734d935195fee37f4054c529ff39 0 {'date': '1339 0', 'user': 'test'}
+
+Rollback//Transaction support
+
+  $ hg debugobsolete -d '1340 0' aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+  $ hg debugobsolete
+  245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
   cdbce2fbb16313928851e97e0d85413f3f7eb77f ca819180edb99ed25ceafb3e9584ac287e240b00 0 {'date': '1337 0', 'user': 'test'}
+  ca819180edb99ed25ceafb3e9584ac287e240b00 1337133713371337133713371337133713371337 0 {'date': '1338 0', 'user': 'test'}
+  1337133713371337133713371337133713371337 5601fb93a350734d935195fee37f4054c529ff39 0 {'date': '1339 0', 'user': 'test'}
+  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb 0 {'date': '1340 0', 'user': 'test'}
+  $ hg rollback -n
+  repository tip rolled back to revision 5 (undo debugobsolete)
+  $ hg rollback
+  repository tip rolled back to revision 5 (undo debugobsolete)
+  $ hg debugobsolete
   245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
+  cdbce2fbb16313928851e97e0d85413f3f7eb77f ca819180edb99ed25ceafb3e9584ac287e240b00 0 {'date': '1337 0', 'user': 'test'}
+  ca819180edb99ed25ceafb3e9584ac287e240b00 1337133713371337133713371337133713371337 0 {'date': '1338 0', 'user': 'test'}
   1337133713371337133713371337133713371337 5601fb93a350734d935195fee37f4054c529ff39 0 {'date': '1339 0', 'user': 'test'}
 
   $ cd ..
@@ -176,9 +195,9 @@
   adding file changes
   added 6 changesets with 6 changes to 6 files (+3 heads)
   $ hg -R tmpd debugobsolete
-  ca819180edb99ed25ceafb3e9584ac287e240b00 1337133713371337133713371337133713371337 0 {'date': '1338 0', 'user': 'test'}
+  245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
   cdbce2fbb16313928851e97e0d85413f3f7eb77f ca819180edb99ed25ceafb3e9584ac287e240b00 0 {'date': '1337 0', 'user': 'test'}
-  245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
+  ca819180edb99ed25ceafb3e9584ac287e240b00 1337133713371337133713371337133713371337 0 {'date': '1338 0', 'user': 'test'}
   1337133713371337133713371337133713371337 5601fb93a350734d935195fee37f4054c529ff39 0 {'date': '1339 0', 'user': 'test'}
 
 
@@ -200,9 +219,9 @@
   (run 'hg heads' to see heads, 'hg merge' to merge)
   $ hg debugobsolete
   2448244824482448244824482448244824482448 1339133913391339133913391339133913391339 0 {'date': '1339 0', 'user': 'test'}
-  ca819180edb99ed25ceafb3e9584ac287e240b00 1337133713371337133713371337133713371337 0 {'date': '1338 0', 'user': 'test'}
+  245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
   cdbce2fbb16313928851e97e0d85413f3f7eb77f ca819180edb99ed25ceafb3e9584ac287e240b00 0 {'date': '1337 0', 'user': 'test'}
-  245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
+  ca819180edb99ed25ceafb3e9584ac287e240b00 1337133713371337133713371337133713371337 0 {'date': '1338 0', 'user': 'test'}
   1337133713371337133713371337133713371337 5601fb93a350734d935195fee37f4054c529ff39 0 {'date': '1339 0', 'user': 'test'}
 
 On push
@@ -213,8 +232,8 @@
   no changes found
   [1]
   $ hg -R ../tmpc debugobsolete
-  ca819180edb99ed25ceafb3e9584ac287e240b00 1337133713371337133713371337133713371337 0 {'date': '1338 0', 'user': 'test'}
+  245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
   cdbce2fbb16313928851e97e0d85413f3f7eb77f ca819180edb99ed25ceafb3e9584ac287e240b00 0 {'date': '1337 0', 'user': 'test'}
-  245bde4270cd1072a27757984f9cda8ba26f08ca cdbce2fbb16313928851e97e0d85413f3f7eb77f 0 {'date': '56 12', 'user': 'test'}
+  ca819180edb99ed25ceafb3e9584ac287e240b00 1337133713371337133713371337133713371337 0 {'date': '1338 0', 'user': 'test'}
   1337133713371337133713371337133713371337 5601fb93a350734d935195fee37f4054c529ff39 0 {'date': '1339 0', 'user': 'test'}
   2448244824482448244824482448244824482448 1339133913391339133913391339133913391339 0 {'date': '1339 0', 'user': 'test'}