[bug] Repository.copy() copies inventory first

[John A Meinel]

Robert Collins wrote:
> On Thu, 2006-02-16 at 14:36 -0600, John A Meinel wrote:
>> Robert Collins wrote:
>>> +1
>>> Rob
>> Do I have a +1 on sign-my-commits? Also on jam-pending.
>>
>> Then I can bring them both in.
> 
> +
> +"""Black-box tests for bzr sign-my-commits.
> +"""
> +
> 
> 
> Should be
> 
> +
> +"""Black-box tests for bzr sign-my-commits."""
> +
> 

Done

> 
> You might like to use TestCaseWithTransport which offers
> 'self.make_branch_and_tree('.'). This api will use different transport
> for the branch and repo when requested to, so can give better testing
> coverage.
> 

Done, though ultimately don't you need to set up some sort of adapter
for it?

> 
> +             pb = bzrlib.ui.ui_factory.progress_bar()
> +             copy_all(self.weave_store, destination.weave_store, pb=pb)
> +             pb.update('copying inventory', 0, 1)
>               destination.control_weaves.copy_multi(self.control_weaves,
>                                                    ['inventory'])
> -             copy_all(self.weave_store, destination.weave_store)
> -             copy_all(self.revision_store, destination.revision_store)
> +             copy_all(self.revision_store, destination.revision_store,
> pb=pb)
> 
> I think you've added an 80-char with PEP violation there. Also, doesn't
> 'destination.control_weaves.copy-multi()' want 'pb=pb' passed to it ?
> 

I wasn't planning on passing the inventory copy, because it is only
copying a single file ('inventory.weave').
Instead I figured it was better to give a specialized update which made
it clear what was being copied.

> 
> === modified file 'bzrlib/store/__init__.py'
> --- bzrlib/store/__init__.py    
> +++ bzrlib/store/__init__.py    
> @@ -35,6 +35,7 @@
>  from bzrlib.trace import mutter
>  import bzrlib.transport as transport
>  from bzrlib.transport.local import LocalTransport
> +import bzrlib.ui
> 
> Seems to be an unused import ?
> 
> 
> Other than that, +1.
> 
> Rob

I thought I got all of those. That one got left because store used to
create a pb in copy_multi, but I thought it was better to be consistent
and pass one in to all copy functions. In general, I don't like deeply
nested code to be creating ui objects.

I would actually prefer not to have ui_factory, since it means the
library is maintaining state, rather than having ui taken care of by
callbacks. Though having a factory is better than a lot of other ways.

Anyway, I made your suggested changes. Is this good enough to be merged?

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060216/3aeb68ba/attachment.pgp