BzrDir.list_branches lists same branch twice when a branch reference exists

[Neil Martinsen-Burrell]

The current implementation of bzrdir.BzrDir.list_branches can list a single branch twice if the BzrDir contains a branch reference to a branch that it contains. Consider:

>>> import bzrlib
>>> from bzrlib import bzrdir
>>> dir = bzrdir.BzrDir.create('/tmp', format=bzrdir.BzrDirMetaFormat1Colo())
>>> dir.create_repository()
CHKInventoryRepository('file:///tmp/.bzr/repository/')
>>> dir.create_branch(name='trunk')
BzrBranch7(file:///tmp/,branch=trunk)
>>> dir.list_branches()
[BzrBranch7(file:///tmp/,branch=trunk)]
>>> from bzrlib import branch
>>> branch.BranchReferenceFormat().initialize(dir, target_branch=dir.open_branch(name='trunk'))
BzrBranch7(file:///tmp/,branch=trunk)
>>> dir.list_branches()
[BzrBranch7(file:///tmp/,branch=trunk), BzrBranch7(file:///tmp/,branch=trunk)]

The offending code in bzrlib/bzrdir.py is

    def list_branches(self):
        """See ControlDir.list_branches."""
        ret = []
        # Default branch
        try:
            ret.append(self.open_branch())
        except (errors.NotBranchError, errors.NoRepositoryPresent):
            pass

        # colocated branches
        ret.extend([self.open_branch(name.decode("utf-8")) for name in
                    self._read_branch_list()])

which adds the branch once as the default branch and once from _read_branch_list. For a colocated workspace, the default branch will always be in the branch list, so list_branches will always return a list with a repeat.

If we have no guarantees on the order in which list_branches returns, we can "return list(set(ret))" to de-dupe.

[Neil Martinsen-Burrell (nmb)]

Oops, we can't list(set(...)) to remove duplicates because apparently branches don't compare equal based on their base attribute. Should they?

[Martin Pool (mbp)]

On 22 November 2011 18:49, Neil Martinsen-Burrell <email address hidden> wrote:
> Oops, we can't list(set(...)) to remove duplicates because apparently
> branches don't compare equal based on their base attribute.  Should
> they?

No. It's tempting, but adding __cmp__ to complex non-value-like
objects tends to get in to trouble when people make different
assumptions about what equality means: is a locked Branch equal to an
unlocked Branch pointing to the same thing?

Better just to use has_same_location, or get the base locations and
uniquify on that.

--
Martin

[Jelmer Vernooij (jelmer)]

Am 22/11/11 09:06, schrieb Martin Pool:
> On 22 November 2011 18:49, Neil Martinsen-Burrell<email address hidden> wrote:
>> Oops, we can't list(set(...)) to remove duplicates because apparently
>> branches don't compare equal based on their base attribute. Should
>> they?
> No. It's tempting, but adding __cmp__ to complex non-value-like
> objects tends to get in to trouble when people make different
> assumptions about what equality means: is a locked Branch equal to an
> unlocked Branch pointing to the same thing?
>
> Better just to use has_same_location, or get the base locations and
> uniquify on that.
I wonder if BzrDir.list_branches() needs to return a dictionary mapping
the names to branches, so it can preserve the original names.

Cheers,

Jelmer

[Jelmer Vernooij (jelmer)]

Am 22/11/11 08:16, schrieb Neil Martinsen-Burrell:
> Public bug reported:
>
> The current implementation of bzrdir.BzrDir.list_branches can list a
> single branch twice if the BzrDir contains a branch reference to a
> branch that it contains. Consider:
>
>>>> import bzrlib
>>>> from bzrlib import bzrdir
>>>> dir = bzrdir.BzrDir.create('/tmp', format=bzrdir.BzrDirMetaFormat1Colo())
>>>> dir.create_repository()
> CHKInventoryRepository('file:///tmp/.bzr/repository/')
>>>> dir.create_branch(name='trunk')
> BzrBranch7(file:///tmp/,branch=trunk)
>>>> dir.list_branches()
> [BzrBranch7(file:///tmp/,branch=trunk)]
>>>> from bzrlib import branch
>>>> branch.BranchReferenceFormat().initialize(dir, target_branch=dir.open_branch(name='trunk'))
> BzrBranch7(file:///tmp/,branch=trunk)
>>>> dir.list_branches()
> [BzrBranch7(file:///tmp/,branch=trunk), BzrBranch7(file:///tmp/,branch=trunk)]
>
> The offending code in bzrlib/bzrdir.py is
>
> def list_branches(self):
> """See ControlDir.list_branches."""
> ret = []
> # Default branch
> try:
> ret.append(self.open_branch())
> except (errors.NotBranchError, errors.NoRepositoryPresent):
> pass
>
> # colocated branches
> ret.extend([self.open_branch(name.decode("utf-8")) for name in
> self._read_branch_list()])
>
> which adds the branch once as the default branch and once from
> _read_branch_list. For a colocated workspace, the default branch will
> always be in the branch list, so list_branches will always return a list
> with a repeat.
The problem isn't really in that code, but in the fact that branches can
be branch references, and those are resolved when the branch is opened.
The default branch doesn't have to be a branch reference, which is why
it's being opened. Likewise, any of the other colocated branches can be
references of any of the other colocated branches.

Cheers,

Jelmer

[Neil Martinsen-Burrell (nmb)]

For colocated branches, I think the question is whether or not the working tree and its reference to the branch should count as an entry in list_branches. If I did "bzr init --development-colo file:test,branch=trunk" then I would be very surprised if "bzr branches" listed trunk twice.

The code that tests list_branches in blacbox/test_switch.py, which actually has a working tree, uses set(list_branches()) presumably to eliminate order dependency, but it also obscures any possible repeats. In bzrlib/tests/per_branch/test_branch.py:test_create_colocated there are no working trees and similarly in bzrlib/tests/per_controldir/test_controldir.py: test_list_branches and bzrlib/tests/per_controldir_colo/test_supported:test_unicode. I think that my original code excerpt could be added as a failing test.

[Neil Martinsen-Burrell (nmb)]

If I add an assert for len(list_branches()) to bb.test_switch.TestSwitch.test_switch_new_colocated:

$ bzr diff blackbox/test_switch.py=== modified file 'bzrlib/tests/blackbox/test_switch.py'
--- bzrlib/tests/blackbox/test_switch.py 2011-11-16 17:52:07 +0000
+++ bzrlib/tests/blackbox/test_switch.py 2011-11-22 21:11:12 +0000
@@ -212,6 +212,7 @@
         self.assertEquals(
             set([b.name for b in bzrdir.list_branches()]),
             set(["foo", "anotherbranch"]))
+ self.assertEquals(len(bzrdir.list_branches()), 2)
         self.assertEquals(bzrdir.open_branch().name, "anotherbranch")
         self.assertEquals(bzrdir.open_branch().last_revision(), revid1)

then that test fails too.

[Neil Martinsen-Burrell (nmb)]

See https://code.launchpad.net/~nmb/bzr/893470-dedupe-list-branches/+merge/83075 for the above failing test and a possible fix.