Merge lp:~mnn282/bzr/sftp-unsupported-operation-more-info into lp:bzr

[mnn]

Unsupported operations now have more information in SFTP transport.

[Martin Packman (gz)]

Idea of adding a new exception here seems reasonable.

You'll want to add it inside _translate_io_exception rather than adding a special separate branch before hand. That might mean tweaking that method a little, but it's private so there's no reason to to adapt it.

You want some tests as well, you can probably just test the translation itself rather than trying to fake out paramiko to raise the exception you're observing.

review: Needs Fixing

[lp:~mnn282/bzr/sftp-unsupported-operation-more-info]

6546. By mnn on 2012-07-27

Moved handling of unsupported operations into SFTPTransport._translate_io_exception, which has now optional argument 'operation'

SFTPTransport._translate_io_exception and Transport._translate_error are now static methods

[mnn (mnn282)]

Martin Packman: So, the handling has been moved into _translate_io_exception. I also added tests, as you requested.

I was told on IRC that I should make _translate_io_exception static/class method, so tests actually won't be depended on SFTP transport itself - I had to make Transport._translate_error static too, because _translate_io_exception was invoking it.

[Martin Packman (gz)]

Thanks for making those changes, not all the suggestions I made were completely correct so I hope you don't mind poking some of the minor details again.

Style nit, two lines between top level things in modules, see:
<http://www.python.org/dev/peps/pep-0008/#blank-lines>

+ self.message = "\n" + message

So, I said 'msg' wasn't a great name, but annoyingly 'message' is worse due to some legacy Python exception stuff. How about 'description' or something?

Also, having english text as a param prevents that part from being translated, but this isn't too important.

Including the path, and just using the operation name rather than the text form would work for everything but the symlink/rename case, which could just be an optional target_path param to the exception.

+ self.assertEquals(e.message, "\nunable to symlink")
\ No newline at end of file

As with the other mp, this needs a terminal newline or bt.test_source fails.

+ @staticmethod
+ def _translate_io_exception(e, path, more_info='',
...
- self._translate_error(e, path, raise_generic=False)
+ Transport._translate_error(e, path, raise_generic=False)

The neat way around needing the Transport import for this is to instead do:

    @classmethod
    def _translate_io_exception(cls, e, path, more_info='',
...
        cls._translate_error(e, path, raise_generic=False)

Leaving _translate_error as @staticerror is fine.

+ # raise errors.TransportNotPossible(more_info)

Just delete this.

review: Needs Fixing

[lp:~mnn282/bzr/sftp-unsupported-operation-more-info]

6547. By mnn on 2012-07-31

- SFTPTransport._translate_io_exception changed to @classmethod
- changed TransportOperationNotSupported.message to description

[mnn (mnn282)]

> Also, having english text as a param prevents that part from being translated, but this isn't too important.
But other exception handling code in SFTPTransport has only english text as parameter too.

> Including the path, and just using the operation name rather than the text form would work for everything but the symlink/rename case, which could just be an optional target_path param to the exception.
This would require altering _translate_io_exception parameters even more.

[Martin Packman (gz)]

> But other exception handling code in SFTPTransport has only english text as
> parameter too.

Right I wouldn't worry to much about doing this.

> This would require altering _translate_io_exception parameters even more.

I think passing the path to the exception is potentially useful, and shouldn't mean too much extra fiddling.

Unmerged revisions

6547. By mnn on 2012-07-31

- SFTPTransport._translate_io_exception changed to @classmethod
- changed TransportOperationNotSupported.message to description

6546. By mnn on 2012-07-27

Moved handling of unsupported operations into SFTPTransport._translate_io_exception, which has now optional argument 'operation'

SFTPTransport._translate_io_exception and Transport._translate_error are now static methods

6545. By mnn on 2012-07-26

Display more information for TransportOperationNotSupported (for rename - path from and path to)

6544. By mnn on 2012-07-25

Unsupported operations now have more information in SFTP transport

- stubs for unsupported transport operations (TransportOperationNotSupported) - used by rename operations in SFTP transport

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2012-07-19 19:27:22 +0000
+++ bzrlib/errors.py	2012-07-31 15:32:30 +0000
@@ -1619,6 +1619,16 @@
         self.traceback_text = ''.join(traceback_strings)
 
 
+class TransportOperationNotSupported(TransportError):
+
+    _fmt = "Transport operation is not supported: %(operation)s%(description)s"
+
+    def __init__(self, operation, description=None):
+        if description != None:
+            self.description = "\n" + description
+        self.operation = operation
+
+
 # A set of semi-meaningful errors which can be thrown
 class TransportNotPossible(TransportError):
 

=== modified file 'bzrlib/tests/test_sftp_transport.py'
--- bzrlib/tests/test_sftp_transport.py	2012-02-23 23:26:35 +0000
+++ bzrlib/tests/test_sftp_transport.py	2012-07-31 15:32:30 +0000
@@ -497,3 +497,40 @@
         # No prompts should've been printed, stdin shouldn't have been read
         self.assertEquals("", stdout.getvalue())
         self.assertEquals(0, ui.ui_factory.stdin.tell())
+
+class TestTranslateIOException(TestCase):
+    """Test that unsupported operation exception from Paramiko is correctly
+    translated into TransportOperationNotSupported."""
+
+    def test_rename_translation(self):
+        """Test unsupported rename operation by matching raised exception"""
+
+        e = self.assertRaises(
+            errors.TransportOperationNotSupported,
+                          _mod_sftp.SFTPTransport._translate_io_exception,
+                          IOError("Operation unsupported"), "",
+                          "unable to rename", operation="rename")
+        self.assertEquals(e.operation, "rename")
+        self.assertEquals(e.message, "\nunable to rename")
+
+    def test_readlink_translation(self):
+        """Test unsupported readlink operation by matching raised exception"""
+
+        e = self.assertRaises(
+            errors.TransportOperationNotSupported,
+                          _mod_sftp.SFTPTransport._translate_io_exception,
+                          IOError("Operation unsupported"), "",
+                          "unable to readlink", operation="readlink")
+        self.assertEquals(e.operation, "readlink")
+        self.assertEquals(e.message, "\nunable to readlink")
+
+    def test_symlink_translation(self):
+        """Test unsupported symlink operation by matching raised exception"""
+
+        e = self.assertRaises(
+            errors.TransportOperationNotSupported,
+                          _mod_sftp.SFTPTransport._translate_io_exception,
+                          IOError("Operation unsupported"), "",
+                          "unable to symlink", operation="symlink")
+        self.assertEquals(e.operation, "symlink")
+        self.assertEquals(e.message, "\nunable to symlink")

=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py	2012-03-13 17:25:29 +0000
+++ bzrlib/transport/__init__.py	2012-07-31 15:32:30 +0000
@@ -244,7 +244,7 @@
     def fdatasync(self):
         """Force data out to physical disk if possible.
 
-        :raises TransportNotPossible: If this transport has no way to 
+        :raises TransportNotPossible: If this transport has no way to
             flush to disk.
         """
         raise errors.TransportNotPossible(
@@ -321,7 +321,7 @@
     #       where the biggest benefit between combining reads and
     #       and seeking is. Consider a runtime auto-tune.
     _bytes_to_read_before_seek = 0
-    
+
     hooks = TransportHooks()
 
     def __init__(self, base):
@@ -330,7 +330,8 @@
         (self._raw_base, self._segment_parameters) = (
             urlutils.split_segment_parameters(base))
 
-    def _translate_error(self, e, path, raise_generic=True):
+    @staticmethod
+    def _translate_error(e, path, raise_generic=True):
         """Translate an IOError or OSError into an appropriate bzr error.
 
         This handles things like ENOENT, ENOTDIR, EEXIST, and EACCESS
@@ -1631,7 +1632,7 @@
 
 def get_transport_from_url(url, possible_transports=None):
     """Open a transport to access a URL.
-    
+
     :param base: a URL
     :param transports: optional reusable transports list. If not None, created
         transports will be added to the list.

=== modified file 'bzrlib/transport/sftp.py'
--- bzrlib/transport/sftp.py	2011-12-19 13:23:58 +0000
+++ bzrlib/transport/sftp.py	2012-07-31 15:32:30 +0000
@@ -55,6 +55,7 @@
     _file_streams,
     ssh,
     ConnectedTransport,
+    Transport
     )
 
 # Disable one particular warning that comes from paramiko in Python2.5; if
@@ -670,8 +671,9 @@
         _file_streams[self.abspath(relpath)] = handle
         return FileFileStream(self, relpath, handle)
 
-    def _translate_io_exception(self, e, path, more_info='',
-                                failure_exc=PathError):
+    @classmethod
+    def _translate_io_exception(cls, e, path, more_info='',
+                                failure_exc=PathError, operation=None):
         """Translate a paramiko or IOError into a friendlier exception.
 
         :param e: The original exception
@@ -683,9 +685,10 @@
                            no more information.
                            If this parameter is set, it defines the exception
                            to raise in these cases.
+        :param operation: Operation that failed (needs to check if it's supported)
         """
         # paramiko seems to generate detailless errors.
-        self._translate_error(e, path, raise_generic=False)
+        Transport._translate_error(e, path, raise_generic=False)
         if getattr(e, 'args', None) is not None:
             if (e.args == ('No such file or directory',) or
                 e.args == ('No such file',)):
@@ -703,7 +706,7 @@
                 or getattr(e, 'errno', None) == errno.ENOTEMPTY):
                 raise errors.DirectoryNotEmpty(path, str(e))
             if e.args == ('Operation unsupported',):
-                raise errors.TransportNotPossible()
+                raise errors.TransportOperationNotSupported(operation, more_info)
             mutter('Raising exception with args %s', e.args)
         if getattr(e, 'errno', None) is not None:
             mutter('Raising exception with errno %s', e.errno)
@@ -732,7 +735,7 @@
                               self._remote_path(rel_to))
         except (IOError, paramiko.SSHException), e:
             self._translate_io_exception(e, rel_from,
-                    ': unable to rename to %r' % (rel_to))
+                    ': unable to rename to %r' % (rel_to), "rename")
 
     def _rename_and_overwrite(self, abs_from, abs_to):
         """Do a fancy rename on the remote server.
@@ -746,7 +749,7 @@
                          unlink_func=sftp.remove)
         except (IOError, paramiko.SSHException), e:
             self._translate_io_exception(e, abs_from,
-                                         ': unable to rename to %r' % (abs_to))
+                    ': unable to rename to %r' % (abs_to), operation="rename")
 
     def move(self, rel_from, rel_to):
         """Move the item at rel_from to the location at rel_to"""
@@ -760,7 +763,8 @@
         try:
             self._get_sftp().remove(path)
         except (IOError, paramiko.SSHException), e:
-            self._translate_io_exception(e, path, ': unable to delete')
+            self._translate_io_exception(e, path, ': unable to delete',
+                         operation="delete")
 
     def external_url(self):
         """See bzrlib.transport.Transport.external_url."""
@@ -793,7 +797,8 @@
         try:
             return self._get_sftp().rmdir(path)
         except (IOError, paramiko.SSHException), e:
-            self._translate_io_exception(e, path, ': failed to rmdir')
+            self._translate_io_exception(e, path, ': failed to rmdir',
+                        operation="rmdir")
 
     def stat(self, relpath):
         """Return the stat information for a file."""
@@ -809,7 +814,8 @@
         try:
             return self._get_sftp().readlink(path)
         except (IOError, paramiko.SSHException), e:
-            self._translate_io_exception(e, path, ': unable to readlink')
+            self._translate_io_exception(e, path, ': unable to readlink',
+                    operation="readlink")
 
     def symlink(self, source, link_name):
         """See Transport.symlink."""
@@ -823,7 +829,8 @@
                 )
         except (IOError, paramiko.SSHException), e:
             self._translate_io_exception(e, link_name,
-                                         ': unable to create symlink to %r' % (source))
+                    ': unable to create symlink to %r' % (source),
+                    operation="symlink")
 
     def lock_read(self, relpath):
         """
@@ -884,7 +891,7 @@
             return SFTPFile(self._get_sftp(), handle, 'wb', -1)
         except (paramiko.SSHException, IOError), e:
             self._translate_io_exception(e, abspath, ': unable to open',
-                failure_exc=FileExists)
+                failure_exc=FileExists, operation="open")
 
     def _can_roundtrip_unix_modebits(self):
         if sys.platform == 'win32':