This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4634

transaction: make entries a private attribute (API)
ClosedPublic

Authored by indygreg on Sep 17 2018, 7:36 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG3d22aef3ecd5: transaction: make entries a private attribute (API)
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Sep 17 2018, 7:36 PM
Closed by commit rHG3d22aef3ecd5: transaction: make entries a private attribute (API) (authored by indygreg). · Explain WhySep 18 2018, 5:29 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/repair.py (10 lines)
Mmercurial/transaction.py (16 lines)

Diff 11160

mercurial/repair.py

# are already backed up and we did not touched the markers for the # are already backed up and we did not touched the markers for the
# saved changesets. # saved changesets.
tmpbundlefile = backupbundle(repo, savebases, saveheads, node, 'temp', tmpbundlefile = backupbundle(repo, savebases, saveheads, node, 'temp',
compress=False, obsolescence=False) compress=False, obsolescence=False)
with ui.uninterruptable(): with ui.uninterruptable():
try: try:
with repo.transaction("strip") as tr: 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() tr.startgroup()
cl.strip(striprev, tr) cl.strip(striprev, tr)
stripmanifest(repo, striprev, tr, files) stripmanifest(repo, striprev, tr, files)
for fn in files: for fn in files:
repo.file(fn).strip(striprev, tr) repo.file(fn).strip(striprev, tr)
tr.endgroup() tr.endgroup()
for i in pycompat.xrange(offset, len(tr.entries)): for i in pycompat.xrange(offset, len(tr._entries)):
file, troffset, ignore = tr.entries[i] file, troffset, ignore = tr._entries[i]
with repo.svfs(file, 'a', checkambig=True) as fp: with repo.svfs(file, 'a', checkambig=True) as fp:
fp.truncate(troffset) fp.truncate(troffset)
if troffset == 0: if troffset == 0:
repo.store.markremoved(file) repo.store.markremoved(file)
deleteobsmarkers(repo.obsstore, stripobsidx) deleteobsmarkers(repo.obsstore, stripobsidx)
del repo.obsstore del repo.obsstore
repo.invalidatevolatilesets() repo.invalidatevolatilesets()

mercurial/transaction.py

self._report = report self._report = report
# a vfs to the store content # a vfs to the store content
self._opener = opener self._opener = opener
# a map to access file in various {location -> vfs} # a map to access file in various {location -> vfs}
vfsmap = vfsmap.copy() vfsmap = vfsmap.copy()
vfsmap[''] = opener # set default value vfsmap[''] = opener # set default value
self._vfsmap = vfsmap self._vfsmap = vfsmap
self._after = after self._after = after
self.entries = [] self._entries = []
self._map = {} self._map = {}
self._journal = journalname self._journal = journalname
self._undoname = undoname self._undoname = undoname
self._queue = [] self._queue = []
# A callback to validate transaction content before closing it. # A callback to validate transaction content before closing it.
# should raise exception is anything is wrong. # should raise exception is anything is wrong.
# target user is repository hooks. # target user is repository hooks.
if validator is None: if validator is None:
return return
self._addentry(file, offset, data) self._addentry(file, offset, data)
def _addentry(self, file, offset, data): def _addentry(self, file, offset, data):
"""add a append-only entry to memory and on-disk state""" """add a append-only entry to memory and on-disk state"""
if file in self._map or file in self._backupmap: if file in self._map or file in self._backupmap:
return return
self.entries.append((file, offset, data)) self._entries.append((file, offset, data))
self._map[file] = len(self.entries) - 1 self._map[file] = len(self._entries) - 1
# add enough data to the journal to do the truncate # add enough data to the journal to do the truncate
self._file.write("%s\0%d\n" % (file, offset)) self._file.write("%s\0%d\n" % (file, offset))
self._file.flush() self._file.flush()
@active @active
def addbackup(self, file, hardlink=True, location=''): def addbackup(self, file, hardlink=True, location=''):
"""Adds a backup of the file to the transaction """Adds a backup of the file to the transaction
finally: finally:
for f in files: for f in files:
f.close() f.close()
return any return any
@active @active
def find(self, file): def find(self, file):
if file in self._map: if file in self._map:
return self.entries[self._map[file]] return self._entries[self._map[file]]
if file in self._backupmap: if file in self._backupmap:
return self._backupentries[self._backupmap[file]] return self._backupentries[self._backupmap[file]]
return None return None
@active @active
def replace(self, file, offset, data=None): def replace(self, file, offset, data=None):
''' '''
replace can only replace already committed entries replace can only replace already committed entries
that are not pending in the queue that are not pending in the queue
''' '''
if file not in self._map: if file not in self._map:
raise KeyError(file) raise KeyError(file)
index = self._map[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.write("%s\0%d\n" % (file, offset))
self._file.flush() self._file.flush()
@active @active
def nest(self, name=r'<unnamed>'): def nest(self, name=r'<unnamed>'):
self._count += 1 self._count += 1
self._usages += 1 self._usages += 1
self._names.append(name) self._names.append(name)
try: try:
vfs.unlink(b) vfs.unlink(b)
except (IOError, OSError, error.Abort) as inst: except (IOError, OSError, error.Abort) as inst:
if not c: if not c:
raise raise
# Abort may be raise by read only opener # Abort may be raise by read only opener
self._report("couldn't remove %s: %s\n" self._report("couldn't remove %s: %s\n"
% (vfs.join(b), inst)) % (vfs.join(b), inst))
self.entries = [] self._entries = []
self._writeundo() self._writeundo()
if self._after: if self._after:
self._after() self._after()
self._after = None # Help prevent cycles. self._after = None # Help prevent cycles.
if self._opener.isfile(self._backupjournal): if self._opener.isfile(self._backupjournal):
self._opener.unlink(self._backupjournal) self._opener.unlink(self._backupjournal)
if self._opener.isfile(self._journal): if self._opener.isfile(self._journal):
self._opener.unlink(self._journal) self._opener.unlink(self._journal)
def _abort(self): def _abort(self):
self._count = 0 self._count = 0
self._usages = 0 self._usages = 0
self._file.close() self._file.close()
self._backupsfile.close() self._backupsfile.close()
try: try:
if not self.entries and not self._backupentries: if not self._entries and not self._backupentries:
if self._backupjournal: if self._backupjournal:
self._opener.unlink(self._backupjournal) self._opener.unlink(self._backupjournal)
if self._journal: if self._journal:
self._opener.unlink(self._journal) self._opener.unlink(self._journal)
return return
self._report(_("transaction abort!\n")) self._report(_("transaction abort!\n"))
try: try:
for cat in sorted(self._abortcallback): for cat in sorted(self._abortcallback):
self._abortcallback[cat](self) self._abortcallback[cat](self)
# Prevent double usage and help clear cycles. # Prevent double usage and help clear cycles.
self._abortcallback = None self._abortcallback = None
_playback(self._journal, self._report, self._opener, _playback(self._journal, self._report, self._opener,
self._vfsmap, self.entries, self._backupentries, self._vfsmap, self._entries, self._backupentries,
False, checkambigfiles=self._checkambigfiles) False, checkambigfiles=self._checkambigfiles)
self._report(_("rollback completed\n")) self._report(_("rollback completed\n"))
except BaseException: except BaseException:
self._report(_("rollback failed - please run hg recover\n")) self._report(_("rollback failed - please run hg recover\n"))
finally: finally:
self._journal = None self._journal = None
self._releasefn(self, False) # notify failure of transaction self._releasefn(self, False) # notify failure of transaction
self._releasefn = None # Help prevent cycles. self._releasefn = None # Help prevent cycles.