[MERGE] init remote branches

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Wouter van Heyst wrote:
> Hi all,
> 
> unlike init-repo init isn't able to work with remote urls. I think it
> should be, especially for centralized workflows. Help and comments from
> Rob on irc resulted in the attached bundle.
> 
> https://launchpad.net/products/bzr/+bug/48904
> mergeable from http://bzr.richtlijn.be/remote-init
> 
> Wouter van Heyst
> 

Thanks for doing this. I agree that 'init' should work on a remote location.

...

> === modified file bzrlib/builtins.py
> --- bzrlib/builtins.py
> +++ bzrlib/builtins.py
> @@ -39,6 +39,7 @@
>  from bzrlib.revision import common_ancestor
>  from bzrlib.revisionspec import RevisionSpec
>  from bzrlib.trace import mutter, note, log_error, warning, is_quiet, info
> +import  bzrlib.transport
         ^^- you have two spaces here


...

>      def run(self, location=None, format=None):
>          if format is None:
>              format = get_format_type('default')
> +        transport = bzrlib.transport.get_transport(location)
>          if location is None:
>              location = u'.'

^-- You are opening the transport before you are changing it to '.'
I don't know if the tests are passing or not, but get_transport *might*
default to '.'
Anyway, I wouldn't count on this, this get_transport() should be done
after the 'if location is None' check.


>          else:
> @@ -1015,8 +1015,9 @@
>              # Just using os.mkdir, since I don't
>              # believe that we want to create a bunch of
>              # locations if the user supplies an extended path
> -            if not os.path.exists(location):
> -                os.mkdir(location)
> +            if not transport.has('.'):
> +                transport.mkdir('')
> +                    

I also prefer to see:
try:
  transport.mkdir('.')
except FileExists:
  pass

The problem is that 'has' is not defined on all transports, especially
when asking about directories.
For example, IIRC ftp doesn't support has() on a directory. has() in
general is not well supported, and means that we are doing 2 round trips
rather than just 1 and handling if there is an error.

Also, you aren't supporting:

bzr init --create-prefix sftp://foo

Which means that we only support 'init' 1-layer deep, which should at
least be documented.


>          try:
>              existing_bzrdir = bzrdir.BzrDir.open(location)
>          except NotBranchError:
> @@ -1024,10 +1025,10 @@
>              bzrdir.BzrDir.create_branch_convenience(location, format=format)
>          else:
>              if existing_bzrdir.has_branch():
> -                if existing_bzrdir.has_workingtree():
> -                    raise errors.AlreadyBranchError(location)
> -                else:
> -                    raise errors.BranchExistsWithoutWorkingTree(location)
> +                if isinstance(transport, LocalTransport):
> +                    if not existing_bzrdir.has_workingtree():
> +                        raise errors.BranchExistsWithoutWorkingTree(location)
> +                raise errors.AlreadyBranchError(location)
>              else:
>                  existing_bzrdir.create_branch()
>                  existing_bzrdir.create_workingtree()
> @@ -1059,10 +1060,9 @@
>                               ' a working tree')]
>      aliases = ["init-repo"]
>      def run(self, location, format=None, trees=False):
> -        from bzrlib.transport import get_transport
>          if format is None:
>              format = get_format_type('default')
> -        transport = get_transport(location)
> +        transport = bzrlib.transport.get_transport(location)
>          if not transport.has('.'):
>              transport.mkdir('')
>          newdir = format.initialize_on_transport(transport)
> 

I realize you probably just pulled it from here, but really I would
prefer this to be fixed up as well.
I also think mkdir('.') is clearer than mkdir(''), especially confusing
is the fact that the 'has()' is using '.', but the mkdir() isn't.


> === modified file bzrlib/tests/blackbox/test_init.py
> --- bzrlib/tests/blackbox/test_init.py
> +++ bzrlib/tests/blackbox/test_init.py
> @@ -22,6 +22,7 @@
>  
>  from bzrlib.bzrdir import BzrDirMetaFormat1
>  from bzrlib.tests.blackbox import ExternalBase
> +from bzrlib.tests.test_sftp_transport import TestCaseWithSFTPServer
>  from bzrlib.workingtree import WorkingTree
>  
>  
> @@ -100,3 +101,28 @@
>          # suggests using checkout
>          self.assertContainsRe(err, 'ontains a branch.*but no working tree.*checkout')
>  
> +
> +class TestSFTPInit(TestCaseWithSFTPServer):
> +
> +    def test_init(self):
> +        # init on a remote url should succeed.
> +        out, err = self.run_bzr('init', self.get_url())
> +        self.assertEqual('', out)
> +        self.assertEqual('', err)
> +    
> +    def test_init_existing_branch(self):
> +        # when there is already a branch present, make mention
> +        self.make_branch('.')
> +
> +        # rely on SFTPServer get_url() pointing at '.'
> +        out, err = self.run_bzr('init', self.get_url(), retcode=3)
> +        self.assertContainsRe(err, 'Already a branch')
> +
> +    def test_init_existing_branch_with_workingtree(self):
> +        # don't distinguish between the branch having a working tree or not
> +        # when the branch itself is remote.
> +        self.make_branch_and_tree('.')
> +
> +        # rely on SFTPServer get_url() pointing at '.'
> +        out, err = self.run_bzr('init', self.get_url(), retcode=3)
> +        self.assertFalse(re.search(r'checkout', err))


Can you put a comment about what the assertFalse here is doing? It isn't
clear to me.

So a couple of things I would like to see, but overall it looks good.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEqAuzJdeBCYSNAAMRAq2XAKC8I2vnbm93rG2wO9CySHiETKpnwgCfUG4d
X/VcZKCDTg52EVRZeSQ61Fw=
=gIag
-----END PGP SIGNATURE-----