launchpad-dev team mailing list archive

[Bjorn Tillenius <bjorn@xxxxxxxxxxxxx>]

On Wed, Dec 09, 2009 at 02:33:19PM -0000, noreply@xxxxxxxxxxxxx wrote:
> Merge authors:
>   Aaron Bentley (abentley)
> Related merge proposals:
>   https://code.launchpad.net/~abentley/launchpad/sftp-ls/+merge/15580
>   proposed by: Aaron Bentley (abentley)
>   review: Approve - Michael Hudson (mwhudson)
> ------------------------------------------------------------
> revno: 10012 [merge]
> committer: Launchpad Patch Queue Manager <launchpad@xxxxxxxxxxxxxxxxx>
> branch nick: launchpad
> timestamp: Wed 2009-12-09 14:30:52 +0000
> message:
>   [r=mwhudson][ui=none] Allow listing sftp directories on
>   	MemoryTransport
> modified:
>   lib/lp/codehosting/sftp.py
>   lib/lp/codehosting/tests/test_sftp.py


> === modified file 'lib/lp/codehosting/sftp.py'
> --- lib/lp/codehosting/sftp.py	2009-10-29 00:51:48 +0000
> +++ lib/lp/codehosting/sftp.py	2009-11-27 21:59:26 +0000
> @@ -20,6 +20,7 @@
>      ]
>  
>  
> +from copy import copy
>  import errno
>  import os
>  import stat
> @@ -319,6 +320,10 @@
>          """
>          for stat_result, filename in zip(stat_results, filenames):
>              shortname = urlutils.unescape(filename).encode('utf-8')
> +            stat_result = copy(stat_result)
> +            for attribute in ['st_uid', 'st_gid', 'st_mtime', 'st_nlink']:
> +                if getattr(stat_result, attribute, None) is None:
> +                    setattr(stat_result, attribute, 0)

This code is very unobvious to me. Why do you have to replace None with
0? A comment would be useful here.


>              longname = lsLine(shortname, stat_result)
>              attr_dict = self._translate_stat(stat_result)
>              yield (shortname, longname, attr_dict)
> 
> === modified file 'lib/lp/codehosting/tests/test_sftp.py'
> --- lib/lp/codehosting/tests/test_sftp.py	2009-10-31 12:07:16 +0000
> +++ lib/lp/codehosting/tests/test_sftp.py	2009-12-08 16:24:12 +0000
> @@ -3,12 +3,15 @@
>  
>  """Tests for the transport-backed SFTP server implementation."""
>  
> +from __future__ import with_statement
> +from contextlib import closing
>  import os
>  import unittest
>  
>  from bzrlib.tests import TestCaseInTempDir
>  from bzrlib import errors as bzr_errors
>  from bzrlib.transport import get_transport
> +from bzrlib.transport.memory import MemoryTransport
>  from bzrlib import urlutils
>  
>  from twisted.conch.ssh import filetransfer
> @@ -579,6 +582,39 @@
>          deferred = self.sftp_server.openDirectory(nonexistent)
>          return self.assertFailure(deferred, filetransfer.SFTPError)
>  
> +    def test_openDirectoryMemory(self):
> +        """openDirectory works on MemoryTransport."""
> +        transport = MemoryTransport()
> +        transport.put_bytes('hello', 'hello')
> +        sftp_server = TransportSFTPServer(AsyncTransport(transport))
> +        deferred = sftp_server.openDirectory('.')
> +        def check_directory(directory):
> +            with closing(directory):
> +                names = [entry[0] for entry in directory]
> +            self.assertEqual(['hello'], names)
> +        return deferred.addCallback(check_directory)
> +
> +    def test__format_directory_entries_with_MemoryStat(self):
> +        """format_directory_entries works with MemoryStat.
> +
> +        MemoryStat lacks many fields, but format_directory_entries works
> +        around that.
> +        """
> +        t = MemoryTransport()
> +        stat_result = t.stat('.')
> +        entries = self.sftp_server._format_directory_entries(
> +            [stat_result], ['filename'])
> +        self.assertEqual(list(entries), [
> +            ('filename', 'drwxr-xr-x    0 0        0               0 '
> +             'Jan 01  1970 filename',
> +             {'atime': 0,
> +              'gid': 0,
> +              'mtime': 0,
> +              'permissions': 16877,
> +              'size': 0,
> +              'uid': 0})])
> +        self.assertIs(None, getattr(stat_result, 'st_mtime', None))

What's this last assert for? Why don't you care whether stat_result has
an st_mtime attribute?



-- 
Björn Tillenius | https://launchpad.net/~bjornt