[RFC][Merge] repository support

[Robert Collins]

On Wed, 2006-02-15 at 11:12 -0600, John A Meinel wrote:
> Robert Collins wrote:
> >
> >  
> > -    def clone(self, url, revision_id=None, basis=None):
> > +    def clone(self, url, revision_id=None, basis=None, force_new_repo=False):
> >          """Clone this bzrdir and its contents to url verbatim.
> >  
> >          If urls last component does not exist, it will be created.
> >  
> >          if revision_id is not None, then the clone operation may tune
> >              itself to download less data.
> > +        :param force_new_repo: Do not use a shared repository for the target 
> > +                               even if one is available.
> >          """
> 
> I want to make sure I understand the objects here...
> 'local_repo' is the repository local to the branch we are copying from.
> 'basis_repo' is something given by --basis
> 'result_repo' is the target repository.

Yes.

> >          self._make_tail(url)
> > +        basis_repo, basis_branch, basis_tree = self._get_basis_components(basis)
> >          result = self._format.initialize(url)
> > -        basis_repo, basis_branch, basis_tree = self._get_basis_components(basis)
> > -        try:
> > -            self.open_repository().clone(result, revision_id=revision_id, basis=basis_repo)
> > +        try:
> > +            local_repo = self.find_repository()
> >          except errors.NoRepositoryPresent:
> > -            pass
> > +            local_repo = None
> > +        if local_repo:
> > +            # may need to copy content in
> > +            if force_new_repo:
> > +                local_repo.clone(result, revision_id=revision_id, basis=basis_repo)
> > +            else:
> > +                try:
> > +                    result_repo = result.find_repository()
> > +                    # fetch content this dir needs.
> > +                    if basis_repo:
> > +                        # XXX FIXME RBC 20060214 need tests for this when the basis
> > +                        # is incomplete
> > +                        result_repo.fetch(basis_repo, revision_id=revision_id)
> > +                    result_repo.fetch(local_repo, revision_id=revision_id)
> > +                except errors.NoRepositoryPresent:
> > +                    # needed to make one anyway.
> > +                    local_repo.clone(result, revision_id=revision_id, basis=basis_repo)
> > +        # 1 if there is a branch present
> > +        #   make sure its content is available in the target repository
> > +        #   clone it.
> 
> Wouldn't this be cleaner as (paraphrased):
> 
> if local_repo:
>   if force_new_repo:
>     result_repo = None
>   else:
>     try:
>       result_repo = result.find_repository()
>     except
>       result_repo = None
> 
>   if not result_repo:
>     result_repo.fetch(...)
>   else:
>     local_repo.clone(result, revision_id=revision_id, basis=basis_repo)
> 
> It just seems less nested, with less redundancy.

Uhm, your paraphrased code seems buggy :O.

You would introduce a new repository even if the source bzrdir did not
have one and force_new_repo was off.

> It also seems like code would be cleaner if "find_repository" returned
> None if it didn't find any, instead of throwing an exception. Then you
> could just do:
> 
> if force_new_repo:
>   result_repo = None
> else:
>   result_repo = result.find_repository()
> 
> if result_repo is None:
>   ...

I much prefer guaranteed apis where 'repo = find_repository()' always
worked if it returns rather than throwing. Its difficult for developers
if they have to remember that some things signal errors by returning
None, some by returning -X, some by throwing exceptions. 

> You have this same pattern later where you have:
> 
> if force:
>   do X
> else:
>   try:
>     find
>     do Y
>   except
>     do X

One complicating issue is that for clone() and sprout() we delegate the
creation of the target repository to the repository itself, which is
important because we don't know the right format to use and the
repository does. sprout and clone also have different rules about when
to make a new repository - because of their different contracts.

> >          try:
> >              self.open_branch().clone(result, revision_id=revision_id)
> >          except errors.NotBranchError:
> > @@ -140,26 +161,73 @@
> >          raise NotImplementedError(self.create_branch)
> >  
> >      @staticmethod
> > -    def create_branch_and_repo(base):
> > +    def create_branch_and_repo(base, force_new_repo=False):
> >          """Create a new BzrDir, Branch and Repository at the url 'base'.
> >  
> >          This will use the current default BzrDirFormat, and use whatever 
> >          repository format that that uses via bzrdir.create_branch and
> > -        create_repository.
> > +        create_repository. If a shared repository is available that is used
> > +        preferentially.
> >  
> >          The created Branch object is returned.
> > +
> > +        :param base: The URL to create the branch at.
> > +        :param force_new_repo: If True a new repository is always created.
> >          """
> >          bzrdir = BzrDir.create(base)
> > -        bzrdir.create_repository()
> > +        if force_new_repo:
> > +            bzrdir.create_repository()
> > +        try:
> > +            repo = bzrdir.find_repository()
> > +        except errors.NoRepositoryPresent:
> > +            bzrdir.create_repository()
> >          return bzrdir.create_branch()
> >          
> >      @staticmethod
> > -    def create_repository(base):
> > +    def create_branch_convenience(base, force_new_repo=False, force_new_tree=None):
> > +        """Create a new BzrDir, Branch and Repository at the url 'base'.
> > +
> > +        This is a convenience function - it will use an existing repository
> > +        if possible, can be told explicitly whether to create a working tree or
> > +        nor.
> 
> not
> 
> > +
> > +        This will use the current default BzrDirFormat, and use whatever 
> > +        repository format that that uses via bzrdir.create_branch and
> > +        create_repository. If a shared repository is available that is used
> > +        preferentially. Whatever repository is used, its tree creation policy
> > +        is followed.
> > +
> > +        The created Branch object is returned.
> > +        If a working tree cannot be made due to base not being a file:// url,
> > +        no error is raised.
> > +
> > +        :param base: The URL to create the branch at.
> > +        :param force_new_repo: If True a new repository is always created.
> > +        :param force_new_tree: If True or False force creation of a tree or 
> > +                               prevent such creation respectively.
> > +        """
> > +        bzrdir = BzrDir.create(base)
> > +        if force_new_repo:
> > +            bzrdir.create_repository()
> 
> Isn't this incorrect? You have create, and then find, and then create
> again. Shouldn't it be:

BzrDir.create() creates the bzrdir itself. I think that lead to a
misread of the code :(. Its clearer now with the extracted
_find_or_create_repository.

...

> can you name this "open_containing_from_transport"? Otherwise it sounds
> like you are returning a transport not a branch (or bzr dir).

Ok.

> ...
> 
> ...
> 
> > +        if force_new_repo:
> > +            result_repo = None
> > +        else:
> > +            try:
> > +                result_repo = result.find_repository()
> > +            except errors.NoRepositoryPresent:
> > +                result_repo = None
> 
> Again we have this pattern. Mabye it would be better to factor it into a
> function:
> 
> def find_or_create_repository(force=False):

I've factored out the two cases where it does follow that pattern into
such a method.
...
> I didn't go through all of the tests very thoroughly. But generally
> things look good. My only comment is the repeating paradigm of
> find_or_create. (+1 from me)


Thanks

Are you happy with my replies/actions above? If so I'll merge it.

Cheers,
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: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060216/5a97f8f1/attachment.pgp