[MERGE] Nested trees: Store tree-reference locations in the branch.

Ian Clatworthy ian.clatworthy at internode.on.net
Mon Apr 20 08:20:50 BST 2009


Aaron Bentley wrote:
> Hi all,
> 
> This patch implements a new branch type that can store the branch
> locations of tree references.

bb:tweak

> I haven't yet implemented the "reference-location" command, but I intend
> to do so.  It will be similar to the alias command, providing a way to
> list, view and set reference locations.

I think calling it simple "reference" would be fine.

BTW, we're not fully consistent here. We have:

* case where we have multiple commands, e.g. tag & tags
* cases where we use just one command, e.g. alias
* cases where the single command lists the active thing and --all
  is needed to display all things, e.g. view.
* cases where only the list aspect is supported, e.g. plugins.

In other cases, I'm proposing the management is a new command and
the listing is the responsibility of the info command, e.g.
http://bazaar-vcs.org/DraftSpecs/RemoteBranchTracking.

I'll have the same decision to make when I spec/implement branch
requirements (if they aren't just special rules), e.g.

  bzr require bzrlib 1.15

Maybe we need to standardise on one policy and make that part of
the http://bazaar-vcs.org/DraftSpecs/BetterNewUserUI work?

Anyhow, on to the review tweaks ...

> +    def initialize(self, a_bzrdir):
> +        """Create a branch of this format in a_bzrdir."""
> +        utf8_files = [('last-revision', '0 null:\n'),
> +                      ('branch.conf', ''),
> +                      ('tags', ''),
> +                      ('references', '')
> +                      ]
> +        return self._initialize_helper(a_bzrdir, utf8_files)

I suspect you need to also define and register a simple converter
from format7 to format8 that initialises an empty references file.

> +    def _get_all_reference_info(self):
> +        """Return all the reference info stored in a branch.
> +
> +        :return: A dict of {file_id: (tree_path, branch_location)}
> +        """
> +        rio_file = self._transport.get('references')
> +        try:
> +            stanzas = rio.read_stanzas(rio_file)
> +            info_dict = dict((s['file_id'], (s['tree_path'],
> +                             s['branch_location'])) for s in stanzas)
> +        finally:
> +            rio_file.close()
> +        return info_dict

Is it worth caching this dict in a branch attribute? Getting it from
disk every time seems unnecessary.

> +    def update_references_skips_known_references(self):
> +        branch = self.make_branch_with_reference('branch', 'reference')
> +        new_branch = branch.bzrdir.sprout('branch/new-branch').open_branch()
> +        new_branch.set_reference_info('file-id', '../foo', '../foo')
> +        new_branch.update_references(branch)
> +        self.assertEqual('../reference',
> +                         branch.get_reference_info('file-id')[1])

This doesn't seem to be used anywhere as a helper method. I suspect you
meant to put test_ at the front of the method name.

Nice job on this, particularly on the tests.

Ian C.



More information about the bazaar mailing list