Bazaar

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

  1. deprecation
  2. Merge into bzr.dev
Proposed by Martin Pool
Status: Work in progress
Proposed branch: lp:~mbp/bzr/deprecation
Merge into: lp:bzr
Diff against target: 503 lines (+119/-179)
7 files modified
bzrlib/add.py (+0/-95)
bzrlib/builtins.py (+16/-13)
bzrlib/mutabletree.py (+40/-18)
bzrlib/tests/per_workingtree/test_smart_add.py (+8/-16)
bzrlib/tests/test_smart_add.py (+2/-36)
bzrlib/tree.py (+47/-1)
doc/en/release-notes/bzr-2.3.txt (+6/-0)
To merge this branch: bzr merge lp:~mbp/bzr/deprecation
Reviewer Review Type Date Requested Status
bzr-core Pending
Review via email: mp+38647@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

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.

Reply
Revision history for this message
Robert Collins (lifeless) 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.

-Rob

Reply
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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.

Reply
Revision history for this message
Martin Pool (mbp) wrote :

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

Reply
Revision history for this message
Martin Pool (mbp) wrote :

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

Reply

Unmerged revisions

5363. By Martin Pool

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

Delete remaining AddAction and all of bzrlib.add.

5362. By Martin Pool

Delete obsolete AddAction code

5361. By Martin Pool

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

5360. By Martin Pool

fix broken imports

5359. By Martin Pool

Document deprecation of smart_add(action=...)

5358. By Martin Pool

merge trunk

5357. By Martin Pool

trim imports

5356. By Martin Pool

news

5355. By Martin Pool

Delete obsolete and bad AddAction tests

5354. By Martin Pool

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== removed file 'bzrlib/add.py'
2--- bzrlib/add.py 2010-06-23 08:19:28 +0000
3+++ bzrlib/add.py 1970-01-01 00:00:00 +0000
4@@ -1,95 +0,0 @@
5-# Copyright (C) 2005-2010 Canonical Ltd
6-#
7-# This program is free software; you can redistribute it and/or modify
8-# it under the terms of the GNU General Public License as published by
9-# the Free Software Foundation; either version 2 of the License, or
10-# (at your option) any later version.
11-#
12-# This program is distributed in the hope that it will be useful,
13-# but WITHOUT ANY WARRANTY; without even the implied warranty of
14-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15-# GNU General Public License for more details.
16-#
17-# You should have received a copy of the GNU General Public License
18-# along with this program; if not, write to the Free Software
19-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
20-
21-"""Helper functions for adding files to working trees."""
22-
23-import sys
24-
25-import bzrlib.osutils
26-
27-
28-class AddAction(object):
29- """A class which defines what action to take when adding a file."""
30-
31- def __init__(self, to_file=None, should_print=None):
32- """Initialize an action which prints added files to an output stream.
33-
34- :param to_file: The stream to write into. This is expected to take
35- Unicode paths. If not supplied, it will default to ``sys.stdout``.
36- :param should_print: If False, printing will be suppressed.
37- """
38- self._to_file = to_file
39- if to_file is None:
40- self._to_file = sys.stdout
41- self.should_print = False
42- if should_print is not None:
43- self.should_print = should_print
44-
45- def __call__(self, inv, parent_ie, path, kind, _quote=bzrlib.osutils.quotefn):
46- """Add path to inventory.
47-
48- The default action does nothing.
49-
50- :param inv: The inventory we are working with.
51- :param path: The FastPath being added
52- :param kind: The kind of the object being added.
53- """
54- if self.should_print:
55- self._to_file.write('adding %s\n' % _quote(path.raw_path))
56- return None
57-
58-
59-class AddFromBaseAction(AddAction):
60- """This class will try to extract file ids from another tree."""
61-
62- def __init__(self, base_tree, base_path, to_file=None, should_print=None):
63- super(AddFromBaseAction, self).__init__(to_file=to_file,
64- should_print=should_print)
65- self.base_tree = base_tree
66- self.base_path = base_path
67-
68- def __call__(self, inv, parent_ie, path, kind):
69- # Place the parent call
70- # Now check to see if we can extract an id for this file
71- file_id, base_path = self._get_base_file_id(path, parent_ie)
72- if file_id is not None:
73- if self.should_print:
74- self._to_file.write('adding %s w/ file id from %s\n'
75- % (path.raw_path, base_path))
76- else:
77- # we aren't doing anything special, so let the default
78- # reporter happen
79- file_id = super(AddFromBaseAction, self).__call__(
80- inv, parent_ie, path, kind)
81- return file_id
82-
83- def _get_base_file_id(self, path, parent_ie):
84- """Look for a file id in the base branch.
85-
86- First, if the base tree has the parent directory,
87- we look for a file with the same name in that directory.
88- Else, we look for an entry in the base tree with the same path.
89- """
90-
91- if (parent_ie.file_id in self.base_tree):
92- base_parent_ie = self.base_tree.inventory[parent_ie.file_id]
93- base_child_ie = base_parent_ie.children.get(path.base_path)
94- if base_child_ie is not None:
95- return (base_child_ie.file_id,
96- self.base_tree.id2path(base_child_ie.file_id))
97- full_base_path = bzrlib.osutils.pathjoin(self.base_path, path.raw_path)
98- # This may return None, but it is our last attempt
99- return self.base_tree.path2id(full_base_path), full_base_path
100
101=== modified file 'bzrlib/builtins.py'
102--- bzrlib/builtins.py 2010-10-15 11:30:54 +0000
103+++ bzrlib/builtins.py 2010-10-17 10:21:09 +0000
104@@ -627,9 +627,7 @@
105
106 def run(self, file_list, no_recurse=False, dry_run=False, verbose=False,
107 file_ids_from=None):
108- import bzrlib.add
109-
110- base_tree = None
111+ file_id_callback = report_callback = None
112 if file_ids_from is not None:
113 try:
114 base_tree, base_path = WorkingTree.open_containing(
115@@ -638,18 +636,23 @@
116 base_branch, base_path = Branch.open_containing(
117 file_ids_from)
118 base_tree = base_branch.basis_tree()
119-
120- action = bzrlib.add.AddFromBaseAction(base_tree, base_path,
121- to_file=self.outf, should_print=(not is_quiet()))
122- else:
123- action = bzrlib.add.AddAction(to_file=self.outf,
124- should_print=(not is_quiet()))
125-
126- if base_tree:
127 self.add_cleanup(base_tree.lock_read().unlock)
128+ if is_quiet():
129+ to_file = None
130+ else:
131+ to_file = self.outf
132+ file_id_callback = base_tree.file_id_suggestion(
133+ base_path, to_file)
134+ elif not is_quiet():
135+ def report_callback(ie, path):
136+ self.outf.write("adding " + path + "\n")
137 tree, file_list = tree_files_for_add(file_list)
138- added, ignored = tree.smart_add(file_list, not
139- no_recurse, action=action, save=not dry_run)
140+ added, ignored = tree.smart_add(
141+ file_list,
142+ recurse=(not no_recurse),
143+ file_id_callback=file_id_callback,
144+ save=not dry_run,
145+ report_callback=report_callback)
146 self.cleanup_now()
147 if len(ignored) > 0:
148 if verbose:
149
150=== modified file 'bzrlib/mutabletree.py'
151--- bzrlib/mutabletree.py 2010-08-17 06:45:33 +0000
152+++ bzrlib/mutabletree.py 2010-10-17 10:21:09 +0000
153@@ -30,9 +30,9 @@
154 bzrdir,
155 errors,
156 hooks,
157+ inventory,
158 osutils,
159 revisiontree,
160- inventory,
161 symbol_versioning,
162 trace,
163 tree,
164@@ -370,7 +370,13 @@
165 raise NotImplementedError(self.set_parent_trees)
166
167 @needs_tree_write_lock
168- def smart_add(self, file_list, recurse=True, action=None, save=True):
169+ def smart_add(self,
170+ file_list,
171+ recurse=True,
172+ action=None,
173+ save=True,
174+ file_id_callback=None,
175+ report_callback=None):
176 """Version file_list, optionally recursing into directories.
177
178 This is designed more towards DWIM for humans than API clarity.
179@@ -386,15 +392,22 @@
180 :param save: Save the inventory after completing the adds. If False
181 this provides dry-run functionality by doing the add and not saving
182 the inventory.
183+ :param file_id_callback: Optional,
184+ file_id_callback(parent_ie, path, kind) => file_id.
185+ :param report_callback: Optional, called after each file is added
186+ with (ie, path); result ignored.
187 :return: A tuple - files_added, ignored_files. files_added is the count
188 of added files, and ignored_files is a dict mapping files that were
189 ignored to the rule that caused them to be ignored.
190 """
191- # not in an inner loop; and we want to remove direct use of this,
192- # so here as a reminder for now. RBC 20070703
193- from bzrlib.inventory import InventoryEntry
194- if action is None:
195- action = add.AddAction()
196+ if action is not None:
197+ raise ValueError(
198+ "passing action to smart_add is no longer supported in bzr 2.3")
199+ elif file_id_callback is None:
200+ file_id_callback = lambda *a: None
201+
202+ if report_callback is None:
203+ report_callback = lambda *a: None
204
205 if not file_list:
206 # no paths supplied: add the entire tree.
207@@ -437,7 +450,7 @@
208 # schedule the dir for scanning
209 user_dirs.add(rf)
210 else:
211- if not InventoryEntry.versionable_kind(kind):
212+ if not inventory.InventoryEntry.versionable_kind(kind):
213 raise errors.BadFileKindError(filename=abspath, kind=kind)
214 # ensure the named path is added, so that ignore rules in the later
215 # directory walk dont skip it.
216@@ -446,7 +459,8 @@
217 versioned = inv.has_filename(rf.raw_path)
218 if versioned:
219 continue
220- added.extend(_add_one_and_parent(self, inv, None, rf, kind, action))
221+ added.extend(_add_one_and_parent(self, inv, None, rf, kind,
222+ file_id_callback, report_callback))
223
224 if not recurse:
225 # no need to walk any directories at all.
226@@ -478,7 +492,7 @@
227 # find the kind of the path being added.
228 kind = osutils.file_kind(abspath)
229
230- if not InventoryEntry.versionable_kind(kind):
231+ if not inventory.InventoryEntry.versionable_kind(kind):
232 trace.warning("skipping %s (can't add file of kind '%s')",
233 abspath, kind)
234 continue
235@@ -529,7 +543,8 @@
236 # 20070306
237 trace.mutter("%r is a nested bzr tree", abspath)
238 else:
239- _add_one(self, inv, parent_ie, directory, kind, action)
240+ _add_one(self, inv, parent_ie, directory, kind,
241+ file_id_callback, report_callback)
242 added.append(directory.raw_path)
243
244 if kind == 'directory' and not sub_tree:
245@@ -681,7 +696,8 @@
246 return hash(self.raw_path)
247
248
249-def _add_one_and_parent(tree, inv, parent_ie, path, kind, action):
250+def _add_one_and_parent(tree, inv, parent_ie, path, kind, file_id_callback,
251+ report_callback):
252 """Add a new entry to the inventory and automatically add unversioned parents.
253
254 :param inv: Inventory which will receive the new entry.
255@@ -689,7 +705,8 @@
256 None, the parent is looked up by name and used if present, otherwise it
257 is recursively added.
258 :param kind: Kind of new entry (file, directory, etc)
259- :param action: callback(inv, parent_ie, path, kind); return ignored.
260+ :param file_id_callback: callback(inv, parent_ie, path, kind); return a
261+ file_id or None to generate a new file id
262 :return: A list of paths which have been added.
263 """
264 # Nothing to do if path is already versioned.
265@@ -707,20 +724,24 @@
266 # there are a limited number of dirs we can be nested under, it should
267 # generally find it very fast and not recurse after that.
268 added = _add_one_and_parent(tree, inv, None,
269- _FastPath(osutils.dirname(path.raw_path)), 'directory', action)
270+ _FastPath(osutils.dirname(path.raw_path)), 'directory',
271+ file_id_callback,
272+ report_callback)
273 parent_id = inv.path2id(osutils.dirname(path.raw_path))
274 parent_ie = inv[parent_id]
275- _add_one(tree, inv, parent_ie, path, kind, action)
276+ _add_one(tree, inv, parent_ie, path, kind, file_id_callback,
277+ report_callback)
278 return added + [path.raw_path]
279
280
281-def _add_one(tree, inv, parent_ie, path, kind, file_id_callback):
282+def _add_one(tree, inv, parent_ie, path, kind, file_id_callback,
283+ report_callback):
284 """Add a new entry to the inventory.
285
286 :param inv: Inventory which will receive the new entry.
287 :param parent_ie: Parent inventory entry.
288 :param kind: Kind of new entry (file, directory, etc)
289- :param file_id_callback: callback(inv, parent_ie, path, kind); return a
290+ :param file_id_callback: callback(parent_ie, path, kind); return a
291 file_id or None to generate a new file id
292 :returns: None
293 """
294@@ -735,7 +756,8 @@
295 del inv[parent_ie.file_id]
296 inv.add(new_parent_ie)
297 parent_ie = new_parent_ie
298- file_id = file_id_callback(inv, parent_ie, path, kind)
299+ file_id = file_id_callback(parent_ie, path, kind)
300 entry = inv.make_entry(kind, path.base_path, parent_ie.file_id,
301 file_id=file_id)
302 inv.add(entry)
303+ report_callback(entry, path.raw_path)
304
305=== modified file 'bzrlib/tests/per_workingtree/test_smart_add.py'
306--- bzrlib/tests/per_workingtree/test_smart_add.py 2010-02-23 07:43:11 +0000
307+++ bzrlib/tests/per_workingtree/test_smart_add.py 2010-10-17 10:21:09 +0000
308@@ -20,7 +20,6 @@
309 import sys
310
311 from bzrlib import (
312- add,
313 errors,
314 ignores,
315 osutils,
316@@ -28,12 +27,16 @@
317 workingtree,
318 )
319 from bzrlib.tests import (
320- features,
321- test_smart_add,
322 per_workingtree,
323 )
324
325
326+def _make_custom_file_id(parent_ie, path, kind):
327+ return osutils.safe_file_id(kind + '-'
328+ + path.raw_path.replace('/', '%'),
329+ warn=False)
330+
331+
332 class TestSmartAddTree(per_workingtree.TestCaseWithWorkingTree):
333
334 def test_single_file(self):
335@@ -203,20 +206,9 @@
336 [path for path, ie in tree.iter_entries_by_dir()])
337
338 def test_custom_ids(self):
339- sio = StringIO()
340- action = test_smart_add.AddCustomIDAction(to_file=sio,
341- should_print=True)
342+ wt = self.make_branch_and_tree('.')
343 self.build_tree(['file1', 'dir1/', 'dir1/file2'])
344-
345- wt = self.make_branch_and_tree('.')
346- wt.smart_add(['.'], action=action)
347- # The order of adds is not strictly fixed:
348- sio.seek(0)
349- lines = sorted(sio.readlines())
350- self.assertEqualDiff(['added dir1 with id directory-dir1\n',
351- 'added dir1/file2 with id file-dir1%file2\n',
352- 'added file1 with id file-file1\n',
353- ], lines)
354+ wt.smart_add(['.'], file_id_callback=_make_custom_file_id)
355 wt.lock_read()
356 self.addCleanup(wt.unlock)
357 self.assertEqual([('', wt.path2id('')),
358
359=== modified file 'bzrlib/tests/test_smart_add.py'
360--- bzrlib/tests/test_smart_add.py 2010-02-10 17:52:08 +0000
361+++ bzrlib/tests/test_smart_add.py 2010-10-17 10:21:09 +0000
362@@ -18,26 +18,10 @@
363
364 from bzrlib import (
365 add,
366- inventory,
367- osutils,
368 tests,
369 )
370
371
372-class AddCustomIDAction(add.AddAction):
373-
374- def __call__(self, inv, parent_ie, path, kind):
375- # The first part just logs if appropriate
376- # Now generate a custom id
377- file_id = osutils.safe_file_id(kind + '-'
378- + path.raw_path.replace('/', '%'),
379- warn=False)
380- if self.should_print:
381- self._to_file.write('added %s with id %s\n'
382- % (path.raw_path, file_id))
383- return file_id
384-
385-
386 class TestAddFrom(tests.TestCaseWithTransport):
387 """Tests for AddFromBaseAction"""
388
389@@ -62,7 +46,8 @@
390 action = add.AddFromBaseAction(base_tree, base_path,
391 to_file=to_file,
392 should_print=should_print)
393- new_tree.smart_add(file_list, action=action)
394+ new_tree.smart_add(
395+ file_list, file_id_callback=action.generate_file_id)
396 finally:
397 new_tree.unlock()
398 finally:
399@@ -141,22 +126,3 @@
400 self.base_tree.lock_read()
401 self.addCleanup(self.base_tree.unlock)
402 self.failIf(a_id in self.base_tree)
403-
404-
405-class TestAddActions(tests.TestCase):
406-
407- def test_quiet(self):
408- self.run_action("")
409-
410- def test__print(self):
411- self.run_action("adding path\n")
412-
413- def run_action(self, output):
414- from bzrlib.mutabletree import _FastPath
415- inv = inventory.Inventory()
416- stdout = StringIO()
417- action = add.AddAction(to_file=stdout, should_print=bool(output))
418-
419- self.apply_redirected(None, stdout, None, action, inv, None,
420- _FastPath('path'), 'file')
421- self.assertEqual(stdout.getvalue(), output)
422
423=== modified file 'bzrlib/tree.py'
424--- bzrlib/tree.py 2010-08-31 07:12:18 +0000
425+++ bzrlib/tree.py 2010-10-17 10:21:09 +0000
426@@ -37,7 +37,10 @@
427 from bzrlib.inter import InterObject
428 from bzrlib.osutils import fingerprint_file
429 from bzrlib.symbol_versioning import deprecated_function, deprecated_in
430-from bzrlib.trace import note
431+from bzrlib.trace import (
432+ mutter,
433+ note,
434+ )
435
436
437 class Tree(object):
438@@ -588,6 +591,49 @@
439 """
440 pass
441
442+ def file_id_suggestion(self, under_dir, print_to_file):
443+ """Return a callback to use with smart_add file_id_callback.
444+
445+ The returned callable tries to guess a file id for a newly added
446+ file in a mutable tree, based on finding one at the same path in
447+ this tree.
448+
449+ First, if this tree has the same parent directory id,
450+ we look for a file with the same name in that directory.
451+ Otherwise, we look for an entry in this tree with the same path.
452+
453+ :param under_path: Look for file ids under the given directory.
454+ :param print_to_file: If non-None, write a message about matched
455+ files to this stream.
456+ """
457+ def find_one(parent_ie, path, kind):
458+ # mutter("look up suggestion for %r in parent %r"
459+ # % (path.raw_path, parent_ie))
460+ file_id = None
461+ if parent_ie.file_id in self:
462+ # mutter("found parent in %r" % self)
463+ base_parent_ie = self.inventory[parent_ie.file_id]
464+ # mutter(" looking for child %r" % path.base_path)
465+ base_child_ie = base_parent_ie.children.get(path.base_path)
466+ if base_child_ie is not None:
467+ # mutter(" found child under it")
468+ file_id = base_child_ie.file_id
469+ origin_path = self.id2path(base_child_ie.file_id)
470+ if file_id is None:
471+ # Not found yet: try looking for the whole path.
472+ origin_path = osutils.pathjoin(under_dir, path.raw_path)
473+ file_id = self.path2id(origin_path)
474+ if print_to_file is None:
475+ pass
476+ elif file_id is not None:
477+ print_to_file.write(
478+ 'adding %s w/ file id from %s\n'
479+ % (path.raw_path, origin_path))
480+ else:
481+ print_to_file.write('adding %s\n' % path.raw_path)
482+ return file_id
483+ return find_one
484+
485 def revision_tree(self, revision_id):
486 """Obtain a revision tree for the revision revision_id.
487
488
489=== modified file 'doc/en/release-notes/bzr-2.3.txt'
490--- doc/en/release-notes/bzr-2.3.txt 2010-10-15 15:05:09 +0000
491+++ doc/en/release-notes/bzr-2.3.txt 2010-10-17 10:21:09 +0000
492@@ -72,6 +72,12 @@
493 deprecated in favour of ``known_hooks.key_to_parent_and_attribute`` in
494 the same module. (Andrew Bennetts)
495
496+* The ``action`` parameter to ``MutableTree.smart_add`` is no longer
497+ supported and the AddAction classes have been removed. Instead,
498+ file ids can be generated by passing a ``file_id_callback``, and output
499+ can be shown through the ``report_callback``.
500+ (Martin Pool)
501+
502 Internals
503 *********
504