[MERGE] init remote branches

Wouter van Heyst larstiq at larstiq.dyndns.org
Tue Jul 4 02:28:47 BST 2006


On Sun, Jul 02, 2006 at 01:08:51PM -0500, John Arbash Meinel wrote:
> -----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

Fixed.

> 
> >      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.

Good point, I mimicked init-repo code without much thought here.

> 
> 
> >          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.

Is doing a transport.has() first never better? Changing to EAFP style.

> 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.

There was a comment in cmd_init:

            # The path has to exist to initialize a
            # branch inside of it.
            # 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

introduced by mbp at sourcefrog.net-20051031120610-da41deafd12fdbb8 , merged in
1498. --create-prefix seems to have been introduced in 1495 for push.

I think doing an optional --create-prefix for init, init-repo and branch also
is fine.

<snip>

> > @@ -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.

I originally copied the 'from bzrlib.transport import get_transport' and
transport = get_transport(location) for init from here, until Robert mentioned
the function local import was undesirable. I think Aaron has good
reasons for importing the function, but I also think you bring up good
reasons not to. I would probably go for importing the module on account
of hooking in the general case. get_transport is accomodated well enough
to hook in extra transports already though, so here not having to use
bzrlib.transport.get_transport() might win. Any objections to switching
back to that style?

<snip>

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

Done. bzr init on a local path will suggest to use 'checkout' to create
a working tree, but for a remote location that is a bit bogus.

The tests suffered from not being entirely TDD. The assertFalse should
actually be in the test_init_existing_branch method, but since
bzrdir.has_workingtree() only works on local paths, I return
AlreadyBranchError in both cases and the tests pass, even though
logically the assert is in the wrong place.

Thanks for the review! Especially for catching that logic inversion.

Wouter van Heyst




More information about the bazaar mailing list