[RFC][MERGE] bzrdir phase 3
John A Meinel
john at arbash-meinel.com
Fri Feb 17 02:28:09 GMT 2006
Robert Collins wrote:
> On Thu, 2006-02-16 at 09:39 -0600, John A Meinel wrote:
>
>>> +
>>> +
>>> +class TestIsControlFilename(TestCaseWithWorkingTree):
>>> +
>>> + def validate_tree_is_controlfilename(self, tree):
>>> + """check that 'tree' obeys the contract for is_control_filename."""
>>> + bzrdirname = basename(tree.bzrdir.transport.base[:-1])
>>> + self.assertTrue(tree.is_control_filename(bzrdirname))
>>> + self.assertTrue(tree.is_control_filename(bzrdirname + '/subdir'))
>>> + self.assertFalse(tree.is_control_filename('dir/' + bzrdirname))
>>> + self.assertFalse(tree.is_control_filename('dir/' + bzrdirname + '/sub'))
>> What is the contract for 'is_control_filename', because most likely
>> 'dir/.bzr/sub' would be a control file for a different branch. I don't
>> think we want an outer branch to be controlling an inner branch's meta
>> files.
>
> To add 'dir/.bzr/sub' to this tree, first we have to add 'dir'. If 'dir'
> is a tree, then that triggers the 'cant add a subtree' code path.
>
> The contract is that it returns true IF and only IF the file is part of
> the control meta data bzr maintains.
As long as that is made obvious.
>
>>> +
>>> + def test_dotbzr_is_control_in_cwd(self):
>>> + tree = self.make_branch_and_tree('.')
>>> + self.validate_tree_is_controlfilename(tree)
>>> +
>>> + def test_dotbzr_is_control_in_subdir(self):
>>> + tree = self.make_branch_and_tree('subdir')
>>> + self.validate_tree_is_controlfilename(tree)
>>> +
>> Why are you checking 'subdir' if you don't have an outer tree?
>
> if `cwd` != 'tree_root', its possible to get different results. I found
> a bug by testing that :).
>
Okay. I thought it would be worthwhile to check nested trees as well, I
guess.
...
>>> class cmd_diff(Command):
>>> @@ -1451,11 +1450,29 @@
>>> this command. When the default format has changed you may also be warned
>>> during other operations to upgrade.
>>> """
>>> + # NB: this is used from the class without creating an instance, which is
>>> + # why it does not have a self parameter.
>>> + def get_format_type(typestring):
>>> + """Parse and return a format specifier."""
>>> + if typestring == "metadir":
>>> + return bzrdir.BzrDirMetaFormat1
>>> + msg = "No known bzr-dir format %s. Supported types are: metadir\n" %\
>>> + (typestring)
>>> + raise BzrCommandError(msg)
>>> +
>> Doesn't this need to be declared 'staticmethod'?
>
> No. The comment there should explain it. Its a quirk of the way the
> command classes are used; I haven't spent the time digging into it yet
> to explain it.
>
Looking at it, it is just being passed to Option() as the way to
interpret get_format_type. I understand what it is doing. But I would
recommend that we move it outside of the class, just like all of our
other parsers. Plus it would make it easier to write tests for it in the
future.
>
>>> === modified file 'bzrlib/bzrdir.py'
>>> --- bzrlib/bzrdir.py
>>> +++ bzrlib/bzrdir.py
>>> @@ -21,18 +21,32 @@
>>> """
>>>
>>> from copy import deepcopy
>>> +import os
>>> from cStringIO import StringIO
>>> from unittest import TestSuite
>>> -
>>>
>>> import bzrlib
>>> import bzrlib.errors as errors
>>> from bzrlib.lockable_files import LockableFiles
>>> from bzrlib.osutils import safe_unicode
>>> +from bzrlib.osutils import (
>>> + abspath,
>>> + pathjoin,
>>> + safe_unicode,
>>> + sha_strings,
>>> + sha_string,
>>> + )
>> Why do you have the closing ')' line up with the entries, rather than
>> lining up with the opening '('. This isn't the only place where occurs
>> (as I recall the test name listing does this as well). I think it would
>> look better with the () lined up.
>
> In Launchpad the coding style is that such things line up with the
> content like I do here, I don't recall if thats PEP8 or just a visual
> aid. Regardless of that we can choose our destiny... but I like it
> lining up with the indented entries, its less distruptive to the eye
> while still being trivial to add lines to.
>
You don't think:
from bzrlib.osutils import (
abspath,
pathjoin,
safe_unicode,
sha_strings,
sha_string,
)
Looks better? I guess I'm used to C++ braces lining up to make it clear
what the section is. I always use either:
if (something) {
}
or
if (something really long that it doesn't fit on a line)
{
}
I think it would look funny to do
if (something)
{
}
But maybe that is just me.
>
...
>> The problem is 'BzrFormat6.needs_format_upgrade(BzrFormat4) would return
>> True, even though it is 'downgrading' the format.
>
> Thats correct though: if someone wants to change the format to be
> BzrFormat4, the will need to run a format converter.
I guess calling it 'upgrade' is confusing. Calling it something like
"needs_conversion" would be more accurate.
...
>> Small typo. And also why I thought the target format should be the one
>> who knows how to upgrade.
>
> Its not quite ready for plugins - I didn't want to write the
> infrastructure code with no examples, but as I'm not working on
> versioned file [well, after this patch gets beaten on], I should have
> the use cases to drive it to completion.
>
> Hmm, wiki is offline - the spec is there. So from memory, we essentially
> have pairs of changes that need to occur: (FROMFORMAT, TOFORMAT). And if
> we gather those all up we can build a graph. Note that its a full graph,
> not a DAG, because we have a generic converter 'read everything, write
> everything' which allows downgrades, and we should also be able to write
> specific reverters if needed. This is only relevant for bleeding edge
> stuff of course - but its essential to be able to get back to the
> 'stable' format if you upgraded too early, or to play around.
>
> So a plugin while it knows enough to convert *from* any given format,
> also needs to be able to tell bzr how to convert *to* the mainline
> format again. So being owned by either the target or the source is
> wrong. So at the moment, I've given it to the source but passed in the
> eventual target as well. That allows me to convert the internals from
> hard coded results into a conversion graph lookup - and when that is
> done plugins will do:
>
> bzrdir.BzrDir.Converter.register_converter(from_format, to_format,
> converter_factory)
I guess it seems like it should just be a generic set of from->to rather
than a member of from or to, but it sounds like you are getting there
anyway.
>
> i.e. for the current bound branch format you might offer a converter
> from them to the metadir, using a different branch output to preserve
> the bound branch data
> Converter.register_converter(BoundBzrDirFormat, BzrMetaFormat1,
> ConvertBoundBranchToMetaBoundBranch)
>
>
> class ConvertBoundBranchToMetaBoundBranch(ConvertFormat6ToMeta):
> ...
>
By the way, I'm not sure if you realized, but I don't have a specific
branch format for 'BoundBranch'. It is simply a new property of any
branch, and the only need for a new format is to prevent old clients
from writing directly to the branch, and not honoring the new property's
contract (while writing and testing it, I did that fairly often, since
the bound branch was ~/dev/bzr/bzr-bound-branch/bzr but I occasionally
just typed 'bzr foo', and then wondered what was wrong with my code that
the remote branch wasn't updated :).
> that would output a meta 1 bzr dir, format 7 repo, format 3 working tree
> and BoundBranchFormat format branch.
>
> But importantly: the source format is provided by your plugin, the
> target bzrdir format is -not-.
>
It just depends on how you look at it. You provide *a* format. It could
be a source or a target. Depending on which way you are upgrading.
>
> And finally, the reason the api is on the source format is that that is
> the order we walk the conceptual graph: SOURCE->OUTPUT, rinse and repeat
> until we are done.
>
I kind of see that, and its certainly how it has been done. But I could
also see finding the path from source to output, (say with graph
search), and then apply the proper converters.
When I provide a plugin with a new format, it would make sense that I
would know how to convert from the main format to my custom format. But
it would be very wrong of me to assume that the main format would know
how to convert my custom format.
Hence why I think it shouldn't be part of either end, and should just be
a general registry of arcs in a graph.
...
> We can start using the Meta-1 format when we decide its time to start
> guarantee support for branches in it - we just freeze the format and
> bump to a new one internally. I think letting the lock code get in is
> good, but I'd like an intermediary format between the weave and the knit
> transition. So, we could start using this now I think.
>
I would also like to see at least 1 format bump before knits. I would
like to get a couple more things into Meta-1 format. I actually have
written escaped stores in a moderately robust manner so someone can use
a baz->bzr branch on windows, and I would like to see bound branches in
Meta-1.
Otherwise we would again have some clients that support these things,
and some clients that don't. All which *think* they support the same format.
By the way, for escaped stores, we could make it a property of the
format (All metadir format 1 branches are escaped), or we could make it
an orthogonal property (format 1 branches with escaped=True have escaped
stores).
>
>>> @@ -80,7 +106,6 @@
>>>
>>> print
>>> print 'branch history:'
>>> - history = b.revision_history()
>>> revno = len(history)
>>> print ' %8d revision%s' % (revno, plural(revno))
>>> committers = {}
>>> @@ -107,7 +132,7 @@
>>> print
>>> print 'revision store:'
>>> c, t = b.repository.revision_store.total_size()
>>> - print ' %8d revisions' % c
>>> + print ' %8d revision%s' % (c, plural(c))
>>> print ' %8d kB' % (t/1024)
>> Isn't there supposed to be a special way of doing pluralization for
>> internationalization work? (Rather than just hard-coding 's' for
>> English). Speaking of which, is there some time when we are planning on
>> going through the code and adding _() around strings so that we can do
>> i18n work?
>> Perhaps that is an 0.999 version. But it probably should be done. (And
>> is going to lead to a lot of conflicts).
>
> Agreed, but not for this pastch :). gettext has plural forms yes, and
> fortunately I have a handy supply of folk that know all about that just
> over in the rosetta developers :)
>
Certainly not for this patch. I was just curious if there was a specific
plan for using gettext.
>
....
>>> + self.assertEqual("\r \r", stderr.getvalue())
>>>
>> This looks like a syntax error. Which means the code is not being run.
>
> nah, its your terminal or our diff getting confused.. Thats the literal
> output from a progress bar.
I figured out the problem. It is just a really long line, so I couldn't
see the end of it. And I didn't think to scroll.
I think you violated 80 characters yourself. :) But it would be much
easier to read as '\r' + (' '*75) + '\r'. And easier to maintain. Rather
than trying to count the whitespace.
>
>
>> ...
>>
>>> + def __init__(self, url, format):
>>> + self.format = format
>>> + self.bzrdir = BzrDir.open_unsupported(url)
>>> + if self.bzrdir.root_transport.is_readonly():
>>> + raise errors.UpgradeReadonly
>>> + self.transport = self.bzrdir.root_transport
>>> + self.convert()
>> I generally feel that having an action occur in a constructor is not
>> desirable.
>
> That was already like that, so I'm going to plead 'SEP' here. But I
> agree in terms of rules of thumbs.
>
>> Overall, it looks good. I'd like a few things addressed. But it looks
>> very close to mergeable.
>
> Cool.
>
> Rob
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060216/17b71c2b/attachment.pgp
More information about the bazaar
mailing list