[MERGE (0.12)] Bundle format 0.9
John Arbash Meinel
john at arbash-meinel.com
Wed Sep 27 20:32:39 BST 2006
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
> 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).
>>> + 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
>>> 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.
>>> + 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)
> I've attached two patches: one showing my latest changes, one showing
> the difference from bzr.dev
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
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060927/bd4ea55b/attachment.pgp
More information about the bazaar