Merge lp:~jelmer/brz/memorytree-symlinks into lp:brz/3.0

[Jelmer Vernooij]

Implement MemoryTree.get_symlink_target.

Implement MemoryTree.get_symlink_target.

[Martin Packman (gz)]

Thanks! See one inline comment about a duplicated statement.

review: Approve

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
https://ci.breezy-vcs.org/job/brz-3.0/job/brz-3.0-land/12/

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
https://ci.breezy-vcs.org/job/brz-3.0/job/brz-3.0-land/16/

=== modified file 'breezy/git/memorytree.py'
--- breezy/git/memorytree.py	2018-11-18 00:25:19 +0000
+++ breezy/git/memorytree.py	2019-03-04 01:49:51 +0000
@@ -258,3 +258,7 @@
     def kind(self, p):
         stat_value = self._file_transport.stat(p)
         return osutils.file_kind_from_stat_mode(stat_value.st_mode)
+
+    def get_symlink_target(self, path):
+        with self.lock_read():
+            return self._file_transport.readlink(path)

=== modified file 'breezy/memorytree.py'
--- breezy/memorytree.py	2018-11-18 00:25:19 +0000
+++ breezy/memorytree.py	2019-03-04 01:49:51 +0000
@@ -22,6 +22,7 @@
 from __future__ import absolute_import
 
 import os
+import stat
 
 from . import (
     errors,
@@ -62,7 +63,15 @@
         with self.lock_tree_write():
             for f, file_id, kind in zip(files, ids, kinds):
                 if kind is None:
-                    kind = 'file'
+                    st_mode = self._file_transport.stat(f).st_mode
+                    if stat.S_ISREG(st_mode):
+                        kind = 'file'
+                    elif stat.S_ISLNK(st_mode):
+                        kind = 'symlink'
+                    elif stat.S_ISDIR(st_mode):
+                        kind = 'directory'
+                    else:
+                        raise AssertionError('Unknown file kind')
                 if file_id is None:
                     self._inventory.add_path(f, kind=kind)
                 else:
@@ -127,7 +136,7 @@
             # memory tree does not support nested trees yet.
             return kind, None, None, None
         elif kind == 'symlink':
-            raise NotImplementedError('symlink support')
+            return kind, None, None, self._inventory[id].symlink_target
         else:
             raise NotImplementedError('unknown kind')
 
@@ -148,8 +157,7 @@
         return self._inventory.get_entry_by_path(path).executable
 
     def kind(self, path):
-        file_id = self.path2id(path)
-        return self._inventory[file_id].kind
+        return self._inventory.get_entry_by_path(path).kind
 
     def mkdir(self, path, file_id=None):
         """See MutableTree.mkdir()."""
@@ -227,6 +235,8 @@
                 continue
             if entry.kind == 'directory':
                 self._file_transport.mkdir(path)
+            elif entry.kind == 'symlink':
+                self._file_transport.symlink(entry.symlink_target, path)
             elif entry.kind == 'file':
                 self._file_transport.put_file(
                     path, self._basis_tree.get_file(path))
@@ -302,6 +312,10 @@
             else:
                 raise
 
+    def get_symlink_target(self, path):
+        with self.lock_read():
+            return self._file_transport.readlink(path)
+
     def set_parent_trees(self, parents_list, allow_leftmost_as_ghost=False):
         """See MutableTree.set_parent_trees()."""
         if len(parents_list) == 0:

=== modified file 'breezy/tests/test_memorytree.py'
--- breezy/tests/test_memorytree.py	2018-11-11 04:08:32 +0000
+++ breezy/tests/test_memorytree.py	2019-03-04 01:49:51 +0000
@@ -46,21 +46,17 @@
         rev_id = tree.commit('first post')
         tree.unlock()
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_read()
-        self.assertEqual([rev_id], tree.get_parent_ids())
-        with tree.get_file('foo') as f:
-            self.assertEqual(b'contents of foo\n', f.read())
-        tree.unlock()
+        with tree.lock_read():
+            self.assertEqual([rev_id], tree.get_parent_ids())
+            with tree.get_file('foo') as f:
+                self.assertEqual(b'contents of foo\n', f.read())
 
     def test_get_root_id(self):
         branch = self.make_branch('branch')
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_write()
-        try:
+        with tree.lock_write():
             tree.add([''])
             self.assertIsNot(None, tree.get_root_id())
-        finally:
-            tree.unlock()
 
     def test_lock_tree_write(self):
         """Check we can lock_tree_write and unlock MemoryTrees."""
@@ -73,9 +69,8 @@
         """Check that we error when trying to upgrade a read lock to write."""
         branch = self.make_branch('branch')
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_read()
-        self.assertRaises(errors.ReadOnlyError, tree.lock_tree_write)
-        tree.unlock()
+        with tree.lock_read():
+            self.assertRaises(errors.ReadOnlyError, tree.lock_tree_write)
 
     def test_lock_write(self):
         """Check we can lock_write and unlock MemoryTrees."""
@@ -88,58 +83,63 @@
         """Check that we error when trying to upgrade a read lock to write."""
         branch = self.make_branch('branch')
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_read()
-        self.assertRaises(errors.ReadOnlyError, tree.lock_write)
-        tree.unlock()
+        with tree.lock_read():
+            self.assertRaises(errors.ReadOnlyError, tree.lock_write)
 
     def test_add_with_kind(self):
         branch = self.make_branch('branch')
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_write()
-        tree.add(['', 'afile', 'adir'], None,
-                 ['directory', 'file', 'directory'])
-        self.assertEqual('afile', tree.id2path(tree.path2id('afile')))
-        self.assertEqual('adir', tree.id2path(tree.path2id('adir')))
-        self.assertFalse(tree.has_filename('afile'))
-        self.assertFalse(tree.has_filename('adir'))
-        tree.unlock()
+        with tree.lock_write():
+            tree.add(['', 'afile', 'adir'], None,
+                     ['directory', 'file', 'directory'])
+            self.assertEqual('afile', tree.id2path(tree.path2id('afile')))
+            self.assertEqual('adir', tree.id2path(tree.path2id('adir')))
+            self.assertFalse(tree.has_filename('afile'))
+            self.assertFalse(tree.has_filename('adir'))
 
     def test_put_new_file(self):
         branch = self.make_branch('branch')
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_write()
-        tree.add(['', 'foo'], ids=[b'root-id', b'foo-id'],
-                 kinds=['directory', 'file'])
-        tree.put_file_bytes_non_atomic('foo', b'barshoom')
-        self.assertEqual(b'barshoom', tree.get_file('foo').read())
-        tree.unlock()
+        with tree.lock_write():
+            tree.add(['', 'foo'], ids=[b'root-id', b'foo-id'],
+                     kinds=['directory', 'file'])
+            tree.put_file_bytes_non_atomic('foo', b'barshoom')
+            with tree.get_file('foo') as f:
+                self.assertEqual(b'barshoom', f.read())
 
     def test_put_existing_file(self):
         branch = self.make_branch('branch')
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_write()
-        tree.add(['', 'foo'], ids=[b'root-id', b'foo-id'],
-                 kinds=['directory', 'file'])
-        tree.put_file_bytes_non_atomic('foo', b'first-content')
-        tree.put_file_bytes_non_atomic('foo', b'barshoom')
-        self.assertEqual(b'barshoom', tree.get_file('foo').read())
-        tree.unlock()
+        with tree.lock_write():
+            tree.add(['', 'foo'], ids=[b'root-id', b'foo-id'],
+                     kinds=['directory', 'file'])
+            tree.put_file_bytes_non_atomic('foo', b'first-content')
+            tree.put_file_bytes_non_atomic('foo', b'barshoom')
+            self.assertEqual(b'barshoom', tree.get_file('foo').read())
 
     def test_add_in_subdir(self):
         branch = self.make_branch('branch')
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_write()
-        self.addCleanup(tree.unlock)
-        tree.add([''], [b'root-id'], ['directory'])
-        # Unfortunately, the only way to 'mkdir' is to call 'tree.mkdir', but
-        # that *always* adds the directory as well. So if you want to create a
-        # file in a subdirectory, you have to split out the 'mkdir()' calls
-        # from the add and put_file_bytes_non_atomic calls. :(
-        tree.mkdir('adir', b'dir-id')
-        tree.add(['adir/afile'], [b'file-id'], ['file'])
-        self.assertEqual('adir/afile', tree.id2path(b'file-id'))
-        self.assertEqual('adir', tree.id2path(b'dir-id'))
-        tree.put_file_bytes_non_atomic('adir/afile', b'barshoom')
+        with tree.lock_write():
+            tree.add([''], [b'root-id'], ['directory'])
+            # Unfortunately, the only way to 'mkdir' is to call 'tree.mkdir', but
+            # that *always* adds the directory as well. So if you want to create a
+            # file in a subdirectory, you have to split out the 'mkdir()' calls
+            # from the add and put_file_bytes_non_atomic calls. :(
+            tree.mkdir('adir', b'dir-id')
+            tree.add(['adir/afile'], [b'file-id'], ['file'])
+            self.assertEqual('adir/afile', tree.id2path(b'file-id'))
+            self.assertEqual('adir', tree.id2path(b'dir-id'))
+            tree.put_file_bytes_non_atomic('adir/afile', b'barshoom')
+
+    def test_add_symlink(self):
+        branch = self.make_branch('branch')
+        tree = MemoryTree.create_on_branch(branch)
+        with tree.lock_write():
+            tree._file_transport.symlink('bar', 'foo')
+            tree.add(['', 'foo'])
+            self.assertEqual('symlink', tree.kind('foo'))
+            self.assertEqual('bar', tree.get_symlink_target('foo'))
 
     def test_commit_trivial(self):
         """Smoke test for commit on a MemoryTree.
@@ -149,40 +149,35 @@
         """
         branch = self.make_branch('branch')
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_write()
-        tree.add(['', 'foo'], ids=[b'root-id', b'foo-id'],
-                 kinds=['directory', 'file'])
-        tree.put_file_bytes_non_atomic('foo', b'barshoom')
-        revision_id = tree.commit('message baby')
-        # the parents list for the tree should have changed.
-        self.assertEqual([revision_id], tree.get_parent_ids())
-        tree.unlock()
+        with tree.lock_write():
+            tree.add(['', 'foo'], ids=[b'root-id', b'foo-id'],
+                     kinds=['directory', 'file'])
+            tree.put_file_bytes_non_atomic('foo', b'barshoom')
+            revision_id = tree.commit('message baby')
+            # the parents list for the tree should have changed.
+            self.assertEqual([revision_id], tree.get_parent_ids())
         # and we should have a revision that is accessible outside the tree lock
         revtree = tree.branch.repository.revision_tree(revision_id)
-        revtree.lock_read()
-        self.addCleanup(revtree.unlock)
-        with revtree.get_file('foo') as f:
+        with revtree.lock_read(), revtree.get_file('foo') as f:
             self.assertEqual(b'barshoom', f.read())
 
     def test_unversion(self):
         """Some test for unversion of a memory tree."""
         branch = self.make_branch('branch')
         tree = MemoryTree.create_on_branch(branch)
-        tree.lock_write()
-        tree.add(['', 'foo'], ids=[b'root-id', b'foo-id'],
-                 kinds=['directory', 'file'])
-        tree.unversion(['foo'])
-        self.assertFalse(tree.is_versioned('foo'))
-        self.assertFalse(tree.has_id(b'foo-id'))
-        tree.unlock()
+        with tree.lock_write():
+            tree.add(['', 'foo'], ids=[b'root-id', b'foo-id'],
+                     kinds=['directory', 'file'])
+            tree.unversion(['foo'])
+            self.assertFalse(tree.is_versioned('foo'))
+            self.assertFalse(tree.has_id(b'foo-id'))
 
     def test_last_revision(self):
         """There should be a last revision method we can call."""
         tree = self.make_branch_and_memory_tree('branch')
-        tree.lock_write()
-        tree.add('')
-        rev_id = tree.commit('first post')
-        tree.unlock()
+        with tree.lock_write():
+            tree.add('')
+            rev_id = tree.commit('first post')
         self.assertEqual(rev_id, tree.last_revision())
 
     def test_rename_file(self):

=== modified file 'breezy/transport/memory.py'
--- breezy/transport/memory.py	2018-11-17 16:53:10 +0000
+++ breezy/transport/memory.py	2019-03-04 01:49:51 +0000
@@ -26,9 +26,10 @@
 from io import (
     BytesIO,
     )
+import itertools
 import os
 import errno
-from stat import S_IFREG, S_IFDIR, S_IFLNK
+from stat import S_IFREG, S_IFDIR, S_IFLNK, S_ISDIR
 
 from .. import (
     transport,
@@ -50,20 +51,16 @@
 
 class MemoryStat(object):
 
-    def __init__(self, size, kind, perms):
+    def __init__(self, size, kind, perms=None):
         self.st_size = size
-        if kind == 'file':
+        if not S_ISDIR(kind):
             if perms is None:
                 perms = 0o644
-            self.st_mode = S_IFREG | perms
-        elif kind == 'directory':
+            self.st_mode = kind | perms
+        else:
             if perms is None:
                 perms = 0o755
-            self.st_mode = S_IFDIR | perms
-        elif kind == 'symlink':
-            self.st_mode = S_IFLNK | 0o644
-        else:
-            raise AssertionError('unknown kind %r' % kind)
+            self.st_mode = kind | perms
 
 
 class MemoryTransport(transport.Transport):
@@ -111,7 +108,7 @@
 
     def append_file(self, relpath, f, mode=None):
         """See Transport.append_file()."""
-        _abspath = self._abspath(relpath)
+        _abspath = self._resolve_symlinks(relpath)
         self._check_parent(_abspath)
         orig_content, orig_mode = self._files.get(_abspath, (b"", None))
         if mode is None:
@@ -128,16 +125,20 @@
     def has(self, relpath):
         """See Transport.has()."""
         _abspath = self._abspath(relpath)
-        return ((_abspath in self._files)
-                or (_abspath in self._dirs)
-                or (_abspath in self._symlinks))
+        for container in (self._files, self._dirs, self._symlinks):
+            if _abspath in container.keys():
+                return True
+        return False
 
     def delete(self, relpath):
         """See Transport.delete()."""
         _abspath = self._abspath(relpath)
-        if _abspath not in self._files:
+        if _abspath in self._files:
+            del self._files[_abspath]
+        elif _abspath in self._symlinks:
+            del self._symlinks[_abspath]
+        else:
             raise NoSuchFile(relpath)
-        del self._files[_abspath]
 
     def external_url(self):
         """See breezy.transport.Transport.external_url."""
@@ -147,7 +148,7 @@
 
     def get(self, relpath):
         """See Transport.get()."""
-        _abspath = self._abspath(relpath)
+        _abspath = self._resolve_symlinks(relpath)
         if _abspath not in self._files:
             if _abspath in self._dirs:
                 return LateReadError(relpath)
@@ -157,15 +158,20 @@
 
     def put_file(self, relpath, f, mode=None):
         """See Transport.put_file()."""
-        _abspath = self._abspath(relpath)
+        _abspath = self._resolve_symlinks(relpath)
         self._check_parent(_abspath)
         raw_bytes = f.read()
         self._files[_abspath] = (raw_bytes, mode)
         return len(raw_bytes)
 
+    def symlink(self, source, target):
+        _abspath = self._resolve_symlinks(target)
+        self._check_parent(_abspath)
+        self._symlinks[_abspath] = self._abspath(source)
+
     def mkdir(self, relpath, mode=None):
         """See Transport.mkdir()."""
-        _abspath = self._abspath(relpath)
+        _abspath = self._resolve_symlinks(relpath)
         self._check_parent(_abspath)
         if _abspath in self._dirs:
             raise FileExists(relpath)
@@ -183,13 +189,13 @@
         return True
 
     def iter_files_recursive(self):
-        for file in self._files:
+        for file in itertools.chain(self._files, self._symlinks):
             if file.startswith(self._cwd):
                 yield urlutils.escape(file[len(self._cwd):])
 
     def list_dir(self, relpath):
         """See Transport.list_dir()."""
-        _abspath = self._abspath(relpath)
+        _abspath = self._resolve_symlinks(relpath)
         if _abspath != '/' and _abspath not in self._dirs:
             raise NoSuchFile(relpath)
         result = []
@@ -197,7 +203,7 @@
         if not _abspath.endswith('/'):
             _abspath += '/'
 
-        for path_group in self._files, self._dirs:
+        for path_group in self._files, self._dirs, self._symlinks:
             for path in path_group:
                 if path.startswith(_abspath):
                     trailing = path[len(_abspath):]
@@ -207,8 +213,8 @@
 
     def rename(self, rel_from, rel_to):
         """Rename a file or directory; fail if the destination exists"""
-        abs_from = self._abspath(rel_from)
-        abs_to = self._abspath(rel_to)
+        abs_from = self._resolve_symlinks(rel_from)
+        abs_to = self._resolve_symlinks(rel_to)
 
         def replace(x):
             if x == abs_from:
@@ -233,21 +239,25 @@
         # fail differently depending on dict order. So work on copy, fail on
         # error on only replace dicts if all goes well.
         renamed_files = self._files.copy()
+        renamed_symlinks = self._symlinks.copy()
         renamed_dirs = self._dirs.copy()
         do_renames(renamed_files)
+        do_renames(renamed_symlinks)
         do_renames(renamed_dirs)
         # We may have been cloned so modify in place
         self._files.clear()
         self._files.update(renamed_files)
+        self._symlinks.clear()
+        self._symlinks.update(renamed_symlinks)
         self._dirs.clear()
         self._dirs.update(renamed_dirs)
 
     def rmdir(self, relpath):
         """See Transport.rmdir."""
-        _abspath = self._abspath(relpath)
+        _abspath = self._resolve_symlinks(relpath)
         if _abspath in self._files:
             self._translate_error(IOError(errno.ENOTDIR, relpath), relpath)
-        for path in self._files:
+        for path in itertools.chain(self._files, self._symlinks):
             if path.startswith(_abspath + '/'):
                 self._translate_error(IOError(errno.ENOTEMPTY, relpath),
                                       relpath)
@@ -262,13 +272,13 @@
     def stat(self, relpath):
         """See Transport.stat()."""
         _abspath = self._abspath(relpath)
-        if _abspath in self._files:
-            return MemoryStat(len(self._files[_abspath][0]), 'file',
+        if _abspath in self._files.keys():
+            return MemoryStat(len(self._files[_abspath][0]), S_IFREG,
                               self._files[_abspath][1])
-        elif _abspath in self._dirs:
-            return MemoryStat(0, 'directory', self._dirs[_abspath])
-        elif _abspath in self._symlinks:
-            return MemoryStat(0, 'symlink', 0)
+        elif _abspath in self._dirs.keys():
+            return MemoryStat(0, S_IFDIR, self._dirs[_abspath])
+        elif _abspath in self._symlinks.keys():
+            return MemoryStat(0, S_IFLNK)
         else:
             raise NoSuchFile(_abspath)
 
@@ -280,6 +290,12 @@
         """See Transport.lock_write()."""
         return _MemoryLock(self._abspath(relpath), self)
 
+    def _resolve_symlinks(self, relpath):
+        path = self._abspath(relpath)
+        while path in self._symlinks.keys():
+            path = self._symlinks[path]
+        return path
+
     def _abspath(self, relpath):
         """Generate an internal absolute path."""
         relpath = urlutils.unescape(relpath)
@@ -336,6 +352,7 @@
     def start_server(self):
         self._dirs = {'/': None}
         self._files = {}
+        self._symlinks = {}
         self._locks = {}
         self._scheme = "memory+%s:///" % id(self)
 
@@ -344,6 +361,7 @@
             result = memory.MemoryTransport(url)
             result._dirs = self._dirs
             result._files = self._files
+            result._symlinks = self._symlinks
             result._locks = self._locks
             return result
         self._memory_factory = memory_factory