[MERGE] Stacking policy

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:
> 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 think we're talking about different things.  I was talking about
allowing the user to override the default stacking at branch creation time.

I think you're talking about providing a way to convert a stacked branch
into an unstacked one.  I agree that would be useful, but there are
already workarounds we can use.

> 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 don't really see tension there.  I think we shouldn't stack by default
unless the target location has a default stacking policy.  In fact, I've
voted Resubmit on one of your patches because it does stack by default,
and provides no way for the user to prevent it:t.
http://article.gmane.org/gmane.comp.version-control.bazaar-ng.general/43000

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

We had a short discussion on IRC and he seemed okay with it:

(05:56:42 PM) lifeless: abentley: your bzrdir change, is just a conf file?
(05:57:30 PM) abentley: lifeless: Yes.
(05:58:01 PM) lifeless: abentley: then I think its ok to add it without
bumping the format, but subsequent changes are likely to need a bump
(05:58:13 PM) abentley: lifeless: agreed.
(05:58:44 PM) lifeless: abentley: is the new file cloned?
(05:58:51 PM) abentley: lifeless: no.

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

No.  The old version of bzr won't support stacking.

> If not, will it fail gracefully?

Yes.  They'll get an error that the branch or repo is in an unsupported
format, because old branch/repo formats don't support stacking.

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

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

Very small, I think.  But yes, I can update 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?

- From what I can tell, this is a test of whether the on-disk format of
the source equals the target.  It was failing because source was
RemoteRepositoryFormat, and target was the on-disk format.

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

Good catch.

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

Okay.

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

I think the 3rd assert is redundant with the first.

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

Oh, okay.

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

Sure.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD4DBQFIW7FJ0F+nu1YWqI0RAr4QAJdtifi+RKlr2/IT4FXQOuzRHebbAJ9R2Cxv
4vLHA47EaNds8Gsn446uog==
=04il
-----END PGP SIGNATURE-----