[merge] updated store-escaping for windows

[Robert Collins]

On Tue, 2006-04-11 at 18:57 +1000, Martin Pool wrote:
> This change, based on John's previous work, changes Knit stores to avoid
> characters in filenames that can't be safely stored on Windows.
> (Basically everything but lowercase letters, digits, and a few
> punctuation characters.)
> 
> - Add a FakeVFAT transport decorator that lets us check some of these 
>   constraints.  (Eventually it might be nice to be able to run
>   WorkingTree tests through this too to trap Windows-specific bugs
>   even when testing on Unix.)
> 
> - Add an 'escaped' parameter to the TransportStore.

Some feedback

the propogation of _cloning in some function calls is undesirable - that
was a hack to allow the legacy formats - the non metadir ones - to
privately signal that they did not need to create their data structures.
Certainly it shouldn't be part of the public api.

from bzrlib.symbol_versioning import *
-from bzrlib.trace import mutter
+from bzrlib.symbol_versioning import deprecated_method, zero_eight
+from bzrlib.trace import mutter, note

The '+from bzrlib.symbol_versioning import deprecated_method,
zero_eight' line is redundant - see the line above. That uses __all__ in
the symbol_version file as a very nice way of exposing such common
symbols.

+
+    # XXX: Is this actually called?
+    @needs_read_lock
+    def copy(self, destination):

if you add a deprecated_method decorator there and a docstring, you'll
_know_ if it gets called ;)



@@ -206,7 +206,10 @@
 
     def has_id(self, fileid, suffix=None):
         """See Store.has_id."""
-        return self._transport.has_any(self._id_to_names(fileid,
suffix))
+        paths = self._id_to_names(fileid, suffix)
+        if not self._transport.has_any(paths):
+            return False
+        return True


has_any returns True or False - what does explicitly testing it in this
method gain ? Also the temp variable here, whats it for ? Its used
precisely once, to pass to the method that returns the exact value this
method needs to return.


=== 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 :). 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'.


+    def test_forbidden_chars(self):
+        transport = self.get_vfat_transport('.')
+        self.assertRaises(ValueError, transport.has, "<NU>")
+
+
+
 class BadTransportHandler(Transport):

PEP8 - 2 lines of vertical whitespace not 3 please.



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 ..)


Rob







-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060412/873ff7e5/attachment.pgp