[MERGE] move smart_add_tree to MutableTree, tested on WorkingTree..

[Robert Collins]

On Tue, 2007-07-03 at 13:07 -0400, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Robert Collins wrote:
> > This moves smart_add_tree as I discussed yesterday. Because its not
> > fully refactored - it still uses inventory, and uses os specific
> > functions not transport functions while it is on MutableTree (where i
> > think it should be), its only functional on WorkingTree today.
> 
> In that case, I would expect MutableTree.smart_add_tree to raise
> NotImplementedError, and WorkingTree to implement it.

I felt that that would just cause more churn when the next step is
taken. I can move it there if you insist; my preference is to leave the
location as is for now.

> 
> > # Bazaar merge directive format 1
> > # revision_id: robertc at robertcollins.net-20070703023836-\
> > #   etkrpxm2rmbhqgaj
> > # target_branch: http://bazaar-vcs.org/bzr/bzr.dev
> > # testament_sha1: 78604b8a043e3386e4f782001f3118e92379c465
> > # timestamp: 2007-07-03 12:39:32 +1000
> > # source_branch: sftp://rookery/~/public_html/baz2.0/add
> 
> ^^^ could you please set the public branch to one I can use?

I haven't pushed at all; generally when I'm sending bundles I don't see
any need to push?

> > +    * ``bzrlib.add.glob_expand_for_win32`` is now
> > +      ``bzrlib.win32utils.glob_expand``.  (Robert Collins)
> 
> This removes a public symbol without a deprecation period, so this would
> reset the API compatibility to 0, right?

Yes - which has already been done in the 0.18 cycle. Note that inside
NEWS they are in the LIBRARY API BREAKS section making this clear.

> > +    * ``bzrlib.add.FastPath`` is now private and moved to 
> > +      ``bzrlib.mutabletree._FastPath``. (Robert Collins, Martin Pool)
> 
> (As above)
> 
> > +    * New method ``_glob_expand_file_list_if_needed`` on the ``Command`` class
> > +      for dealing with unexpanded glob lists - e.g. on the win32 platform. This
> > +      was moved from ``bzrlib.add._prepare_file_list``. (Robert Collins)
> 
> This is probably correct, but it makes me wonder where else we should be
> doing this, and how we can ensure that it happens in the right places as
> we develop new code.  Any ideas?

theres another thread about this already :) I think as its not going to
affect merging this patch we should discuss that --> over there.

> 
> vvv Could we shorten this name?  I think "file list" is implied.  I'd
> like _maybe_glob_expand.
> 
> > +    def _glob_expand_file_list_if_needed(self, file_list):
> > +        """Glob expand file_list if the platform does not do that itself.
> > +        

mmm, perhaps '_maybe_expand_globs' ? That scans better to me.

> > === modified file bzrlib/inventory.py // last-changed:robertc at robertcollins.net
> > ... -20070703023332-jz0qdt2dwhzjuhlc
> > --- bzrlib/inventory.py
> > +++ bzrlib/inventory.py
> > @@ -1047,6 +1047,10 @@
> >                          child_dirs.append((child_relpath+'/', child_ie))
> >              stack.extend(reversed(child_dirs))
> >  
> > +    def make_entry(self, kind, name, parent_id, file_id=None):
> > +        """Simple thunk to bzrlib.inventory.make_entry."""
> > +        return make_entry(kind, name, parent_id, file_id)
> 
> If you want it to be a method of Inventory, wouldn't it make sense to
> migrate the code there?  bzrlib.inventory.make_entry can invoke
> Inventory.make_entry if it's a static method.

The main reason I did that was to avoid importing inventory just to get
at a function which really should have been a method on Inventory in the
first place. As we're not trying to do a lot with Inventory today I
didn't see any value in moving stuff around further.

> > +from bzrlib.lazy_import import lazy_import
> > +lazy_import(globals(), """
> > +import os
> > +
> > +from bzrlib import add
> > +from bzrlib import bzrdir
> > +from bzrlib.osutils import dirname
> > +from bzrlib.trace import mutter
> 
> Isn't it usual to do
> 
> from bzrlib import (
>     add,
>     bzrdir,
> )

Copy and paste of code is at fault. I'll fix.

> ?
> 
> Also, isn't it a bad idea to combine direct imports with lazy_import?

Not at all.

> > @@ -260,3 +270,246 @@
> >              parent tree - i.e. a ghost.
> >          """
> >          raise NotImplementedError(self.set_parent_trees)
> > +
> > +    @needs_tree_write_lock
> > +    def smart_add(self, file_list, recurse=True, action=None, save=True):
> > +        """Version file_list, optionally recursing into directories.
> > +
> > +        This is designed more towards DWIM for humans than API clarity.
> > +        For the specific behaviour see the help for cmd_add().
> > +
> > +        Returns the number of files added.
> 
> ^^^ needs to be updated.

Done.

> > +        :param save: Save the inventory after completing the adds. If False
> > +            this provides dry-run functionality by doing the add and not saving
> > +            the inventory.  Note that the modified inventory is left in place,
> > +            allowing further dry-run tasks to take place. To restore the
> > +            original inventory call self.read_working_inventory().
> 
> ^^^ Retaining the modified inventory doesn't mesh well with dirstate
> trees.  I think if we're introducing this as a new function, it would
> make sense change this behavior.

Huh? Meshs well AFAICT - in what way doesn't it?

> > +        # validate user file paths and convert all paths to tree 
> > +        # relative : its cheaper to make a tree relative path an abspath
> 
> ^^^ "...it's cheaper..."

Copy n paste - fixed.

> > === modified file bzrlib/workingtree.py // last-changed:robertc at robertcollins.n
> > ... et-20070702224620-rptec516zwv3opwn
> > --- bzrlib/workingtree.py
> > +++ bzrlib/workingtree.py
> > @@ -615,7 +615,7 @@
> >          # should probably put it back with the previous ID.
> >          # the read and write working inventory should not occur in this 
> >          # function - they should be part of lock_write and unlock.
> > -        inv = self.read_working_inventory()
> > +        inv = self.inventory
> >          for f, file_id, kind in zip(files, ids, kinds):
> >              assert kind is not None
> >              if file_id is None:
> > @@ -623,7 +623,7 @@
> >              else:
> >                  file_id = osutils.safe_file_id(file_id)
> >                  inv.add_path(f, kind=kind, file_id=file_id)
> > -        self._write_inventory(inv)
> > +            self._inventory_is_modified = True
> 
> It would be nice to use apply_inventory_delta instead.

That may be true; however for now this is an improvement ?

-Rob


-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add-tree-incremental.patch
Type: text/x-patch
Size: 4014 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070704/155c5e96/attachment-0001.bin 
-------------- 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/20070704/155c5e96/attachment-0001.pgp