This is an archive of the discontinued Mercurial Phabricator instance.

manifest: move matches method to be outside the interface
ClosedPublic

Authored by durin42 on Feb 5 2020, 5:34 PM.

Download Raw Diff

Details

Reviewers

martinvonz
pulkit

Group Reviewers

hg-reviewers

Commits

rHG0bf3b5e80d30: manifest: move matches method to be outside the interface

Summary

In order to adequately smoke out any legacy consumers of the method, we rename
it to _matches so it's clear that it's class-private. To my amazement, all
consumers of this method really only wanted matching filenames, not a full
filtered manifest.

Diff Detail

Repository

rHG Mercurial

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

durin42 created this revision.Feb 5 2020, 5:34 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptFeb 5 2020, 5:34 PM

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

martinvonz requested changes to this revision.Feb 7 2020, 5:08 PM

martinvonz added a subscriber: martinvonz.

martinvonz added inline comments.

mercurial/manifest.py
1200	This now checks `match.always()` recursively for every tree manifest. Seems wasteful, but maybe not noticeable? Could you test (or just revert that part of the patch)?

This revision now requires changes to proceed.Feb 7 2020, 5:08 PM

durin42 updated this revision to Diff 20101.Feb 10 2020, 7:49 PM

pulkit accepted this revision.Feb 10 2020, 9:33 PM

durin42 added a commit: rHG0bf3b5e80d30: manifest: move matches method to be outside the interface.Feb 10 2020, 9:43 PM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHG0bf3b5e80d30: manifest: move matches method to be outside the interface (authored by durin42). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/interfaces/repository.py (9 lines)
M			mercurial/manifest.py (26 lines)
M			tests/test-manifest.py (22 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	19928		Feb 5 2020, 5:34 PM	★	★
Diff 2	20101		Feb 10 2020, 7:49 PM	★	★
Diff 3	20142	rHG0bf3b5e80d3028d533f04defdffe84d0aa79194b	Feb 5 2020, 5:13 PM	★	★

Commit	Parents	Author	Summary	Date
88f0820a2ec7	748e1cf7450f	Augie Fackler		Feb 5 2020, 5:13 PM

Status	Author	Revision
Closed	durin42	D8083 archival: use walk() instead of matches() on manifest
Closed	durin42	D8090 archival: use walk() instead of matches() on manifest
Closed	durin42	D8085 manifest: move matches method to be outside the interface
Closed	durin42	D8084 merge: use manifestdict.walk() instead of manifestdict.matches()
Closed	durin42	D8082 manifest: rewrite filesnotin to not make superfluous manifest copies
Closed	durin42	D8081 manifest: fix _very_ subtle bug with exact matchers passed to walk()
Closed	durin42	D8080 context: use manifest.walk() instead of manifest.match() to get file list
Closed	durin42	D8079 manifest: remove `.new()` from the interface

Diff 19928

mercurial/interfaces/repository.py

	"""			"""

	def dirs():			def dirs():
	"""Returns an object implementing the ``idirs`` interface."""			"""Returns an object implementing the ``idirs`` interface."""

	def hasdir(dir):			def hasdir(dir):
	"""Returns a bool indicating if a directory is in this manifest."""			"""Returns a bool indicating if a directory is in this manifest."""

	def matches(match):
	"""Generate a new manifest filtered through a matcher.

	Returns an object conforming to the ``imanifestdict`` interface.
	"""

	def walk(match):			def walk(match):
	"""Generator of paths in manifest satisfying a matcher.			"""Generator of paths in manifest satisfying a matcher.

	This is equivalent to ``self.matches(match).iterkeys()`` except a new
	manifest object is not created.

	If the matcher has explicit files listed and they don't exist in			If the matcher has explicit files listed and they don't exist in
	the manifest, ``match.bad()`` is called for each missing file.			the manifest, ``match.bad()`` is called for each missing file.
	"""			"""

	def diff(other, match=None, clean=False):			def diff(other, match=None, clean=False):
	"""Find differences between this manifest and another.			"""Find differences between this manifest and another.

	This manifest is compared to ``other``.			This manifest is compared to ``other``.

mercurial/manifest.py

	# for dirstate.walk, files=[''] means "walk the whole tree".			# for dirstate.walk, files=[''] means "walk the whole tree".
	# follow that here, too			# follow that here, too
	fset.discard(b'')			fset.discard(b'')

	for fn in sorted(fset):			for fn in sorted(fset):
	if not self.hasdir(fn):			if not self.hasdir(fn):
	match.bad(fn, None)			match.bad(fn, None)

	def matches(self, match):			def _matches(self, match):
	'''generate a new manifest filtered by the match argument'''			'''generate a new manifest filtered by the match argument'''
	if match.always():			if match.always():
	return self.copy()			return self.copy()

	if self._filesfastpath(match):			if self._filesfastpath(match):
	m = manifestdict()			m = manifestdict()
	lm = self._lm			lm = self._lm
	for fn in match.files():			for fn in match.files():
	The result is returned as a dict with filename as key and			The result is returned as a dict with filename as key and
	values of the form ((n1,fl1),(n2,fl2)), where n1/n2 is the			values of the form ((n1,fl1),(n2,fl2)), where n1/n2 is the
	nodeid in the current/other manifest and fl1/fl2 is the flag			nodeid in the current/other manifest and fl1/fl2 is the flag
	in the current/other manifest. Where the file does not exist,			in the current/other manifest. Where the file does not exist,
	the nodeid will be None and the flags will be the empty			the nodeid will be None and the flags will be the empty
	string.			string.
	'''			'''
	if match:			if match:
	m1 = self.matches(match)			m1 = self._matches(match)
	m2 = m2.matches(match)			m2 = m2._matches(match)
	return m1.diff(m2, clean=clean)			return m1.diff(m2, clean=clean)
	return self._lm.diff(m2._lm, clean)			return self._lm.diff(m2._lm, clean)

	def setflag(self, key, flag):			def setflag(self, key, flag):
	self._lm[key] = self[key], flag			self._lm[key] = self[key], flag

	def get(self, key, default=None):			def get(self, key, default=None):
	try:			try:
	copy._copyfunc = _copyfunc			copy._copyfunc = _copyfunc
	else:			else:
	copy._copyfunc = self._copyfunc			copy._copyfunc = self._copyfunc
	return copy			return copy

	def filesnotin(self, m2, match=None):			def filesnotin(self, m2, match=None):
	'''Set of files in this manifest that are not in the other'''			'''Set of files in this manifest that are not in the other'''
	if match and not match.always():			if match and not match.always():
	m1 = self.matches(match)			m1 = self._matches(match)
	m2 = m2.matches(match)			m2 = m2._matches(match)
	return m1.filesnotin(m2)			return m1.filesnotin(m2)

	files = set()			files = set()

	def _filesnotin(t1, t2):			def _filesnotin(t1, t2):
	if t1._node == t2._node and not t1._dirty and not t2._dirty:			if t1._node == t2._node and not t1._dirty and not t2._dirty:
	return			return
	t1._load()			t1._load()
	return self._dirs[topdir].hasdir(subdir)			return self._dirs[topdir].hasdir(subdir)
	return False			return False
	dirslash = dir + b'/'			dirslash = dir + b'/'
	return dirslash in self._dirs or dirslash in self._lazydirs			return dirslash in self._dirs or dirslash in self._lazydirs

	def walk(self, match):			def walk(self, match):
	'''Generates matching file names.			'''Generates matching file names.

	Equivalent to manifest.matches(match).iterkeys(), but without creating
	an entirely new manifest.

	It also reports nonexistent files by marking them bad with match.bad().			It also reports nonexistent files by marking them bad with match.bad().
	'''			'''
	if match.always():			if match.always():
	for f in iter(self):			for f in iter(self):
	yield f			yield f
	return			return

	fset = set(match.files())			fset = set(match.files())
	fullp = self._subpath(p)			fullp = self._subpath(p)
	if match(fullp):			if match(fullp):
	yield fullp			yield fullp
	else:			else:
	if not visit or p[:-1] in visit:			if not visit or p[:-1] in visit:
	for f in self._dirs[p]._walk(match):			for f in self._dirs[p]._walk(match):
	yield f			yield f

	def matches(self, match):
	'''generate a new manifest filtered by the match argument'''
	if match.always():
	return self.copy()

	return self._matches(match)

	def _matches(self, match):			def _matches(self, match):
	'''recursively generate a new manifest filtered by the match argument.			'''recursively generate a new manifest filtered by the match argument.
	'''			'''
				if match.always():
				return self.copy()

	visit = match.visitchildrenset(self._dir[:-1])			visit = match.visitchildrenset(self._dir[:-1])
	if visit == b'all':			if visit == b'all':
	return self.copy()			return self.copy()
	ret = treemanifest(self._dir)			ret = treemanifest(self._dir)
	if not visit:			if not visit:
	return ret			return ret

	ret._files[fn] = self._files[fn]			ret._files[fn] = self._files[fn]
	if fn in self._flags:			if fn in self._flags:
	ret._flags[fn] = self._flags[fn]			ret._flags[fn] = self._flags[fn]

	visit = self._loadchildrensetlazy(visit)			visit = self._loadchildrensetlazy(visit)
	for dir, subm in pycompat.iteritems(self._dirs):			for dir, subm in pycompat.iteritems(self._dirs):
	if visit and dir[:-1] not in visit:			if visit and dir[:-1] not in visit:
	continue			continue
	m = subm._matches(match)			m = subm._matches(match)
				martinvonzUnsubmitted Not Done This now checks `match.always()` recursively for every tree manifest. Seems wasteful, but maybe not noticeable? Could you test (or just revert that part of the patch)? martinvonz: This now checks `match.always()` recursively for every tree manifest. Seems wasteful, but maybe…
	if not m._isempty():			if not m._isempty():
	ret._dirs[dir] = m			ret._dirs[dir] = m

	if not ret._isempty():			if not ret._isempty():
	ret._dirty = True			ret._dirty = True
	return ret			return ret

	def diff(self, m2, match=None, clean=False):			def diff(self, m2, match=None, clean=False):
	'''Finds changes between the current manifest and m2.			'''Finds changes between the current manifest and m2.

	Args:			Args:
	m2: the manifest to which this manifest should be compared.			m2: the manifest to which this manifest should be compared.
	clean: if true, include files unchanged between these manifests			clean: if true, include files unchanged between these manifests
	with a None value in the returned dictionary.			with a None value in the returned dictionary.

	The result is returned as a dict with filename as key and			The result is returned as a dict with filename as key and
	values of the form ((n1,fl1),(n2,fl2)), where n1/n2 is the			values of the form ((n1,fl1),(n2,fl2)), where n1/n2 is the
	nodeid in the current/other manifest and fl1/fl2 is the flag			nodeid in the current/other manifest and fl1/fl2 is the flag
	in the current/other manifest. Where the file does not exist,			in the current/other manifest. Where the file does not exist,
	the nodeid will be None and the flags will be the empty			the nodeid will be None and the flags will be the empty
	string.			string.
	'''			'''
	if match and not match.always():			if match and not match.always():
	m1 = self.matches(match)			m1 = self._matches(match)
	m2 = m2.matches(match)			m2 = m2._matches(match)
	return m1.diff(m2, clean=clean)			return m1.diff(m2, clean=clean)
	result = {}			result = {}
	emptytree = treemanifest()			emptytree = treemanifest()

	def _iterativediff(t1, t2, stack):			def _iterativediff(t1, t2, stack):
	"""compares two tree manifests and append new tree-manifests which			"""compares two tree manifests and append new tree-manifests which
	needs to be compared to stack"""			needs to be compared to stack"""
	if t1._node == t2._node and not t1._dirty and not t2._dirty:			if t1._node == t2._node and not t1._dirty and not t2._dirty:

tests/test-manifest.py

	list(m.items()),			list(m.items()),
	)			)
	# Sometimes it even tries a 22-byte fake hash, but we can			# Sometimes it even tries a 22-byte fake hash, but we can
	# return 21 and it'll work out			# return 21 and it'll work out
	m[b'foo'] = want + b'+'			m[b'foo'] = want + b'+'
	self.assertEqual(want, m[b'foo'])			self.assertEqual(want, m[b'foo'])
	# make sure the suffix survives a copy			# make sure the suffix survives a copy
	match = matchmod.match(util.localpath(b'/repo'), b'', [b're:foo'])			match = matchmod.match(util.localpath(b'/repo'), b'', [b're:foo'])
	m2 = m.matches(match)			m2 = m._matches(match)
	self.assertEqual(want, m2[b'foo'])			self.assertEqual(want, m2[b'foo'])
	self.assertEqual(1, len(m2))			self.assertEqual(1, len(m2))
	m2 = m.copy()			m2 = m.copy()
	self.assertEqual(want, m2[b'foo'])			self.assertEqual(want, m2[b'foo'])
	# suffix with iteration			# suffix with iteration
	self.assertEqual(			self.assertEqual(
	[(b'bar/baz/qux.py', BIN_HASH_2), (b'foo', want)], list(m.items())			[(b'bar/baz/qux.py', BIN_HASH_2), (b'foo', want)], list(m.items())
	)			)

	# shows up in diff			# shows up in diff
	self.assertEqual({b'foo': ((want, f), (h, b''))}, m.diff(clean))			self.assertEqual({b'foo': ((want, f), (h, b''))}, m.diff(clean))
	self.assertEqual({b'foo': ((h, b''), (want, f))}, clean.diff(m))			self.assertEqual({b'foo': ((h, b''), (want, f))}, clean.diff(m))

	def testMatchException(self):			def testMatchException(self):
	m = self.parsemanifest(A_SHORT_MANIFEST)			m = self.parsemanifest(A_SHORT_MANIFEST)
	match = matchmod.match(util.localpath(b'/repo'), b'', [b're:.*'])			match = matchmod.match(util.localpath(b'/repo'), b'', [b're:.*'])

	def filt(path):			def filt(path):
	if path == b'foo':			if path == b'foo':
	assert False			assert False
	return True			return True

	match.matchfn = filt			match.matchfn = filt
	with self.assertRaises(AssertionError):			with self.assertRaises(AssertionError):
	m.matches(match)			m._matches(match)

	def testRemoveItem(self):			def testRemoveItem(self):
	m = self.parsemanifest(A_SHORT_MANIFEST)			m = self.parsemanifest(A_SHORT_MANIFEST)
	del m[b'foo']			del m[b'foo']
	with self.assertRaises(KeyError):			with self.assertRaises(KeyError):
	m[b'foo']			m[b'foo']
	self.assertEqual(1, len(m))			self.assertEqual(1, len(m))
	self.assertEqual(1, len(list(m)))			self.assertEqual(1, len(list(m)))

	def testMatchesMetadata(self):			def testMatchesMetadata(self):
	'''Tests matches() for a few specific files to make sure that both			'''Tests matches() for a few specific files to make sure that both
	the set of files as well as their flags and nodeids are correct in			the set of files as well as their flags and nodeids are correct in
	the resulting manifest.'''			the resulting manifest.'''
	m = self.parsemanifest(A_HUGE_MANIFEST)			m = self.parsemanifest(A_HUGE_MANIFEST)

	match = matchmod.exact([b'file1', b'file200', b'file300'])			match = matchmod.exact([b'file1', b'file200', b'file300'])
	m2 = m.matches(match)			m2 = m._matches(match)

	w = (b'file1\0%sx\n' b'file200\0%sl\n' b'file300\0%s\n') % (			w = (b'file1\0%sx\n' b'file200\0%sl\n' b'file300\0%s\n') % (
	HASH_2,			HASH_2,
	HASH_1,			HASH_1,
	HASH_1,			HASH_1,
	)			)
	self.assertEqual(w, m2.text())			self.assertEqual(w, m2.text())

	def testMatchesNonexistentFile(self):			def testMatchesNonexistentFile(self):
	'''Tests matches() for a small set of specific files, including one			'''Tests matches() for a small set of specific files, including one
	nonexistent file to make sure in only matches against existing files.			nonexistent file to make sure in only matches against existing files.
	'''			'''
	m = self.parsemanifest(A_DEEPER_MANIFEST)			m = self.parsemanifest(A_DEEPER_MANIFEST)

	match = matchmod.exact(			match = matchmod.exact(
	[b'a/b/c/bar.txt', b'a/b/d/qux.py', b'readme.txt', b'nonexistent']			[b'a/b/c/bar.txt', b'a/b/d/qux.py', b'readme.txt', b'nonexistent']
	)			)
	m2 = m.matches(match)			m2 = m._matches(match)

	self.assertEqual(			self.assertEqual(
	[b'a/b/c/bar.txt', b'a/b/d/qux.py', b'readme.txt'], m2.keys()			[b'a/b/c/bar.txt', b'a/b/d/qux.py', b'readme.txt'], m2.keys()
	)			)

	def testMatchesNonexistentDirectory(self):			def testMatchesNonexistentDirectory(self):
	'''Tests matches() for a relpath match on a directory that doesn't			'''Tests matches() for a relpath match on a directory that doesn't
	actually exist.'''			actually exist.'''
	m = self.parsemanifest(A_DEEPER_MANIFEST)			m = self.parsemanifest(A_DEEPER_MANIFEST)

	match = matchmod.match(			match = matchmod.match(
	util.localpath(b'/repo'), b'', [b'a/f'], default=b'relpath'			util.localpath(b'/repo'), b'', [b'a/f'], default=b'relpath'
	)			)
	m2 = m.matches(match)			m2 = m._matches(match)

	self.assertEqual([], m2.keys())			self.assertEqual([], m2.keys())

	def testMatchesExactLarge(self):			def testMatchesExactLarge(self):
	'''Tests matches() for files matching a large list of exact files.			'''Tests matches() for files matching a large list of exact files.
	'''			'''
	m = self.parsemanifest(A_HUGE_MANIFEST)			m = self.parsemanifest(A_HUGE_MANIFEST)

	flist = m.keys()[80:300]			flist = m.keys()[80:300]
	match = matchmod.exact(flist)			match = matchmod.exact(flist)
	m2 = m.matches(match)			m2 = m._matches(match)

	self.assertEqual(flist, m2.keys())			self.assertEqual(flist, m2.keys())

	def testMatchesFull(self):			def testMatchesFull(self):
	'''Tests matches() for what should be a full match.'''			'''Tests matches() for what should be a full match.'''
	m = self.parsemanifest(A_DEEPER_MANIFEST)			m = self.parsemanifest(A_DEEPER_MANIFEST)

	match = matchmod.match(util.localpath(b'/repo'), b'', [b''])			match = matchmod.match(util.localpath(b'/repo'), b'', [b''])
	m2 = m.matches(match)			m2 = m._matches(match)

	self.assertEqual(m.keys(), m2.keys())			self.assertEqual(m.keys(), m2.keys())

	def testMatchesDirectory(self):			def testMatchesDirectory(self):
	'''Tests matches() on a relpath match on a directory, which should			'''Tests matches() on a relpath match on a directory, which should
	match against all files within said directory.'''			match against all files within said directory.'''
	m = self.parsemanifest(A_DEEPER_MANIFEST)			m = self.parsemanifest(A_DEEPER_MANIFEST)

	match = matchmod.match(			match = matchmod.match(
	util.localpath(b'/repo'), b'', [b'a/b'], default=b'relpath'			util.localpath(b'/repo'), b'', [b'a/b'], default=b'relpath'
	)			)
	m2 = m.matches(match)			m2 = m._matches(match)

	self.assertEqual(			self.assertEqual(
	[			[
	b'a/b/c/bar.py',			b'a/b/c/bar.py',
	b'a/b/c/bar.txt',			b'a/b/c/bar.txt',
	b'a/b/c/foo.py',			b'a/b/c/foo.py',
	b'a/b/c/foo.txt',			b'a/b/c/foo.txt',
	b'a/b/d/baz.py',			b'a/b/d/baz.py',
	b'a/b/d/qux.py',			b'a/b/d/qux.py',
	b'a/b/d/ten.txt',			b'a/b/d/ten.txt',
	b'a/b/dog.py',			b'a/b/dog.py',
	b'a/b/fish.py',			b'a/b/fish.py',
	],			],
	m2.keys(),			m2.keys(),
	)			)

	def testMatchesExactPath(self):			def testMatchesExactPath(self):
	'''Tests matches() on an exact match on a directory, which should			'''Tests matches() on an exact match on a directory, which should
	result in an empty manifest because you can't perform an exact match			result in an empty manifest because you can't perform an exact match
	against a directory.'''			against a directory.'''
	m = self.parsemanifest(A_DEEPER_MANIFEST)			m = self.parsemanifest(A_DEEPER_MANIFEST)

	match = matchmod.exact([b'a/b'])			match = matchmod.exact([b'a/b'])
	m2 = m.matches(match)			m2 = m._matches(match)

	self.assertEqual([], m2.keys())			self.assertEqual([], m2.keys())

	def testMatchesCwd(self):			def testMatchesCwd(self):
	'''Tests matches() on a relpath match with the current directory ('.')			'''Tests matches() on a relpath match with the current directory ('.')
	when not in the root directory.'''			when not in the root directory.'''
	m = self.parsemanifest(A_DEEPER_MANIFEST)			m = self.parsemanifest(A_DEEPER_MANIFEST)

	match = matchmod.match(			match = matchmod.match(
	util.localpath(b'/repo'), b'a/b', [b'.'], default=b'relpath'			util.localpath(b'/repo'), b'a/b', [b'.'], default=b'relpath'
	)			)
	m2 = m.matches(match)			m2 = m._matches(match)

	self.assertEqual(			self.assertEqual(
	[			[
	b'a/b/c/bar.py',			b'a/b/c/bar.py',
	b'a/b/c/bar.txt',			b'a/b/c/bar.txt',
	b'a/b/c/foo.py',			b'a/b/c/foo.py',
	b'a/b/c/foo.txt',			b'a/b/c/foo.txt',
	b'a/b/d/baz.py',			b'a/b/d/baz.py',
	b'a/b/d/qux.py',			b'a/b/d/qux.py',
	b'a/b/d/ten.txt',			b'a/b/d/ten.txt',
	b'a/b/dog.py',			b'a/b/dog.py',
	b'a/b/fish.py',			b'a/b/fish.py',
	],			],
	m2.keys(),			m2.keys(),
	)			)

	def testMatchesWithPattern(self):			def testMatchesWithPattern(self):
	'''Tests matches() for files matching a pattern that reside			'''Tests matches() for files matching a pattern that reside
	deeper than the specified directory.'''			deeper than the specified directory.'''
	m = self.parsemanifest(A_DEEPER_MANIFEST)			m = self.parsemanifest(A_DEEPER_MANIFEST)

	match = matchmod.match(util.localpath(b'/repo'), b'', [b'a/b//.txt'])			match = matchmod.match(util.localpath(b'/repo'), b'', [b'a/b//.txt'])
	m2 = m.matches(match)			m2 = m._matches(match)

	self.assertEqual(			self.assertEqual(
	[b'a/b/c/bar.txt', b'a/b/c/foo.txt', b'a/b/d/ten.txt'], m2.keys()			[b'a/b/c/bar.txt', b'a/b/c/foo.txt', b'a/b/d/ten.txt'], m2.keys()
	)			)


	class testmanifestdict(unittest.TestCase, basemanifesttests):			class testmanifestdict(unittest.TestCase, basemanifesttests):
	def parsemanifest(self, text):			def parsemanifest(self, text):