Merge lp:~mbp/bzr/deprecation into lp:bzr

[Martin Pool]

This cleans up some old and pretty crufty code around AddAction and smart_add. The existing api was sufficiently complicated, and as far I can tell not used by plugins, so I think it's better just to delete it. This is a bit cleaner on its own, but also detangles things a bit to allower a larger refactoring of smart_add to be smaller and to use an inventory delta rather than mutating the in-memory inventory. (bug 79336 asked for this a long time ago; it was worked-around in bzr-svn but it would be better to give them a simpler interface).

There are essentially two ways callers might want to customize smart_add: a way to generate file ids (for example to match those in an existing tree) and to report on what's being done (typically to a file). For no good reason they were tied together into a somewhat strange callable object callback. Instead, there are now just plain callables for each.

Before merging this I'll check whether it impacts other things that may be adding files or implementing smart_add, including bzr-svn, the package importer, and bzr builder. Generally things that are mechanically adding files shouldn't need to call smart_add.

It might be nice to have a reporter object that's told about more actions from inside add and can do more than just say the files that were added.

[Martin Pool (mbp)]

At the moment bzr-svn implements its own smart_add, which is a shame because I don't think there's anything very svn-specific in it. It ignores the 'action' parameter. This won't fix that but it might get closer to using a common implementation.

[Robert Collins (lifeless)]

It would also be good to check speed; add isn't a hugely common
operation, but benchmarkers do tend to test the time of 'bzr add' in
kernel/openoffice trees.

-Rob

[Jelmer Vernooij (jelmer)]

Hi Martin,

I think this is a nice improvement over the previous smart_add() overall.

For bzr-svn this is a useful improvement in that the action argument is no longer necessary. It still wouldn't be able to use the stock smart_add() because that calls _write_inventory().

The file_id_suggestion() callback on Tree seems a bit odd to me, mostly because of its signature. I wonder if it really makes sense as a public method on Tree (which already has a fair amount of methods) rather than as a helper method on cmd_add.

[Martin Pool (mbp)]

On 17 October 2010 21:52, Robert Collins <email address hidden> wrote:
> It would also be good to check speed; add isn't a hugely common
> operation, but benchmarkers do tend to test the time of 'bzr add' in
> kernel/openoffice trees.

Yes, good idea, I will. I may need to dust off usertest...

--
Martin

[Martin Pool (mbp)]

On 18 October 2010 10:54, Jelmer Vernooij <email address hidden> wrote:
> Hi Martin,
>
> I think this is a nice improvement over the previous smart_add() overall.

Great.

> For bzr-svn this is a useful improvement in that the action argument is no longer necessary. It still wouldn't be able to use the stock smart_add() because that calls _write_inventory().

Yes, I know. I am heading towards using an inventory delta instead,
which should make things easier for you, and this helps detangle it.

>
> The file_id_suggestion() callback on Tree seems a bit odd to me, mostly because of its signature. I wonder if it really makes sense as a public method on Tree (which already has a fair amount of methods) rather than as a helper method on cmd_add.

I was wondering about that too. I'll move it.

--
Martin

Unmerged revisions

5363. By Martin Pool on 2010-10-17

Move add-from-base-tree into file_id_suggestion method on Tree.

Delete remaining AddAction and all of bzrlib.add.

5362. By Martin Pool on 2010-10-17

Delete obsolete AddAction code

5361. By Martin Pool on 2010-10-17

Remove specific print-to-file arguments from smart_add in favor of reporter callback

5360. By Martin Pool on 2010-10-17

fix broken imports

5359. By Martin Pool on 2010-10-17

Document deprecation of smart_add(action=...)

5358. By Martin Pool on 2010-10-17

merge trunk

5357. By Martin Pool on 2010-07-18

trim imports

5356. By Martin Pool on 2010-07-18

news

5355. By Martin Pool on 2010-07-18

Delete obsolete and bad AddAction tests

5354. By Martin Pool on 2010-07-18

Don't pass an add action for simple command line add

=== removed file 'bzrlib/add.py'
--- bzrlib/add.py	2010-06-23 08:19:28 +0000
+++ bzrlib/add.py	1970-01-01 00:00:00 +0000
@@ -1,95 +0,0 @@
-# Copyright (C) 2005-2010 Canonical Ltd
-#
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 2 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
-
-"""Helper functions for adding files to working trees."""
-
-import sys
-
-import bzrlib.osutils
-
-
-class AddAction(object):
-    """A class which defines what action to take when adding a file."""
-
-    def __init__(self, to_file=None, should_print=None):
-        """Initialize an action which prints added files to an output stream.
-
-        :param to_file: The stream to write into. This is expected to take
-            Unicode paths. If not supplied, it will default to ``sys.stdout``.
-        :param should_print: If False, printing will be suppressed.
-        """
-        self._to_file = to_file
-        if to_file is None:
-            self._to_file = sys.stdout
-        self.should_print = False
-        if should_print is not None:
-            self.should_print = should_print
-
-    def __call__(self, inv, parent_ie, path, kind, _quote=bzrlib.osutils.quotefn):
-        """Add path to inventory.
-
-        The default action does nothing.
-
-        :param inv: The inventory we are working with.
-        :param path: The FastPath being added
-        :param kind: The kind of the object being added.
-        """
-        if self.should_print:
-            self._to_file.write('adding %s\n' % _quote(path.raw_path))
-        return None
-
-
-class AddFromBaseAction(AddAction):
-    """This class will try to extract file ids from another tree."""
-
-    def __init__(self, base_tree, base_path, to_file=None, should_print=None):
-        super(AddFromBaseAction, self).__init__(to_file=to_file,
-                                                should_print=should_print)
-        self.base_tree = base_tree
-        self.base_path = base_path
-
-    def __call__(self, inv, parent_ie, path, kind):
-        # Place the parent call
-        # Now check to see if we can extract an id for this file
-        file_id, base_path = self._get_base_file_id(path, parent_ie)
-        if file_id is not None:
-            if self.should_print:
-                self._to_file.write('adding %s w/ file id from %s\n'
-                                    % (path.raw_path, base_path))
-        else:
-            # we aren't doing anything special, so let the default
-            # reporter happen
-            file_id = super(AddFromBaseAction, self).__call__(
-                        inv, parent_ie, path, kind)
-        return file_id
-
-    def _get_base_file_id(self, path, parent_ie):
-        """Look for a file id in the base branch.
-
-        First, if the base tree has the parent directory,
-        we look for a file with the same name in that directory.
-        Else, we look for an entry in the base tree with the same path.
-        """
-
-        if (parent_ie.file_id in self.base_tree):
-            base_parent_ie = self.base_tree.inventory[parent_ie.file_id]
-            base_child_ie = base_parent_ie.children.get(path.base_path)
-            if base_child_ie is not None:
-                return (base_child_ie.file_id,
-                        self.base_tree.id2path(base_child_ie.file_id))
-        full_base_path = bzrlib.osutils.pathjoin(self.base_path, path.raw_path)
-        # This may return None, but it is our last attempt
-        return self.base_tree.path2id(full_base_path), full_base_path

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-10-15 11:30:54 +0000
+++ bzrlib/builtins.py	2010-10-17 10:21:09 +0000
@@ -627,9 +627,7 @@
 
     def run(self, file_list, no_recurse=False, dry_run=False, verbose=False,
             file_ids_from=None):
-        import bzrlib.add
-
-        base_tree = None
+        file_id_callback = report_callback = None
         if file_ids_from is not None:
             try:
                 base_tree, base_path = WorkingTree.open_containing(
@@ -638,18 +636,23 @@
                 base_branch, base_path = Branch.open_containing(
                                             file_ids_from)
                 base_tree = base_branch.basis_tree()
-
-            action = bzrlib.add.AddFromBaseAction(base_tree, base_path,
-                          to_file=self.outf, should_print=(not is_quiet()))
-        else:
-            action = bzrlib.add.AddAction(to_file=self.outf,
-                should_print=(not is_quiet()))
-
-        if base_tree:
             self.add_cleanup(base_tree.lock_read().unlock)
+            if is_quiet():
+                to_file = None
+            else:
+                to_file = self.outf
+            file_id_callback = base_tree.file_id_suggestion(
+                base_path, to_file)
+        elif not is_quiet():
+            def report_callback(ie, path):
+                self.outf.write("adding " + path + "\n")
         tree, file_list = tree_files_for_add(file_list)
-        added, ignored = tree.smart_add(file_list, not
-            no_recurse, action=action, save=not dry_run)
+        added, ignored = tree.smart_add(
+            file_list,
+            recurse=(not no_recurse),
+            file_id_callback=file_id_callback,
+            save=not dry_run,
+            report_callback=report_callback)
         self.cleanup_now()
         if len(ignored) > 0:
             if verbose:

=== modified file 'bzrlib/mutabletree.py'
--- bzrlib/mutabletree.py	2010-08-17 06:45:33 +0000
+++ bzrlib/mutabletree.py	2010-10-17 10:21:09 +0000
@@ -30,9 +30,9 @@
     bzrdir,
     errors,
     hooks,
+    inventory,
     osutils,
     revisiontree,
-    inventory,
     symbol_versioning,
     trace,
     tree,
@@ -370,7 +370,13 @@
         raise NotImplementedError(self.set_parent_trees)
 
     @needs_tree_write_lock
-    def smart_add(self, file_list, recurse=True, action=None, save=True):
+    def smart_add(self,
+            file_list,
+            recurse=True,
+            action=None,
+            save=True,
+            file_id_callback=None,
+            report_callback=None):
         """Version file_list, optionally recursing into directories.
 
         This is designed more towards DWIM for humans than API clarity.
@@ -386,15 +392,22 @@
         :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.
+        :param file_id_callback: Optional,
+            file_id_callback(parent_ie, path, kind) => file_id.
+        :param report_callback: Optional, called after each file is added 
+            with (ie, path); result ignored.
         :return: A tuple - files_added, ignored_files. files_added is the count
             of added files, and ignored_files is a dict mapping files that were
             ignored to the rule that caused them to be ignored.
         """
-        # not in an inner loop; and we want to remove direct use of this,
-        # so here as a reminder for now. RBC 20070703
-        from bzrlib.inventory import InventoryEntry
-        if action is None:
-            action = add.AddAction()
+        if action is not None:
+            raise ValueError(
+                "passing action to smart_add is no longer supported in bzr 2.3")
+        elif file_id_callback is None:
+            file_id_callback = lambda *a: None
+
+        if report_callback is None:
+            report_callback = lambda *a: None
 
         if not file_list:
             # no paths supplied: add the entire tree.
@@ -437,7 +450,7 @@
                 # schedule the dir for scanning
                 user_dirs.add(rf)
             else:
-                if not InventoryEntry.versionable_kind(kind):
+                if not inventory.InventoryEntry.versionable_kind(kind):
                     raise errors.BadFileKindError(filename=abspath, kind=kind)
             # ensure the named path is added, so that ignore rules in the later
             # directory walk dont skip it.
@@ -446,7 +459,8 @@
             versioned = inv.has_filename(rf.raw_path)
             if versioned:
                 continue
-            added.extend(_add_one_and_parent(self, inv, None, rf, kind, action))
+            added.extend(_add_one_and_parent(self, inv, None, rf, kind,
+                file_id_callback, report_callback))
 
         if not recurse:
             # no need to walk any directories at all.
@@ -478,7 +492,7 @@
             # find the kind of the path being added.
             kind = osutils.file_kind(abspath)
 
-            if not InventoryEntry.versionable_kind(kind):
+            if not inventory.InventoryEntry.versionable_kind(kind):
                 trace.warning("skipping %s (can't add file of kind '%s')",
                               abspath, kind)
                 continue
@@ -529,7 +543,8 @@
                 # 20070306
                 trace.mutter("%r is a nested bzr tree", abspath)
             else:
-                _add_one(self, inv, parent_ie, directory, kind, action)
+                _add_one(self, inv, parent_ie, directory, kind,
+                    file_id_callback, report_callback)
                 added.append(directory.raw_path)
 
             if kind == 'directory' and not sub_tree:
@@ -681,7 +696,8 @@
         return hash(self.raw_path)
 
 
-def _add_one_and_parent(tree, inv, parent_ie, path, kind, action):
+def _add_one_and_parent(tree, inv, parent_ie, path, kind, file_id_callback,
+    report_callback):
     """Add a new entry to the inventory and automatically add unversioned parents.
 
     :param inv: Inventory which will receive the new entry.
@@ -689,7 +705,8 @@
         None, the parent is looked up by name and used if present, otherwise it
         is recursively added.
     :param kind: Kind of new entry (file, directory, etc)
-    :param action: callback(inv, parent_ie, path, kind); return ignored.
+    :param file_id_callback: callback(inv, parent_ie, path, kind); return a
+        file_id or None to generate a new file id
     :return: A list of paths which have been added.
     """
     # Nothing to do if path is already versioned.
@@ -707,20 +724,24 @@
         # there are a limited number of dirs we can be nested under, it should
         # generally find it very fast and not recurse after that.
         added = _add_one_and_parent(tree, inv, None,
-            _FastPath(osutils.dirname(path.raw_path)), 'directory', action)
+            _FastPath(osutils.dirname(path.raw_path)), 'directory',
+            file_id_callback,
+            report_callback)
         parent_id = inv.path2id(osutils.dirname(path.raw_path))
         parent_ie = inv[parent_id]
-    _add_one(tree, inv, parent_ie, path, kind, action)
+    _add_one(tree, inv, parent_ie, path, kind, file_id_callback,
+        report_callback)
     return added + [path.raw_path]
 
 
-def _add_one(tree, inv, parent_ie, path, kind, file_id_callback):
+def _add_one(tree, inv, parent_ie, path, kind, file_id_callback,
+    report_callback):
     """Add a new entry to the inventory.
 
     :param inv: Inventory which will receive the new entry.
     :param parent_ie: Parent inventory entry.
     :param kind: Kind of new entry (file, directory, etc)
-    :param file_id_callback: callback(inv, parent_ie, path, kind); return a
+    :param file_id_callback: callback(parent_ie, path, kind); return a
         file_id or None to generate a new file id
     :returns: None
     """
@@ -735,7 +756,8 @@
         del inv[parent_ie.file_id]
         inv.add(new_parent_ie)
         parent_ie = new_parent_ie
-    file_id = file_id_callback(inv, parent_ie, path, kind)
+    file_id = file_id_callback(parent_ie, path, kind)
     entry = inv.make_entry(kind, path.base_path, parent_ie.file_id,
         file_id=file_id)
     inv.add(entry)
+    report_callback(entry, path.raw_path)

=== modified file 'bzrlib/tests/per_workingtree/test_smart_add.py'
--- bzrlib/tests/per_workingtree/test_smart_add.py	2010-02-23 07:43:11 +0000
+++ bzrlib/tests/per_workingtree/test_smart_add.py	2010-10-17 10:21:09 +0000
@@ -20,7 +20,6 @@
 import sys
 
 from bzrlib import (
-    add,
     errors,
     ignores,
     osutils,
@@ -28,12 +27,16 @@
     workingtree,
     )
 from bzrlib.tests import (
-    features,
-    test_smart_add,
     per_workingtree,
     )
 
 
+def _make_custom_file_id(parent_ie, path, kind):
+    return osutils.safe_file_id(kind + '-'
+        + path.raw_path.replace('/', '%'),
+        warn=False)
+
+
 class TestSmartAddTree(per_workingtree.TestCaseWithWorkingTree):
 
     def test_single_file(self):
@@ -203,20 +206,9 @@
             [path for path, ie in tree.iter_entries_by_dir()])
 
     def test_custom_ids(self):
-        sio = StringIO()
-        action = test_smart_add.AddCustomIDAction(to_file=sio,
-                                                  should_print=True)
+        wt = self.make_branch_and_tree('.')
         self.build_tree(['file1', 'dir1/', 'dir1/file2'])
-
-        wt = self.make_branch_and_tree('.')
-        wt.smart_add(['.'], action=action)
-        # The order of adds is not strictly fixed:
-        sio.seek(0)
-        lines = sorted(sio.readlines())
-        self.assertEqualDiff(['added dir1 with id directory-dir1\n',
-                              'added dir1/file2 with id file-dir1%file2\n',
-                              'added file1 with id file-file1\n',
-                             ], lines)
+        wt.smart_add(['.'], file_id_callback=_make_custom_file_id)
         wt.lock_read()
         self.addCleanup(wt.unlock)
         self.assertEqual([('', wt.path2id('')),

=== modified file 'bzrlib/tests/test_smart_add.py'
--- bzrlib/tests/test_smart_add.py	2010-02-10 17:52:08 +0000
+++ bzrlib/tests/test_smart_add.py	2010-10-17 10:21:09 +0000
@@ -18,26 +18,10 @@
 
 from bzrlib import (
     add,
-    inventory,
-    osutils,
     tests,
     )
 
 
-class AddCustomIDAction(add.AddAction):
-
-    def __call__(self, inv, parent_ie, path, kind):
-        # The first part just logs if appropriate
-        # Now generate a custom id
-        file_id = osutils.safe_file_id(kind + '-'
-                                       + path.raw_path.replace('/', '%'),
-                                       warn=False)
-        if self.should_print:
-            self._to_file.write('added %s with id %s\n'
-                                % (path.raw_path, file_id))
-        return file_id
-
-
 class TestAddFrom(tests.TestCaseWithTransport):
     """Tests for AddFromBaseAction"""
 
@@ -62,7 +46,8 @@
                 action = add.AddFromBaseAction(base_tree, base_path,
                                                to_file=to_file,
                                                should_print=should_print)
-                new_tree.smart_add(file_list, action=action)
+                new_tree.smart_add(
+                    file_list, file_id_callback=action.generate_file_id)
             finally:
                 new_tree.unlock()
         finally:
@@ -141,22 +126,3 @@
         self.base_tree.lock_read()
         self.addCleanup(self.base_tree.unlock)
         self.failIf(a_id in self.base_tree)
-
-
-class TestAddActions(tests.TestCase):
-
-    def test_quiet(self):
-        self.run_action("")
-
-    def test__print(self):
-        self.run_action("adding path\n")
-
-    def run_action(self, output):
-        from bzrlib.mutabletree import _FastPath
-        inv = inventory.Inventory()
-        stdout = StringIO()
-        action = add.AddAction(to_file=stdout, should_print=bool(output))
-
-        self.apply_redirected(None, stdout, None, action, inv, None,
-            _FastPath('path'), 'file')
-        self.assertEqual(stdout.getvalue(), output)

=== modified file 'bzrlib/tree.py'
--- bzrlib/tree.py	2010-08-31 07:12:18 +0000
+++ bzrlib/tree.py	2010-10-17 10:21:09 +0000
@@ -37,7 +37,10 @@
 from bzrlib.inter import InterObject
 from bzrlib.osutils import fingerprint_file
 from bzrlib.symbol_versioning import deprecated_function, deprecated_in
-from bzrlib.trace import note
+from bzrlib.trace import (
+    mutter, 
+    note,
+    )
 
 
 class Tree(object):
@@ -588,6 +591,49 @@
         """
         pass
 
+    def file_id_suggestion(self, under_dir, print_to_file):
+        """Return a callback to use with smart_add file_id_callback.
+
+        The returned callable tries to guess a file id for a newly added
+        file in a mutable tree, based on finding one at the same path in 
+        this tree.
+
+        First, if this tree has the same parent directory id,
+        we look for a file with the same name in that directory.
+        Otherwise, we look for an entry in this tree with the same path.
+
+        :param under_path: Look for file ids under the given directory.
+        :param print_to_file: If non-None, write a message about matched
+            files to this stream.
+        """
+        def find_one(parent_ie, path, kind):
+            # mutter("look up suggestion for %r in parent %r"
+            # % (path.raw_path, parent_ie))
+            file_id = None
+            if parent_ie.file_id in self:
+                # mutter("found parent in %r" % self)
+                base_parent_ie = self.inventory[parent_ie.file_id]
+                # mutter("  looking for child %r" % path.base_path)
+                base_child_ie = base_parent_ie.children.get(path.base_path)
+                if base_child_ie is not None:
+                    # mutter("  found child under it")
+                    file_id = base_child_ie.file_id
+                    origin_path = self.id2path(base_child_ie.file_id)
+            if file_id is None:
+                # Not found yet: try looking for the whole path.
+                origin_path = osutils.pathjoin(under_dir, path.raw_path)
+                file_id = self.path2id(origin_path)
+            if print_to_file is None:
+                pass
+            elif file_id is not None:
+                print_to_file.write(
+                    'adding %s w/ file id from %s\n'
+                    % (path.raw_path, origin_path))
+            else:
+                print_to_file.write('adding %s\n' % path.raw_path)
+            return file_id
+        return find_one
+
     def revision_tree(self, revision_id):
         """Obtain a revision tree for the revision revision_id.
 

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-10-15 15:05:09 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-10-17 10:21:09 +0000
@@ -72,6 +72,12 @@
   deprecated in favour of ``known_hooks.key_to_parent_and_attribute`` in
   the same module.  (Andrew Bennetts)
 
+* The ``action`` parameter to ``MutableTree.smart_add`` is no longer
+  supported and the AddAction classes have been removed.  Instead, 
+  file ids can be generated by passing a ``file_id_callback``, and output
+  can be shown through the ``report_callback``.
+  (Martin Pool)
+
 Internals
 *********