D4634: transaction: make entries a private attribute (API)

[indygreg (Gregory Szorc)]

indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This attribute is tracking changes to append-only files. It is
  an implementation detail and should not be exposed as part of
  the public interface.
  
  But code in repair was accessing it, so it seemingly does belong
  as part of the public API. But that code in repair is making
  assumptions about how storage works and is grossly wrong when
  alternate storage backends are in play. We'll need some kind of
  "strip" API at the storage layer that knows how to handle things
  in a storage-agnostic manner. I don't think accessing a private
  attribute on the transaction is any worse than what this code
  is already doing. So I'm fine with violating the abstraction for
  transactions.
  
  And with this change, all per-instance attributes on transaction
  have been made private except for "changes" and "hookargs." Both
  are used by multiple consumers and look like they need to be
  part of the public interface.
  
  .. api::
  
    Various attributes of ``transaction.transaction`` are now ``_``
    prefixed to indicate they shouldn't be used by external
    consumers.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/repair.py
  mercurial/transaction.py

CHANGE DETAILS

diff --git a/mercurial/transaction.py b/mercurial/transaction.py
--- a/mercurial/transaction.py
+++ b/mercurial/transaction.py
@@ -129,7 +129,7 @@
         vfsmap[''] = opener  # set default value
         self._vfsmap = vfsmap
         self._after = after
-        self.entries = []
+        self._entries = []
         self._map = {}
         self._journal = journalname
         self._undoname = undoname
@@ -230,8 +230,8 @@
         """add a append-only entry to memory and on-disk state"""
         if file in self._map or file in self._backupmap:
             return
-        self.entries.append((file, offset, data))
-        self._map[file] = len(self.entries) - 1
+        self._entries.append((file, offset, data))
+        self._map[file] = len(self._entries) - 1
         # add enough data to the journal to do the truncate
         self._file.write("%s\0%d\n" % (file, offset))
         self._file.flush()
@@ -352,7 +352,7 @@
     @active
     def find(self, file):
         if file in self._map:
-            return self.entries[self._map[file]]
+            return self._entries[self._map[file]]
         if file in self._backupmap:
             return self._backupentries[self._backupmap[file]]
         return None
@@ -367,7 +367,7 @@
         if file not in self._map:
             raise KeyError(file)
         index = self._map[file]
-        self.entries[index] = (file, offset, data)
+        self._entries[index] = (file, offset, data)
         self._file.write("%s\0%d\n" % (file, offset))
         self._file.flush()
 
@@ -486,7 +486,7 @@
                     # Abort may be raise by read only opener
                     self._report("couldn't remove %s: %s\n"
                                  % (vfs.join(b), inst))
-        self.entries = []
+        self._entries = []
         self._writeundo()
         if self._after:
             self._after()
@@ -564,7 +564,7 @@
         self._backupsfile.close()
 
         try:
-            if not self.entries and not self._backupentries:
+            if not self._entries and not self._backupentries:
                 if self._backupjournal:
                     self._opener.unlink(self._backupjournal)
                 if self._journal:
@@ -579,7 +579,7 @@
                 # Prevent double usage and help clear cycles.
                 self._abortcallback = None
                 _playback(self._journal, self._report, self._opener,
-                          self._vfsmap, self.entries, self._backupentries,
+                          self._vfsmap, self._entries, self._backupentries,
                           False, checkambigfiles=self._checkambigfiles)
                 self._report(_("rollback completed\n"))
             except BaseException:
diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -190,7 +190,11 @@
     with ui.uninterruptable():
         try:
             with repo.transaction("strip") as tr:
-                offset = len(tr.entries)
+                # TODO this code violates the interface abstraction of the
+                # transaction and makes assumptions that file storage is
+                # using append-only files. We'll need some kind of storage
+                # API to handle stripping for us.
+                offset = len(tr._entries)
 
                 tr.startgroup()
                 cl.strip(striprev, tr)
@@ -200,8 +204,8 @@
                     repo.file(fn).strip(striprev, tr)
                 tr.endgroup()
 
-                for i in pycompat.xrange(offset, len(tr.entries)):
-                    file, troffset, ignore = tr.entries[i]
+                for i in pycompat.xrange(offset, len(tr._entries)):
+                    file, troffset, ignore = tr._entries[i]
                     with repo.svfs(file, 'a', checkambig=True) as fp:
                         fp.truncate(troffset)
                     if troffset == 0:



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