This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4803

storageutil: extract most of emitrevisions() to standalone function
ClosedPublic

Authored by indygreg on Sep 28 2018, 8:18 PM.

Download Raw Diff

Details

Reviewers

durin42

Group Reviewers

hg-reviewers

Commits

rHG842ffcf1d42f: storageutil: extract most of emitrevisions() to standalone function

Summary

As part of implementing a storage backend, I found myself copying
most of revlog.emitrevisions(). This code is highly nuanced and it
bothered me greatly to be copying such low-level code.

This commit extracts the bulk of revlog.emitrevisions() into a
new standalone function. In order to make the function generally
usable, all "self" function calls that aren't exposed on the
ifilestorage interface are passed in via callable arguments.

No meaningful behavior should have changed as part of the port.

Upcoming commits will tweak behavior to make the code more
generically usable.

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 28 2018, 8:18 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptSep 28 2018, 8:18 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

indygreg added a child revision: D4804: storageutil: make all callables optional.Sep 28 2018, 8:18 PM

indygreg mentioned this in D4782: remotefilelog: import pruned-down remotefilelog extension from hg-experimental.Oct 1 2018, 6:53 PM

durin42 requested changes to this revision.Oct 3 2018, 10:47 AM

durin42 added a subscriber: durin42.

durin42 added inline comments.

mercurial/utils/storageutil.py
266	This is a huge step backwards for remotefilelog and friends: it's all defined in terms of revs, whereas emitrevisions (as copypasta-inducing as it was) was in terms of nodes. We need to do better.

This revision now requires changes to proceed.Oct 3 2018, 10:47 AM

Oh, I just got to D4805. I think the helper is still of somewhat limited utility and truly novel storage engines will end up totally reimplementing emitrevisions, but that's fine.

Closed by commit rHG842ffcf1d42f: storageutil: extract most of emitrevisions() to standalone function (authored by indygreg). · Explain WhyOct 3 2018, 11:05 AM

This revision was automatically updated to reflect the committed changes.

FWIW I would like to rewrite all of filelog (and this API) to be in terms of nodes. My hands are somewhat tied with this function due to how all the functions are implemented in terms of revs today. We'll get there eventually...

Revision Contents
Changeset List

			Path	Packages
M			mercurial/revlog.py (106 lines)
M			mercurial/utils/storageutil.py (146 lines)

Status	Author	Revision
Closed	indygreg	D4805 storageutil: pass nodes into emitrevisions()
Closed	indygreg	D4804 storageutil: make all callables optional
Closed	indygreg	D4803 storageutil: extract most of emitrevisions() to standalone function
Closed	indygreg	D4802 storageutil: invert logic of file data comparison
Closed	indygreg	D4801 storageutil: extract filelog.cmp() to a standalone function
Closed	indygreg	D4800 storageutil: extract copy metadata retrieval out of filelog
Closed	indygreg	D4799 storageutil: extract functionality for resolving strip revisions
Closed	indygreg	D4798 storageutil: consistently raise LookupError (API)
Closed	indygreg	D4797 storageutil: implement file identifier resolution method (BC)
Closed	indygreg	D4796 testing: add more testing for ifileindex.lookup()
Closed	indygreg	D4795 dagop: extract DAG local heads functionality from revlog
Closed	indygreg	D4794 dagop: extract descendants() from revlog module
Closed	indygreg	D4793 filelog: remove checkhash() (API)
Closed	indygreg	D4792 filelog: remove revdiff() (API)
Closed	indygreg	D4791 localrepo: define storage backend in creation options (API)
Closed	indygreg	D4790 wireprotov2: derive "required" from presence of default value

Diff 11617

mercurial/revlog.py

	if nodesorder not in ('nodes', 'storage', None):			if nodesorder not in ('nodes', 'storage', None):
	raise error.ProgrammingError('unhandled value for nodesorder: %s' %			raise error.ProgrammingError('unhandled value for nodesorder: %s' %
	nodesorder)			nodesorder)

	if nodesorder is None and not self._generaldelta:			if nodesorder is None and not self._generaldelta:
	nodesorder = 'storage'			nodesorder = 'storage'

	frev = self.rev			frev = self.rev
	fnode = self.node

	if nodesorder == 'nodes':			if nodesorder == 'nodes':
	revs = [frev(n) for n in nodes]			revs = [frev(n) for n in nodes]
	elif nodesorder == 'storage':			elif nodesorder == 'storage':
	revs = sorted(frev(n) for n in nodes)			revs = sorted(frev(n) for n in nodes)
	else:			else:
	assert self._generaldelta			assert self._generaldelta
	revs = set(frev(n) for n in nodes)			revs = set(frev(n) for n in nodes)
	revs = dagop.linearize(revs, self.parentrevs)			revs = dagop.linearize(revs, self.parentrevs)

	prevrev = None			return storageutil.emitrevisions(
				self, revs, revlogrevisiondelta,
	if deltaprevious or assumehaveparentrevisions:			deltaparentfn=self.deltaparent,
	prevrev = self.parentrevs(revs[0])[0]			candeltafn=self.candelta,
				rawsizefn=self.rawsize,
	# Set of revs available to delta against.			revdifffn=self.revdiff,
	available = set()			flagsfn=self.flags,
				sendfulltext=not self._storedeltachains,
	for rev in revs:			revisiondata=revisiondata,
	if rev == nullrev:			assumehaveparentrevisions=assumehaveparentrevisions,
	continue			deltaprevious=deltaprevious)

	node = fnode(rev)
	deltaparentrev = self.deltaparent(rev)
	p1rev, p2rev = self.parentrevs(rev)

	# Forced delta against previous mode.
	if deltaprevious:
	baserev = prevrev

	# Revlog is configured to use full snapshots. Stick to that.
	elif not self._storedeltachains:
	baserev = nullrev

	# There is a delta in storage. We try to use that because it
	# amounts to effectively copying data from storage and is
	# therefore the fastest.
	elif deltaparentrev != nullrev:
	# Base revision was already emitted in this group. We can
	# always safely use the delta.
	if deltaparentrev in available:
	baserev = deltaparentrev

	# Base revision is a parent that hasn't been emitted already.
	# Use it if we can assume the receiver has the parent revision.
	elif (assumehaveparentrevisions
	and deltaparentrev in (p1rev, p2rev)):
	baserev = deltaparentrev

	# No guarantee the receiver has the delta parent. Send delta
	# against last revision (if possible), which in the common case
	# should be similar enough to this revision that the delta is
	# reasonable.
	elif prevrev is not None:
	baserev = prevrev
	else:
	baserev = nullrev

	# Storage has a fulltext revision.

	# Let's use the previous revision, which is as good a guess as any.
	# There is definitely room to improve this logic.
	elif prevrev is not None:
	baserev = prevrev
	else:
	baserev = nullrev

	# But we can't actually use our chosen delta base for whatever
	# reason. Reset to fulltext.
	if baserev != nullrev and not self.candelta(baserev, rev):
	baserev = nullrev

	revision = None
	delta = None
	baserevisionsize = None

	if revisiondata:
	if self.iscensored(baserev) or self.iscensored(rev):
	try:
	revision = self.revision(node, raw=True)
	except error.CensoredNodeError as e:
	revision = e.tombstone

	if baserev != nullrev:
	baserevisionsize = self.rawsize(baserev)

	elif baserev == nullrev and not deltaprevious:
	revision = self.revision(node, raw=True)
	available.add(rev)
	else:
	delta = self.revdiff(baserev, rev)
	available.add(rev)

	yield revlogrevisiondelta(
	node=node,
	p1node=fnode(p1rev),
	p2node=fnode(p2rev),
	basenode=fnode(baserev),
	flags=self.flags(rev),
	baserevisionsize=baserevisionsize,
	revision=revision,
	delta=delta)

	prevrev = rev

	DELTAREUSEALWAYS = 'always'			DELTAREUSEALWAYS = 'always'
	DELTAREUSESAMEREVS = 'samerevs'			DELTAREUSESAMEREVS = 'samerevs'
	DELTAREUSENEVER = 'never'			DELTAREUSENEVER = 'never'

	DELTAREUSEFULLADD = 'fulladd'			DELTAREUSEFULLADD = 'fulladd'

	DELTAREUSEALL = {'always', 'samerevs', 'never', 'fulladd'}			DELTAREUSEALL = {'always', 'samerevs', 'never', 'fulladd'}

mercurial/utils/storageutil.py

	for p in parentrevsfn(strippoint):			for p in parentrevsfn(strippoint):
	if p != nullrev:			if p != nullrev:
	plinkrev = linkrevfn(p)			plinkrev = linkrevfn(p)
	heads[p] = plinkrev			heads[p] = plinkrev
	if plinkrev >= minlinkrev:			if plinkrev >= minlinkrev:
	futurelargelinkrevs.add(plinkrev)			futurelargelinkrevs.add(plinkrev)

	return strippoint, brokenrevs			return strippoint, brokenrevs

				def emitrevisions(store, revs, resultcls, deltaparentfn, candeltafn,
				durin42Unsubmitted Not Done This is a huge step backwards for remotefilelog and friends: it's all defined in terms of revs, whereas emitrevisions (as copypasta-inducing as it was) was in terms of nodes. We need to do better. durin42: This is a huge step backwards for remotefilelog and friends: it's all defined in terms of revs…
				rawsizefn, revdifffn, flagsfn, sendfulltext=False,
				revisiondata=False, assumehaveparentrevisions=False,
				deltaprevious=False):
				"""Generic implementation of ifiledata.emitrevisions().

				Emitting revision data is subtly complex. This function attempts to
				encapsulate all the logic for doing so in a backend-agnostic way.

				``store``
				Object conforming to ``ifilestorage`` interface.

				``revs``
				List of integer revision numbers whose data to emit.

				``resultcls``
				A type implementing the ``irevisiondelta`` interface that will be
				constructed and returned.

				``deltaparentfn``
				Callable receiving a revision number and returning the revision number
				of a revision that the internal delta is stored against. This delta
				will be preferred over computing a new arbitrary delta.

				``candeltafn``
				Callable receiving a pair of revision numbers that returns a bool
				indicating whether a delta between them can be produced.

				``rawsizefn``
				Callable receiving a revision number and returning the length of the
				``store.revision(rev, raw=True)``.

				``revdifffn``
				Callable receiving a pair of revision numbers that returns a delta
				between them.

				``flagsfn``
				Callable receiving a revision number and returns the integer flags
				value for it.

				``sendfulltext``
				Whether to send fulltext revisions instead of deltas, if allowed.

				``revisiondata``
				``assumehaveparentrevisions``
				``deltaprevious``
				See ``ifiledata.emitrevisions()`` interface documentation.
				"""

				fnode = store.node

				prevrev = None

				if deltaprevious or assumehaveparentrevisions:
				prevrev = store.parentrevs(revs[0])[0]

				# Set of revs available to delta against.
				available = set()

				for rev in revs:
				if rev == nullrev:
				continue

				node = fnode(rev)
				deltaparentrev = deltaparentfn(rev)
				p1rev, p2rev = store.parentrevs(rev)

				# Forced delta against previous mode.
				if deltaprevious:
				baserev = prevrev

				# We're instructed to send fulltext. Honor that.
				elif sendfulltext:
				baserev = nullrev

				# There is a delta in storage. We try to use that because it
				# amounts to effectively copying data from storage and is
				# therefore the fastest.
				elif deltaparentrev != nullrev:
				# Base revision was already emitted in this group. We can
				# always safely use the delta.
				if deltaparentrev in available:
				baserev = deltaparentrev

				# Base revision is a parent that hasn't been emitted already.
				# Use it if we can assume the receiver has the parent revision.
				elif (assumehaveparentrevisions
				and deltaparentrev in (p1rev, p2rev)):
				baserev = deltaparentrev

				# No guarantee the receiver has the delta parent. Send delta
				# against last revision (if possible), which in the common case
				# should be similar enough to this revision that the delta is
				# reasonable.
				elif prevrev is not None:
				baserev = prevrev
				else:
				baserev = nullrev

				# Storage has a fulltext revision.

				# Let's use the previous revision, which is as good a guess as any.
				# There is definitely room to improve this logic.
				elif prevrev is not None:
				baserev = prevrev
				else:
				baserev = nullrev

				# But we can't actually use our chosen delta base for whatever
				# reason. Reset to fulltext.
				if baserev != nullrev and not candeltafn(baserev, rev):
				baserev = nullrev

				revision = None
				delta = None
				baserevisionsize = None

				if revisiondata:
				if store.iscensored(baserev) or store.iscensored(rev):
				try:
				revision = store.revision(node, raw=True)
				except error.CensoredNodeError as e:
				revision = e.tombstone

				if baserev != nullrev:
				baserevisionsize = rawsizefn(baserev)

				elif baserev == nullrev and not deltaprevious:
				revision = store.revision(node, raw=True)
				available.add(rev)
				else:
				delta = revdifffn(baserev, rev)
				available.add(rev)

				yield resultcls(
				node=node,
				p1node=fnode(p1rev),
				p2node=fnode(p2rev),
				basenode=fnode(baserev),
				flags=flagsfn(rev),
				baserevisionsize=baserevisionsize,
				revision=revision,
				delta=delta)

				prevrev = rev

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	11486		Sep 28 2018, 8:18 PM	★	★
Diff 2	11617	rHG842ffcf1d42ff25e6c57052111055ddf790c97e9	Sep 28 2018, 7:16 PM	★	★