[MERGE] Some small fixes for bzr-hg

[Jelmer Vernooij]

Hi Robert,

Sorry for taking so long to get back to you. Better late than
never... :-)

On Sun, 2006-07-16 at 21:56 +1000, Robert Collins wrote:
> 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.
It's generated by setup.py. There also isn't much harm in ignoring it, I
hate having it showing up in 'bzr status' constantly.

> > +      long_description="""
> > +      This plugin adds support for branching off Mercurial branches
> > in 
> > +      Bazaar.
> The long description can be improved I think - I mean, bzrk in a hg
> branch is even sexier than pulling from hg ;).
Fixed, at least a bit..

> > +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.
> > +    """
> 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,
That was actually a copy-n-paste error :-) Fixed.

> > +        # TODO: Call out to mercurial for consistency checking?
> Please add the TODO to ./TODO in the branch as well, creating that file
> if needed.
Fixed.

> > +    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.
Fixed as well. I now realise I actually meant that branches without repositories weren't supported. 

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

> > @@ -291,6 +337,22 @@
> >          self.control_files = lockfiles
> >          self.repository = HgRepository(hgrepo, hgdir, lockfiles)
> >          self.base = hgdir.root_transport.base
> > +        self._format = HgBranchFormat()
> > +
> > +    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 ?)
Yes, added. Should be pulled from .hgrc.

> > +    def get_physical_lock_status(self):
> > +        return False
> Given this returns False, why does the lock object need peek() defined ?
That was actually a quick-n-dirty hack to get info to work.

> >      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.)
The history was in reverse order. I'll add a test for this later.

> > @@ -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.
I've added a stub class that will later read .hgrc.

Looking for a +1...

Cheers,

Jelmer
-- 
Jelmer Vernooij <jelmer at samba.org> - http://samba.org/~jelmer/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzr-hg-small-fixes.bundle.diff
Type: text/x-patch
Size: 32450 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070120/1673c18f/attachment-0001.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 307 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070120/1673c18f/attachment-0001.pgp