[MERGE] Stacking policy

[Ian Clatworthy]

Aaron Bentley wrote:
> This patch implements policy for stacking branches.

Hooray!

> I have
> not implemented a way to turn this behavior off, but I certainly can.

I'm sure you've thought about this a lot more than I have. My gut feel
is that we need this. For example, if I'm about to go online - while
travelling say, I might want to ensure I have all revisions local,
regardless of some default policy about stacking, yes?

I wonder if there is some inherit tension between the different
stacked-branches use cases? I can see myself wanting stacking when
pushing to LP all the time. I'm less keen on stacking while
branching, though I agree it's the best choice for large projects.
I *think* I'm ok with everything I've seen so far but I wonder whether
I'm missing some corner case where branching after a push won't behave
like I hope it will.

> Per-bzrdir configurations live in our standard metadir format 1.  This
> could be considered a format change, but I would prefer not to do that.
>  Existing clients will ignore it, but this seems reasonable, because
> they can't stack anyhow.  So I don't think there are significant
> compatibility concerns.

I tend to agree. In the interest of full transparency though, I know
Robert would question this. Will everything Just Work if a user has
both an old and a new version of bzr installed on their computer?
If the user creates a branch stacked on another using the newer
version, will running the old version on that branch work? If not,
will it fail gracefully?

> Branches should be accessible across multiple formats.  Launchpad itself
> uses http, sftp and bzr+ssh.  To support this in a host-agnostic way,
> the stacked-on branch may now be a relative or absolute path.  Paths are
> treated the way browsers treat paths encountered on web pages; relative
> paths are joined to the current path and absolute paths are joined to
> the current host/protocol.  This allows a single on-disk stack-on
> location to work no matter which protocol is used to access the branch.

Nice.

Some initial review feedback below. I'm not yet ready to vote but,
the issues above aside, only tweaks so far.

> -    def clone(self, url, revision_id=None, force_new_repo=False):
> +    def clone(self, url, revision_id=None, force_new_repo=False,
> +              preserve_stacking=False):

You've updated the docstring for clone_on_transport. I think you
should for clone as well.

>      for a branch that is being created.  The most basic policy decision is
>      whether to create a new repository or use an existing one.
>      """
> +    def __init__(self, stack_on, stack_on_pwd):
> +        """Constructor.

There seems a small risk of breaking plugins by making the parameters
here mandatory? If so, let's make sure we cover that in API CHANGES
in NEWS.

>          target_transport = a_dir.root_transport.clone('..').clone('target')
>          target_bzrdir = a_dir.clone_on_transport(target_transport)
>          target_repo = target_bzrdir.open_repository()
> +        source_branch = bzrlib.branch.Branch.open(
> +            self.get_vfs_only_url('source'))
>          self.assertEqual(target_repo._format, source_branch.repository._format)

I'm yet to appreciate why this change is needed? Is there a short explanation?

> +class TestTransportConfig(TestCaseWithBzrDir):
> +
> +    def setUp(self):
> +        TestCaseWithBzrDir.setUp(self)
> +
> +    def test_get_config(self):
> +        my_dir = self.make_bzrdir('.')
> +        config = my_dir.get_config()
> +        if config is None:
> +            self.assertFalse(isinstance(my_dir, bzrdir.BzrDirMeta1))
> +            raise TestNotApplicable(
> +                'This BzrDir format does not support configs.')
> +        config.set_default_stack_on('http://example.com')
> +        self.assertEqual('http://example.com', config.get_default_stack_on())
> +        my_dir2 = bzrdir.BzrDir.open('.')
> +        config2 = my_dir.get_config()
> +        self.assertEqual('http://example.com', config2.get_default_stack_on())

my_dir2 is unused here. I'm guessing the line below it ought to be
using it instead of my_dir.

> +    def test_unrelated_urls(self):
> +        e = self.assertRaises(InvalidRebaseURLs, urlutils.rebase_url,
> +                              'foo', 'http://bar', 'http://baz')
> +        self.assertEqual(str(e), "URLs differ by more than path: 'http://bar'"
> +                         " and 'http://baz'")

For completeness, I'd like to see this test extended to check the case
where the net-locations match but the schemes don't.

> +    def test_rebase_success(self):
> +        self.assertEqual('../bar', urlutils.rebase_url('bar', 'http://baz/',
> +                         'http://baz/qux'))
> +        self.assertEqual('qux/bar', urlutils.rebase_url('bar',
> +                         'http://baz/qux', 'http://baz/'))
> +        self.assertEqual('../foo', urlutils.rebase_url('foo',
> +                         'http://bar/', 'http://bar/baz/'))
> +        self.assertEqual('.', urlutils.rebase_url('foo',
> +                         'http://bar/', 'http://bar/foo/'))
> +

I can't see what value the 3rd assert adds the 1st one here?

> +    old_parsed = urlparse.urlparse(old_base)
> +    new_parsed = urlparse.urlparse(new_base)
> +    if (old_parsed[:2]) != (new_parsed[:2]):
> +        raise errors.InvalidRebaseURLs(old_base, new_base)
> +    return determine_relative_path(new_parsed.path,
> +                                   osutils.pathjoin(old_parsed.path, url))

".path" was only added in Python 2.5? You might need [2] instead.

I think this overall feature is worthy of a NEWS entry as well.

A start,
Ian C.