[RFC][MERGE] bzrdir phase 3

[Robert Collins]

On Thu, 2006-02-16 at 20:28 -0600, John A Meinel wrote:

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

I'll move it out of the class then. I'm not sure where to put it though
- it seems awfully kludgy for options.py to centralise everything. For
now I'll put it above the class in builtins.py. Note that it doesn't
actually make testing any easier or harder ;0.

> 
> > 
> >>> === 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)
>   {
> }

Thats not equivalent though, and I agree that that would be funny.
if (something) {
                something++;
                }

Is the equivalent layout, and I think it makes the block very visible to
the eye.


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

ok, s/update/conversion/ being done.

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

Ok. Well we can use the metadir format as the time its 'active' then.
And avoid an explicit bump for it.

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

Sure. I was highlighting that in response to your comment about the
target format owning the converter.

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

But.. its also wrong to assume that as the mainline evolves it will know
how to convert *from* your 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.

The driver data will be. The api will still use a call on the source,
because a simple equality test is not sufficient, and being sufficient
would require breaking abstraction.

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

I am in favour of 'all metadir format 1 branches are escaped'. I'll
happily +1 an addition to the format6-meta1 converter to escape the
stores.



> Certainly not for this patch. I was just curious if there was a specific
> plan for using gettext.

Not yet AFAIK. I think we should just bite the bullet and put it in, and
let translations come in incrementally.

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

Will do.

Do I have +1 with the above done ?

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: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060217/d14a7a4c/attachment.pgp