[MERGE] Some small fixes for bzr-hg

Robert Collins robertc at robertcollins.net
Sun Jul 16 12:56:14 BST 2006


On Sat, 2006-07-15 at 17:57 +0200, Jelmer Vernooij wrote:


> === added file .bzrignore //
> file-id:bzrignore-20060706005544-yzowsrbf322pjinx-
> ... 1
> --- /dev/null
> +++ .bzrignore
> @@ -0,0 +1,2 @@
> +*.pyc
> +build

Whats build here ? Ignoring *.pyc is clearly fine.

> === added file setup.py //
> file-id:setup.py-20060706010156-paz07n490vbyvp96-1 /
> ... / executable:yes
> --- /dev/null
> +++ setup.py
> @@ -0,0 +1,19 @@
> +#!/usr/bin/env python2.4
> +
> +from distutils.core import setup
> +
> +setup(name='bzr-hg',
> +      description='Support for Mercurial branches in Bazaar-NG',
> +      keywords='plugin bzr hg mercurial bazaar',
> +      version='0.1',
> +      url='http://bazaar-vcs.org/BzrForeignBranches/Mercurial',
> +      license='GPL',
> +      author='Robert Collins',
> +      author_email='robertc at robertcollins.net',
> +      long_description="""
> +      This plugin adds support for branching off Mercurial branches
> in 
> +      Bazaar.
> +      """,
> +      package_dir={'bzrlib.plugins.hg':'.'},
> +      packages=['bzrlib.plugins.hg']
> +      )


The long description can be improved I think - I mean, bzrk in a hg
branch is even sexier than pulling from hg ;).

> === modified file __init__.py //
> last-changed:jelmer at samba.org-20060619104352-e
> ... 75b7cba415530b1
> --- __init__.py
> +++ __init__.py
> @@ -39,6 +39,7 @@
>  
>  import bzrlib.branch
>  import bzrlib.bzrdir
> +from bzrlib.config import TreeConfig
>  from bzrlib.decorators import *
>  import bzrlib.errors as errors
>  from bzrlib.inventory import Inventory
> @@ -72,6 +73,9 @@
>      def lock_read(self):
>          self._lock = self._hgrepo.lock()
>  
> +    def peek(self):
> +        raise NotImplementedError(self.peek)
> +
>      def unlock(self):
>          self._lock.release()

Fine.

> @@ -86,6 +90,19 @@
>          self._lock_count = 0
>  
>  
> +class HgRepositoryFormat(bzrlib.repository.RepositoryFormat):
> +    """Mercurial Repository Format.
> +
> +    This is currently not aware of different working tree formats,
> +    but simply relies on the installed copy of mercurial to 
> +    support the working tree format.
> +    """
> +
> +    def get_format_description(self):
> +        """See RepositoryFormat.get_format_description()."""
> +        return "Mercurial Repository Format"
> +
> +


I'm guessing this is for info support right? Anyway, for the docstring -
repositories are never aware of different working tree formats. I think
what you mean is that the Repository is supported by using the installed
copy of hg,

>  class HgRepository(bzrlib.repository.Repository):
>      """An adapter to mercurial repositories for bzr."""
>  
> @@ -93,6 +110,11 @@
>          self._hgrepo = hgrepo
>          self.bzrdir = hgdir
>          self.control_files = lockfiles
> +        self._format = HgRepositoryFormat()
> +
> +    def _check(self, revision_ids):
> +        # TODO: Call out to mercurial for consistency checking?
> +        return bzrlib.branch.BranchCheckResult(self)

Please add the TODO to ./TODO in the branch as well, creating that file
if needed.

>      def get_inventory(self, revision_id):
>          """Synthesize a bzr inventory from an hg manifest...
> @@ -281,6 +303,30 @@
>      def has_revision(self, revision_id):
>          return hgrevid_from_bzr(revision_id) in
> self._hgrepo.changelog.nodemap
>  
> +    def is_shared(self):
> +        """Whether this repository is being shared between multiple
> branches. 
> +        
> +        Always false for Mercurial as it doesn't support checkouts
> yet.
> +        """
> +        return False
> +
> +

I'm not entirely sure that *reason* is true. Its definately true that hg
does not support checkouts yet, but thats not why is_shared should
always be false - we could use a bzr checkout of an hg branch easily,
and in fact a bzr branch with an hg repository. The key reason is that
we have no way to read the configuration data yet, as we haven't got ini
reading logic etc for the hg repository.


> +class HgBranchFormat(bzrlib.branch.BranchFormat):
> +    """Mercurial Branch Format.
> +
> +    This is currently not aware of different working tree formats,
> +    but simply relies on the installed copy of mercurial to 
> +    support the working tree format.
> +    """
> +
> +    def get_format_description(self):
> +        """See BranchFormat.get_format_description()."""
> +        return "Mercurial Branch Format"
> +
> +    def get_format_string(self):
> +        """See BranchFormat.get_branch_format()."""
> +        return "Mercurial Branch Format"
>+

This MUST NOT provide a branch format string, as its not able to be used
with our meta-dir format engine. It hooks in at the ControlDir layer
only. (Nb, this applies to the svn plugin too). A format description is
fine. I'd say that the format description would be better as 
    return "Mercurial"
or perhaps
    return "Mercurial Branch"


 
>  class HgBranch(bzrlib.branch.Branch):
>      """An adapter to mercurial repositories for bzr Branch
> objects."""
> @@ -291,6 +337,22 @@
>          self.control_files = lockfiles
>          self.repository = HgRepository(hgrepo, hgdir, lockfiles)
>          self.base = hgdir.root_transport.base
> +        self._format = HgBranchFormat()
> +
> +    def _check(self):
> +        # TODO: Call out to mercurial for consistency checking?
> +        return bzrlib.branch.BranchCheckResult(self)
> +
> +    def get_parent(self):
> +        """Return the URL of the parent branch."""
> +        return None
> +

Does this need a TODO? (does hg have a default pull facility ?)

> +    def get_physical_lock_status(self):
> +        return False

Given this returns False, why does the lock object need peek() defined ?

> +
> +    def get_push_location(self):
> +        """Return default push location of this branch."""
> +        return None

Ditto the question about hg facilities.

>      def lock_write(self):
>          self.control_files.lock_write()
> @@ -303,6 +365,7 @@
>          while next_rev != mercurial.node.nullid:
>              revs.append(bzrrevid_from_hg(next_rev))
>              next_rev = self._hgrepo.changelog.parents(next_rev)[0]
> +        revs.reverse()
>          return revs

What is this for? Is there a test that fails without the reverse() call
in there ? (There should be, either in bzrlib core, or in the plugin
tests.)

>      @needs_read_lock
> @@ -313,6 +376,9 @@
>      def lock_read(self):
>          self.control_files.lock_read()
>  
> +    def tree_config(self):
> +        return TreeConfig(self)
> +  

TreeConfig is completely wrong for this as the hg branch does not have
the branch.conf ini file - we should provide  MercurialBranchConfig
rather than a TreeConfig.

>      def unlock(self):
>          self.control_files.unlock()
>  
> @@ -326,6 +392,19 @@
>          return to_bzrdir.open_branch()
>  
>  
> +class HgWorkingTreeFormat(bzrlib.workingtree.WorkingTreeFormat):
> +    """Working Tree format for Mercurial Working Trees.
> +
> +    This is currently not aware of different working tree formats,
> +    but simply relies on the installed copy of mercurial to 
> +    support the working tree format.
> +    """
> +
> +    def get_format_description(self):
> +        """See WorkingTreeFormat.get_format_description()."""
> +        return "Mercurial Working Tree Format"
> +
> +

A similar comment to that I made earlier - the docstring here can be
more clear. Also, saying 'currently' implies a plan to change :), and
AFAIK there is no plan to change the approach, as hg does not version
its branch data layout in the same manner we do... and even if they did,
as we are thunking to their API, we should not ever need to have
different thunks.

>  class HgWorkingTree(bzrlib.workingtree.WorkingTree):
>      """An adapter to mercurial repositories for bzr WorkingTree
> obejcts."""
>  
> @@ -334,6 +413,7 @@
>          self.bzrdir = hgdir
>          self._control_files = lockfiles
>          self._branch = HgBranch(hgrepo, hgdir, lockfiles)
> +        self._format = HgWorkingTreeFormat()
>  
>      @needs_write_lock
>      def add(self, files, ids=None):
> @@ -436,6 +516,9 @@
>          """We should write a converter."""
>          return HgToSomethingConverter()
>  
> +    def get_format_description(self):
> +        return "Mercurial Branch"
> +
>      def initialize_on_transport(self, transport):
>          """Initialize a new .not dir in the base directory of a
> Transport."""
>          return self.open(transport, _create=True)
> 


This is looking nice. Not quite ready to merge.

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060716/be4adf88/attachment.pgp 


More information about the bazaar mailing list