[MERGE (0.12)] Bundle format 0.9

[John Arbash Meinel]

Aaron Bentley wrote:
> John Arbash Meinel wrote:
>>> Aaron Bentley wrote:
>>> So before we can merge this, we need to decide as a group if we want the
>>> 'parent_check' patch, and I haven't heard anything from Robert or Martin
>>> yet.
> 
> I've yanked those changes, so they can be considered separately.
> 
>>> Also, if we are bumping the format, shouldn't this format include
>>> Signatures? Since that is the only *functionality* missing. (The fact
>>> that they are slow to use is an optimization, not a functionality flaw)
> 
> I'm focused on getting bzr ready for nested trees.  It's not really a
> goal to add other missing functionality.
> 
>>> You also have some trailing whitespace here and there. It might be nice
>>> to clean that up first.
> 
> Okay.  But I don't think it matters much, unless you've configured your
> editor to make it ugly.
> 
>>> v- This file needs a Copyright comment and a summary doc string. (And if
>>> the other serializer formats don't have one, then they need them added
>>> as well).
> 
> Done
> 
> 
>>> +        f.write(BUNDLE_HEADER)
>>> +        f.write('0.9\n')
>>> +        f.write('#\n')
>>>
>>> ^ it is generally better to do:
>>> f.write(BUNDLE_HEADER + '0.9\n#\n')
>>> Or at least
>>> f.write(BUNDLE_HEADER)
>>> f.write('0.9\n#\n')
>>>
>>> Since the system call overhead is moderately big. (I realize these
>>> aren't writing over a Transport at least).
> 
> Well, this was in imitation of 0.8, but okay.
> 
>>> v- I don't think we want to call the class StrictTestament2 but have the
>>> header string be 'bazaar-ng testament version 3 strict\n'
>>>
>>> 1) We want to switch to 'bazaar' instead of 'bazaar-ng'.
>>> 2) If we are calling it version 3, I think it is best to call the class
>>> ...3 to avoid confusion.
> 
> Okay.
> 
>>> +    def _escape_path(self, path):
>>> +        assert not contains_linebreaks(path)
>>> +        if path == '':
>>> +            path = '.'
>>> +        return unicode(path.replace('\\', '/').replace(' ', '\ '))
>>>
>>> ^- Do we need replace('\\', '/') it seems that by this time, '\\' should
>>> not exist in the path.
> 
> I was just copying what other testament formats do.  I'd rather leave it
> this way, in case it actually is doing something.
> 
>>> v- this should be split across multiple lines.
>>>
>>> -from bzrlib import inventory, treebuilder
>>> +from bzrlib import bzrdir, errors, inventory, repository, treebuilder
> 
> Well, I like the aesthetics of keeping it on one line until it exceeds
> 79 chars, but okay.
> 
>>> v- You should probably update the Copyright line, but not delete it.
>>> (Though I'm assuming the delete was actually accidental)
> 
> Yeah.
> 
> I've attached two patches: one showing my latest changes, one showing
> the difference from bzr.dev
> 
> Aaron


Some of your whitespace fixes weren't actually from your original
changes. But no big deal. I find trailing whitespace unhygienic. Not
strictly bad, but not good either. (especially problematic is that MTA's
feel justified in removing it).



V- If you are going to fix copyright stuff, it should be
Copyright (C) <years> Canonical Ltd

There is no Canonical Development company. But that is also a separate
issue. I just wanted you to know about it. It was brought up in a bug
report recently. (One of those easy bugs to squash, while adding a test
that makes sure copyright exists in every file)


=== modified file 'bzrlib/bundle/serializer/v08.py'
--- bzrlib/bundle/serializer/v08.py	2006-09-22 02:08:10 +0000
+++ bzrlib/bundle/serializer/v08.py	2006-09-26 13:15:04 +0000
@@ -1,4 +1,4 @@
-# (C) 2005 Canonical Development Ltd
+# (C) 2005, 2006 Canonical Development Ltd


You've satisfied my critiques. +1

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060927/bd4ea55b/attachment.pgp