Code review comment for lp:~rls/bzr/transport-link-and-symlink-support

I think this is pretty close; thanks for posting it. Just a few comments:

=== modified file 'bzrlib/tests/per_transport.py'
--- bzrlib/tests/per_transport.py 2010-02-23 07:43:11 +0000
+++ bzrlib/tests/per_transport.py 2010-02-26 04:50:30 +0000
@@ -1083,6 +1083,52 @@
         subdir.stat('./file')
         subdir.stat('.')

+ def test_link(self):
+ from stat import ST_NLINK
+
+ t = self.get_transport()
+
+ source_name = "original_target"
+ link_name = "target_link"
+
+ self.build_tree([source_name], transport=t)
+
+ try:
+ link_result = t.link(source_name, link_name)
+ self.failUnless(link_result)

Returning True for success is a bit strange in Python. Why not just
rely on exceptions to indicate errors?

+
+ self.failUnless(t.has(source_name))
+ self.failUnless(t.has(link_name))
+
+ st = t.stat(link_name)
+ self.failUnlessEqual(st[ST_NLINK], 2)

Good; I think there might be a TestCase method to check they're linked
to each other.

+ except TransportNotPossible:
+ # This transport doesn't do hardlinks
+ return

Probably better to indicate it was skipped: raise TestSkipped(...).

+
+ def test_symlink(self):
+ from stat import S_ISLNK
+
+ t = self.get_transport()
+
+ source_name = "original_target"
+ link_name = "target_link"
+
+ self.build_tree([source_name], transport=t)
+
+ try:
+ link_result = t.symlink(source_name, link_name)
+ self.failUnless(link_result)
+

likewise

+ self.failUnless(t.has(source_name))
+ self.failUnless(t.has(link_name))
+
+ st = t.stat(link_name)
+ self.failUnless(S_ISLNK(st.st_mode))

I think you should actually read the link and check it had what you
expected.

+ except TransportNotPossible:
+ # This transport doesn't do hardlinks
+ return
+
     def test_list_dir(self):
         # TODO: Test list_dir, just try once, and if it throws, stop testing
         t = self.get_transport()

=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py 2010-02-23 07:43:11 +0000
+++ bzrlib/transport/__init__.py 2010-02-26 04:50:30 +0000
@@ -1202,6 +1202,20 @@
         count = self._iterate_over(relpaths, gather, pb, 'stat', expand=False)
         return stats

+ def link(self, source, link_name):
+ """Create a hardlink pointing to source named link_name.
+
+ Return True if successful.
+ """
+ raise errors.TransportNotPossible("Hard links are not supported on %s" % self)
+
+ def symlink(self, source, link_name):
+ """Create a symlink pointing to source named link_name.
+
+ Return True if successful.
+ """
+ raise errors.TransportNotPossible("Symlinks are not supported on %s" % self)
+
     def listable(self):
         """Return True if this store supports listing."""
         raise NotImplementedError(self.listable)

=== modified file 'bzrlib/transport/local.py'
--- bzrlib/transport/local.py 2010-02-23 07:43:11 +0000
+++ bzrlib/transport/local.py 2010-02-26 04:50:30 +0000
@@ -481,7 +481,7 @@
         path = relpath
         try:
             path = self._abspath(relpath)
- return os.stat(path)
+ return os.lstat(path)
         except (IOError, OSError),e:
             self._translate_error(e, path)

This is in LocalTransport.stat, obviously changing it to behave more
like lstat that stat. Since transports never previously expected to
see symlinks perhaps it will not break anything, but on the other hand
perhaps it is a bit surprising that t.stat behaves like lstat. If we
ever actually wanted to follow symlinks, we'd need to add another
method for that.

Perhaps if that behaviour is changed it should be tested.

Alternatively you could add a t.lstat.

I posted an rfc. On the whole I think what you have is fine.

@@ -515,6 +515,32 @@
         except (IOError, OSError),e:
             self._translate_error(e, path)

+ if osutils.hardlinks_good():
+ def link(self, source, link_name):
+ """See Transport.link."""
+ try:
+ os.link(self._abspath(source),
+ self._abspath(link_name))
+ return True
+ except (IOError, OSError), e:
+ self._translate_error(e, source)
+

Similarly to what gzlist said, I think here you should do

  try:
      link = os.link
  except AttributeError, ...
      raise TransportNotPossible(...)

  link(...)

+ def symlink(self, source, link_name):
+ """See Transport.symlink."""
+ abs_link_dirpath = urlutils.dirname(self.abspath(link_name))
+ source_rel = urlutils.file_relpath(
+ urlutils.strip_trailing_slash(abs_link_dirpath),
+ urlutils.strip_trailing_slash(self.abspath(source))
+ )
+
+ try:
+ os.symlink(source_rel, self._abspath(link_name))
+ return True
+ except AttributeError:
+ raise errors.TransportNotPossible("Symlinks are not supported on %s" % self)
+ except (IOError, OSError), e:
+ self._translate_error(e, source_rel)
+
     def _can_roundtrip_unix_modebits(self):
         if sys.platform == 'win32':
             # anyone else?

=== modified file 'bzrlib/transport/sftp.py'
--- bzrlib/transport/sftp.py 2010-02-23 07:43:11 +0000
+++ bzrlib/transport/sftp.py 2010-02-26 04:50:30 +0000
@@ -82,7 +82,7 @@
 else:
     from paramiko.sftp import (SFTP_FLAG_WRITE, SFTP_FLAG_CREATE,
                                SFTP_FLAG_EXCL, SFTP_FLAG_TRUNC,
- CMD_HANDLE, CMD_OPEN)
+ SFTP_OK, CMD_HANDLE, CMD_OPEN)
     from paramiko.sftp_attr import SFTPAttributes
     from paramiko.sftp_file import SFTPFile

@@ -810,10 +810,20 @@
         """Return the stat information for a file."""
         path = self._remote_path(relpath)
         try:
- return self._get_sftp().stat(path)
+ return self._get_sftp().lstat(path)
         except (IOError, paramiko.SSHException), e:
             self._translate_io_exception(e, path, ': unable to stat')

+ def symlink(self, source, link_name):
+ """See Transport.symlink."""
+ try:
+ conn = self._get_sftp()
+ retval = conn.symlink(source, link_name)
+ return (SFTP_OK == retval)
+ except (IOError, paramiko.SSHException), e:
+ self._translate_io_exception(e, link_name,
+ ': unable to create symlink to %r' % (source))
+
     def lock_read(self, relpath):
         """
         Lock the given file for shared (read) access.