Merge lp:~jelmer/bzr/colocated-names into lp:bzr

[Jelmer Vernooij]

Slightly tweak the naming of colocated branches.

Use "" for the name of the default branch - e.g. the branch in .bzr/branch
Use None for the name of the selected branch

The default and selected branch aren't necessarily the same; the selected
branch can be different if selected in the URL ("http://example.com/path/to/dir,branch=foo")

This is a slight API change, but one which only should affect bzr 2.5.

[Vincent Ladeuil (vila)]

Hmm, 2.5 API is frozen :)

Now, if this is internal only I think it's fair enough to go in but the fallouts of having a distinctin between a selected branch and a default branch are unclear. Can you give some more examples of why it matters and where it's needed ?

A few nits:

51 - result = real_bzrdir.open_branch(name=name,
52 - ignore_fallbacks=ignore_fallbacks,
53 + result = real_bzrdir.open_branch(ignore_fallbacks=ignore_fallbacks,

Err, wait, you got 'name' a few lines above why force the callee to get it
again ?

63 + if name is None:
64 + raise ValueError('name must be supplied')
65 + self.bzrdir = a_bzrdir

Sounds like a good time to document 'name' in the ivars few lines above ;)

But going further, why not change the 'name' default value to "" ?

review: Needs Information

[Jelmer Vernooij (jelmer)]

> Now, if this is internal only I think it's fair enough to go in but the fallouts of having a distinctin between a >
> selected branch and a default branch are unclear. Can you give some more examples of why it matters and where it's
> needed ?
I think this is a necessary bug fix, because without it it is impossible to open the current branch through the control directory if the user has already selected a different branch. I.e. we want:

BzrDir.open_from_url("file:foo,branch=bar").open_branch(name=None) to return branch ".bzr/branches/bar" and
BzrDir.open_from_url("file:foo,branch=bar").open_branch(name="") to return ".bzr/branch".

> A few nits:

> 51 - result = real_bzrdir.open_branch(name=name,
> 52 - ignore_fallbacks=ignore_fallbacks,
> 53 + result = real_bzrdir.open_branch(ignore_fallbacks=ignore_fallbacks,

> Err, wait, you got 'name' a few lines above why force the callee to get it
> again ?

real_bzrdir is for the branch we're referencing. name is the name of *our* branch (the referencing branch), not the name of the branch that's referenced. The previous code was wrong.

63 + if name is None:
64 + raise ValueError('name must be supplied')
65 + self.bzrdir = a_bzrdir

> Sounds like a good time to document 'name' in the ivars few lines above ;)
name isn't an instance variable, so I'm not sure I follow what you mean?

> But going further, why not change the 'name' default value to "" ?
Because we don't want to default to "", we want to use "" only IFF the user hasn't specified a branch in the URL.

[Vincent Ladeuil (vila)]

> > Now, if this is internal only I think it's fair enough to go in but the
> fallouts of having a distinctin between a >
> > selected branch and a default branch are unclear. Can you give some more
> examples of why it matters and where it's
> > needed ?
> I think this is a necessary bug fix, because without it it is impossible to
> open the current branch through the control directory if the user has already
> selected a different branch. I.e. we want:
>
> BzrDir.open_from_url("file:foo,branch=bar").open_branch(name=None) to return
> branch ".bzr/branches/bar" and
> BzrDir.open_from_url("file:foo,branch=bar").open_branch(name="") to return
> ".bzr/branch".

Excellent, I smelled something but couldn't put my finger on it.

>
> > A few nits:
>
> > 51 - result = real_bzrdir.open_branch(name=name,
> > 52 - ignore_fallbacks=ignore_fallbacks,
> > 53 + result = real_bzrdir.open_branch(ignore_fallbacks=ignore_fallbacks,
>
> > Err, wait, you got 'name' a few lines above why force the callee to get it
> > again ?
>
> real_bzrdir is for the branch we're referencing. name is the name of *our*
> branch (the referencing branch), not the name of the branch that's referenced.
> The previous code was wrong.

Ok.

>
> 63 + if name is None:
> 64 + raise ValueError('name must be supplied')
> 65 + self.bzrdir = a_bzrdir
>
> > Sounds like a good time to document 'name' in the ivars few lines above ;)
> name isn't an instance variable, so I'm not sure I follow what you mean?

self.name = name a few lines below ?

>
> > But going further, why not change the 'name' default value to "" ?
> Because we don't want to default to "", we want to use "" only IFF the user
> hasn't specified a branch in the URL.

Ok, don't forget to target lp:bzr/2.5 and probably a news entry in the bug fixes section.

review: Approve

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2012-01-04 17:12:42 +0000
+++ bzrlib/branch.py	2012-01-07 01:57:25 +0000
@@ -2038,6 +2038,8 @@
         :param name: Name of colocated branch to create, if any
         :return: a branch in this format
         """
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         mutter('creating branch %r in %s', self, a_bzrdir.user_url)
         branch_transport = a_bzrdir.get_branch_transport(self, name=name)
         control_files = lockable_files.LockableFiles(branch_transport,
@@ -2060,6 +2062,8 @@
     def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False,
             found_repository=None, possible_transports=None):
         """See BranchFormat.open()."""
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         if not _found:
             format = BranchFormatMetadir.find_format(a_bzrdir, name=name)
             if format.__class__ != self.__class__:
@@ -2305,12 +2309,13 @@
         mutter('creating branch reference in %s', a_bzrdir.user_url)
         if a_bzrdir._format.fixed_components:
             raise errors.IncompatibleFormat(self, a_bzrdir._format)
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         branch_transport = a_bzrdir.get_branch_transport(self, name=name)
         branch_transport.put_bytes('location',
             target_branch.user_url)
         branch_transport.put_bytes('format', self.as_string())
-        branch = self.open(
-            a_bzrdir, name, _found=True,
+        branch = self.open(a_bzrdir, name, _found=True,
             possible_transports=[target_branch.bzrdir.root_transport])
         self._run_post_branch_init_hooks(a_bzrdir, name, branch)
         return branch
@@ -2342,6 +2347,8 @@
             a_bzrdir.
         :param possible_transports: An optional reusable transports list.
         """
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         if not _found:
             format = BranchFormatMetadir.find_format(a_bzrdir, name=name)
             if format.__class__ != self.__class__:
@@ -2351,8 +2358,7 @@
             location = self.get_reference(a_bzrdir, name)
         real_bzrdir = controldir.ControlDir.open(
             location, possible_transports=possible_transports)
-        result = real_bzrdir.open_branch(name=name, 
-            ignore_fallbacks=ignore_fallbacks,
+        result = real_bzrdir.open_branch(ignore_fallbacks=ignore_fallbacks,
             possible_transports=possible_transports)
         # this changes the behaviour of result.clone to create a new reference
         # rather than a copy of the content of the branch.
@@ -2445,10 +2451,11 @@
         """Create new branch object at a particular location."""
         if a_bzrdir is None:
             raise ValueError('a_bzrdir must be supplied')
-        else:
-            self.bzrdir = a_bzrdir
+        if name is None:
+            raise ValueError('name must be supplied')
+        self.bzrdir = a_bzrdir
         self._user_transport = self.bzrdir.transport.clone('..')
-        if name is not None:
+        if name != "":
             self._user_transport.set_segment_parameter(
                 "branch", urlutils.escape(name))
         self._base = self._user_transport.base

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2012-01-05 09:50:04 +0000
+++ bzrlib/builtins.py	2012-01-07 01:57:25 +0000
@@ -1444,13 +1444,13 @@
         else:
             dir = controldir.ControlDir.open_containing(location)[0]
             try:
-                active_branch = dir.open_branch(name=None)
+                active_branch = dir.open_branch(name="")
             except errors.NotBranchError:
                 active_branch = None
             branches = dir.get_branches()
             names = {}
             for name, branch in branches.iteritems():
-                if name is None:
+                if name == "":
                     continue
                 active = (active_branch is not None and
                           active_branch.base == branch.base)

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2011-12-22 19:54:56 +0000
+++ bzrlib/bzrdir.py	2012-01-07 01:57:25 +0000
@@ -829,7 +829,9 @@
 
     def destroy_branch(self, name=None):
         """See BzrDir.create_branch."""
-        if name is not None:
+        if name is None:
+            name = self._get_selected_branch()
+        if name != "":
             raise errors.NoColocatedBranchSupport(self)
         self.transport.delete_tree('branch')
 
@@ -887,7 +889,9 @@
 
     def get_branch_transport(self, branch_format, name=None):
         """See BzrDir.get_branch_transport()."""
-        if name is not None:
+        if name is None:
+            name = self._get_selected_branch()
+        if name != "":
             raise errors.NoColocatedBranchSupport(self)
         # XXX: this shouldn't implicitly create the directory if it's just
         # promising to get a transport -- mbp 20090727
@@ -1021,7 +1025,7 @@
         :param name: Optional branch name to use
         :return: Relative path to branch
         """
-        if name is None:
+        if name == "":
             return 'branch'
         return urlutils.join('branches', name.encode("utf-8"))
 
@@ -1056,7 +1060,7 @@
         if name is None:
             name = self._get_selected_branch()
         path = self._get_branch_path(name)
-        if name is not None:
+        if name != "":
             self.control_files.lock_write()
             try:
                 branches = self._read_branch_list()
@@ -1073,17 +1077,19 @@
         """See ControlDir.get_branches."""
         ret = {}
         try:
-            ret[None] = self.open_branch()
+            ret[""] = self.open_branch(name="")
         except (errors.NotBranchError, errors.NoRepositoryPresent):
             pass
 
         for name in self._read_branch_list():
-            ret[name] = self.open_branch(name.decode('utf-8'))
+            ret[name] = self.open_branch(name=name.decode('utf-8'))
 
         return ret
 
     def get_branch_transport(self, branch_format, name=None):
         """See BzrDir.get_branch_transport()."""
+        if name is None:
+            name = self._get_selected_branch()
         path = self._get_branch_path(name)
         # XXX: this shouldn't implicitly create the directory if it's just
         # promising to get a transport -- mbp 20090727
@@ -1093,7 +1099,7 @@
             branch_format.get_format_string()
         except NotImplementedError:
             raise errors.IncompatibleFormat(branch_format, self._format)
-        if name is not None:
+        if name != "":
             try:
                 self.transport.mkdir('branches', mode=self._get_mkdir_mode())
             except errors.FileExists:

=== modified file 'bzrlib/controldir.py'
--- bzrlib/controldir.py	2012-01-03 13:47:01 +0000
+++ bzrlib/controldir.py	2012-01-07 01:57:25 +0000
@@ -116,7 +116,7 @@
         :return: Dictionary mapping branch names to instances.
         """
         try:
-           return { None: self.open_branch() }
+           return { "": self.open_branch() }
         except (errors.NotBranchError, errors.NoRepositoryPresent):
            return {}
 
@@ -294,9 +294,9 @@
         :return: Name of the branch selected by the user, or None.
         """
         branch = self.root_transport.get_segment_parameters().get("branch")
-        if branch is not None:
-            branch = urlutils.unescape(branch)
-        return branch
+        if branch is None:
+            branch = ""
+        return urlutils.unescape(branch)
 
     def has_workingtree(self):
         """Tell if this controldir contains a working tree.

=== modified file 'bzrlib/plugins/weave_fmt/branch.py'
--- bzrlib/plugins/weave_fmt/branch.py	2011-12-19 13:23:58 +0000
+++ bzrlib/plugins/weave_fmt/branch.py	2012-01-07 01:57:25 +0000
@@ -138,7 +138,9 @@
     def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False,
             found_repository=None, possible_transports=None):
         """See BranchFormat.open()."""
-        if name is not None:
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
+        if name != "":
             raise errors.NoColocatedBranchSupport(self)
         if not _found:
             # we are being called directly and must probe.

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2012-01-04 16:02:14 +0000
+++ bzrlib/remote.py	2012-01-07 01:57:25 +0000
@@ -625,6 +625,8 @@
 
     def create_branch(self, name=None, repository=None,
                       append_revisions_only=None):
+        if name is None:
+            name = self._get_selected_branch()
         # as per meta1 formats - just delegate to the format object which may
         # be parameterised.
         real_branch = self._format.get_branch_format().initialize(self,
@@ -724,6 +726,8 @@
 
     def open_branch(self, name=None, unsupported=False,
                     ignore_fallbacks=False, possible_transports=None):
+        if name is None:
+            name = self._get_selected_branch()
         if unsupported:
             raise NotImplementedError('unsupported flag support not implemented yet.')
         if self._next_open_branch_result is not None:
@@ -3102,6 +3106,8 @@
 
     def initialize(self, a_bzrdir, name=None, repository=None,
                    append_revisions_only=None):
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         # 1) get the network name to use.
         if self._custom_format:
             network_name = self._custom_format.network_name()
@@ -3122,7 +3128,7 @@
         # Creating on a remote bzr dir.
         # 2) try direct creation via RPC
         path = a_bzrdir._path_for_remote_call(a_bzrdir._client)
-        if name is not None:
+        if name != "":
             # XXX JRV20100304: Support creating colocated branches
             raise errors.NoColocatedBranchSupport(self)
         verb = 'BzrDir.create_branch'

=== modified file 'bzrlib/smart/bzrdir.py'
--- bzrlib/smart/bzrdir.py	2011-12-19 13:23:58 +0000
+++ bzrlib/smart/bzrdir.py	2012-01-07 01:57:25 +0000
@@ -268,7 +268,7 @@
             self.transport_from_client_path(path))
         format = branch.network_format_registry.get(network_name)
         bzrdir.branch_format = format
-        result = format.initialize(bzrdir)
+        result = format.initialize(bzrdir, name="")
         rich_root, tree_ref, external_lookup = self._format_to_capabilities(
             result.repository._format)
         branch_format = result._format.network_name()

=== modified file 'bzrlib/tests/per_bzrdir/test_bzrdir.py'
--- bzrlib/tests/per_bzrdir/test_bzrdir.py	2011-12-12 12:09:50 +0000
+++ bzrlib/tests/per_bzrdir/test_bzrdir.py	2012-01-07 01:57:25 +0000
@@ -693,5 +693,5 @@
             raise TestNotApplicable('Format does not support colocation')
         reference = branch.BranchReferenceFormat().initialize(
             repo.bzrdir, target_branch=target_branch)
-        self.assertEqual(set([None, 'foo']),
+        self.assertEqual(set(["", 'foo']),
                          set(repo.bzrdir.get_branches().keys()))

=== modified file 'bzrlib/tests/per_controldir/test_controldir.py'
--- bzrlib/tests/per_controldir/test_controldir.py	2011-12-27 12:18:36 +0000
+++ bzrlib/tests/per_controldir/test_controldir.py	2012-01-07 01:57:25 +0000
@@ -1195,7 +1195,7 @@
     def test_get_branches(self):
         repo = self.make_repository('branch-1')
         target_branch = repo.bzrdir.create_branch()
-        self.assertEqual([None], repo.bzrdir.get_branches().keys())
+        self.assertEqual([""], repo.bzrdir.get_branches().keys())
 
     def test_create_repository(self):
         # a bzrdir can construct a repository for itself.
@@ -1342,7 +1342,7 @@
             raise TestSkipped("Can't initialize %r on transport %r"
                               % (self.bzrdir_format, t))
         dir = bzrdir.BzrDir.open(t.base)
-        self.assertIs(None, dir._get_selected_branch())
+        self.assertEqual(u"", dir._get_selected_branch())
 
     def test_root_transport(self):
         dir = self.make_bzrdir('.')

=== modified file 'bzrlib/tests/per_controldir_colo/test_unsupported.py'
--- bzrlib/tests/per_controldir_colo/test_unsupported.py	2011-12-11 04:07:08 +0000
+++ bzrlib/tests/per_controldir_colo/test_unsupported.py	2012-01-07 01:57:25 +0000
@@ -73,4 +73,4 @@
         made_control = self.make_bzrdir_with_repo()
         made_control.create_branch()
         self.assertEqual(made_control.get_branches().keys(),
-                         [None])
+                         [""])

=== modified file 'bzrlib/tests/test_foreign.py'
--- bzrlib/tests/test_foreign.py	2011-12-19 19:15:58 +0000
+++ bzrlib/tests/test_foreign.py	2012-01-07 01:57:25 +0000
@@ -240,6 +240,8 @@
 
     def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False,
             found_repository=None):
+        if name is None:
+            name = a_bzrdir._get_selected_branch()
         if not _found:
             raise NotImplementedError
         try:
@@ -251,7 +253,8 @@
             return DummyForeignVcsBranch(_format=self,
                               _control_files=control_files,
                               a_bzrdir=a_bzrdir,
-                              _repository=found_repository)
+                              _repository=found_repository,
+                              name=name)
         except errors.NoSuchFile:
             raise errors.NotBranchError(path=transport.base)
 
@@ -317,7 +320,9 @@
 
     def open_branch(self, name=None, unsupported=False, ignore_fallbacks=True,
             possible_transports=None):
-        if name is not None:
+        if name is None:
+            name = self._get_selected_branch()
+        if name != "":
             raise errors.NoColocatedBranchSupport(self)
         return self._format.get_branch_format().open(self, _found=True)