[merge] updated store-escaping for windows

[Martin Pool]

On 12/04/2006, at 2:39 PM, Robert Collins wrote:
>
> === modified file 'a/bzrlib/store/versioned/__init__.py'
> ...
> while we are here, we may as well make the imports alpha-betically
> sorted by package.
>
>
> +class TestBranchEscaping(TestCaseWithTransport):
> +    """Test a branch can be correctly stored and used on a vfat-like
> transport
> +
> +    Makes sure we have proper escaping of invalid characters, etc.
> +
> +    It'd be better to test all operations on the
> FakeVFATTransportDecorator,
> +    but working trees go straight to the os not through the Transport
> layer.
> +    Therefore we build some history first in the regular way and then
> +    check it's safe to access for vfat.
> +    """
> +
> +    FOO_ID = 'foo<:>ID'
> +    REV_ID = 'revid-1'
> +
>
>
> Couple of nits here - this class has one test case and a setup - that
> feels weird (as setUp is for reusing a fixture. Also, we've agreed
> previously not to use class scope variables for anything other than
> class variables - and the only reason you need these symbols between
> methods is because there is a setUp :).

Maybe I'm misremembering - I though we agreed to just not use them  
for defaults of fields that will later be instance variables.  Here  
they are essentially acting as constants defined at class scope  
(hence the names)

> I'd be inclined to have a single
> test method at this point, unless you are planning to do another test
> with the exact same fixture. For the class symbols, please consider
> putting them in via the test method/setUp - i.e. self.FOO_ID =
> 'foo<:>ID'.

I thought splitting it into separate setup and run phases might be  
clearer, even if they're not used separately.

> finally, you've added a call to the transport api - the unix modebits
> one - but not tests for it. Should there be a test for it (even though
> its private, having it is now required ..)

How about this:

=== modified file 'a/bzrlib/tests/__init__.py'
--- a/bzrlib/tests/__init__.py
+++ b/bzrlib/tests/__init__.py
@@ -327,9 +327,9 @@
      def assertTransportMode(self, transport, path, mode):
          """Fail if a path does not have mode mode.

-        If modes are not supported on this platform, the test is  
skipped.
-        """
-        if sys.platform == 'win32':
+        If modes are not supported on this transport, the assertion  
is ignored.
+        """
+        if not transport._can_roundtrip_unix_modebits():
              return
          path_stat = transport.stat(path)
          actual_mode = stat.S_IMODE(path_stat.st_mode)

Now many tests that check modes will switch on this.

We could add a test that it returns True on unix local transports but  
that doesn't seem very likely to find bugs.

-- 
Martin