| mharbison72 | |
| pulkit |
| hg-reviewers |
The root argument should already be an absolute path, but we had
tests that passed a relative path. This patch fixes up the tests and
adds an assertion.
This assumes that os.path.isabs('/repo') will be True on all
platforms we care to run tests on. Augie tested for me that it does
work on Windows, so that's good enough for me.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Windows has some very strange rules for paths, and I remember fixing a subrepo bug with a path in the form C:foo. I tried these, and can't think of anything more we'd need to worry about.
>>> import os
>>> os.path.isabs("/")
True
>>> os.path.isabs("C:/")
True
>>> os.path.isabs("C:tests")
False(The last one being at the root of the hg repo.)
In D7649#112837, @mharbison72 wrote:Windows has some very strange rules for paths, and I remember fixing a subrepo bug with a path in the form C:foo. I tried these, and can't think of anything more we'd need to worry about.
>>> import os >>> os.path.isabs("/") True >>> os.path.isabs("C:/") True >>> os.path.isabs("C:tests") False(The last one being at the root of the hg repo.)
Thanks for checking. I think that means we're okay with this series now. I won't queue it because I wrote some of it.
I am bit confused with /repo everywhere that whether that's the only valid or other forms can be valid also. I will prefer making existing root passed as absolute instead of replacing them with /repo (provided I understood correctly). Also I am not sure, but does this counts as API change?
| mercurial/match.py | ||
|---|---|---|
| 186 | Hm, does this has to be always /repo or /foo is also a valid parameter here? | |
| tests/test-match.py | ||
| 69 | This and below ones can be /x too right? | |
In D7649#113440, @pulkit wrote:I am bit confused with /repo everywhere that whether that's the only valid or other forms can be valid also. I will prefer making existing root passed as absolute instead of replacing them with /repo (provided I understood correctly).
I switched to /repo only to try to clarify that the path is the path to the repo (not some subdirectory). Want me to switch them back?
Also I am not sure, but does this counts as API change?
I'm not sure if any extensions might have passed a relative path without noticing. Sure, we can add the (API) tag. Can you do that in flight? Or I can do it later if you want me to change some of the paths anyway.
In D7649#113454, @martinvonz wrote:In D7649#113440, @pulkit wrote:I am bit confused with /repo everywhere that whether that's the only valid or other forms can be valid also. I will prefer making existing root passed as absolute instead of replacing them with /repo (provided I understood correctly).
I switched to /repo only to try to clarify that the path is the path to the repo (not some subdirectory). Want me to switch them back?
Yes, that will be much appreciated.
| mercurial/match.py | ||
|---|---|---|
| 155 | We can mention here that it must be an absolute path. | |
| mercurial/match.py | ||
|---|---|---|
| 155 | I think "canonical" already implied "absolute". | |
| Path | Packages | |||
|---|---|---|---|---|
| M | mercurial/match.py (21 lines) | |||
| M | tests/test-manifest.py (16 lines) | |||
| M | tests/test-match.py (125 lines) |
| Status | Author | Revision | |
|---|---|---|---|
| Closed | mharbison72 | ||
| Closed | martinvonz | ||
| Closed | martinvonz | ||
| Closed | mharbison72 |
| [(b'bar/baz/qux.py', BIN_HASH_2), (b'foo', BIN_HASH_1 + b'a')], | [(b'bar/baz/qux.py', BIN_HASH_2), (b'foo', BIN_HASH_1 + b'a')], | ||||
| 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(b'', b'', [b're:foo']) | match = matchmod.match(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(b'', b'', [b're:.*']) | match = matchmod.match(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): | ||||
| [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(b'/', b'', [b'a/f'], default=b'relpath') | match = matchmod.match(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(b'/', b'', [b'']) | match = matchmod.match(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(b'/', b'', [b'a/b'], default=b'relpath') | match = matchmod.match(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', | ||||
| 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(b'/', b'a/b', [b'.'], default=b'relpath') | match = matchmod.match(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(b'/', b'', [b'a/b/*/*.txt']) | match = matchmod.match(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): | ||||
| dirs = [s._dir for s in m.walksubtrees()] | dirs = [s._dir for s in m.walksubtrees()] | ||||
| self.assertEqual( | self.assertEqual( | ||||
| sorted( | sorted( | ||||
| [b'', b'a/', b'a/c/', b'a/d/', b'a/b/', b'a/b/c/', b'a/b/d/'] | [b'', b'a/', b'a/c/', b'a/d/', b'a/b/', b'a/b/c/', b'a/b/d/'] | ||||
| ), | ), | ||||
| sorted(dirs), | sorted(dirs), | ||||
| ) | ) | ||||
| match = matchmod.match(b'/', b'', [b'path:a/b/']) | match = matchmod.match(b'/repo', b'', [b'path:a/b/']) | ||||
| dirs = [s._dir for s in m.walksubtrees(matcher=match)] | dirs = [s._dir for s in m.walksubtrees(matcher=match)] | ||||
| self.assertEqual(sorted([b'a/b/', b'a/b/c/', b'a/b/d/']), sorted(dirs)) | self.assertEqual(sorted([b'a/b/', b'a/b/c/', b'a/b/d/']), sorted(dirs)) | ||||
| if __name__ == '__main__': | if __name__ == '__main__': | ||||
| silenttestrunner.main(__name__) | silenttestrunner.main(__name__) | ||||
We can mention here that it must be an absolute path.