Merge lp:~bzr/bzr/iter-changes-excluding into lp:~bzr/bzr/trunk-old

Proposed by Ian Clatworthy
Status: Rejected
Rejected by: Ian Clatworthy
Proposed branch: lp:~bzr/bzr/iter-changes-excluding
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 116 lines
To merge this branch: bzr merge lp:~bzr/bzr/iter-changes-excluding
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing
Review via email: mp+6674@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

Another building block for faster selective file commit.

Note: 'exclude_files' will probably become a parameter to iter_changes() in the medium to long term. Implementing it efficiently across the half dozen implementations of iter_changes() is quite a bit of work though and not without risk. In comparison, this decorator is much simpler to code and test. It is also useful in its own right and, above all, helps deliver a large performance gain for faster selective commit real soon now.

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

It would be nice to have a test that made sure that old paths (in case of removed files) are treated correctly. Other than, that it seems good to me.

  vote tweak

Revision history for this message
Robert Collins (lifeless) wrote :

I would 'review resubmit' I think, but last time I tried it bounced the
review instead :(.

Conceptually fine. A few specific concerns:
 - should it expand out across renames? [I suspect yes]. If yes, the
tests need to get boundary cases for this. If no, the tests should
demonstrate that choice.
 - The test has 'magic' in it:
   tree, do stuff, compare to an iter_changes list.
   It would be a lot clearer to do:
   here is an iter_changes list (hardcoded even), filter, compare.

-Rob

Revision history for this message
John A Meinel (jameinel) wrote :

I'm not sure if this is still up for review or not, as the status says "in progress".

Anyway, I believe there is a logic bug with:

+ if exclude_files:
+ return _filter_iter_changes_excluding(changes, exclude_files)
+ else:
+ return changes

Specifically, I believe if you have:

exclude_files = []

Then it should actually filter *all* files, not return all changes.

Also, as for:
def _filter_iter_changes_excluding(changes, exclude_files):

If old_path != new_path, shouldn't it filter based on both sides?

I'm not 100% sure how --exclude works, though. But if I did "bzr mv a/foo b/foo" "bzr commit --exclude a" should/shouldn't that include b/foo?

review: Needs Fixing
Revision history for this message
Robert Collins (lifeless) wrote :

On Mon, 2009-06-01 at 20:23 +0000, John A Meinel wrote:
>
> I'm not 100% sure how --exclude works, though. But if I did "bzr mv
> a/foo b/foo" "bzr commit --exclude a" should/shouldn't that include
> b/foo?
It should exclude b (and thus b/foo).

Rob
--

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
> On Mon, 2009-06-01 at 20:23 +0000, John A Meinel wrote:
>> I'm not 100% sure how --exclude works, though. But if I did "bzr mv
>> a/foo b/foo" "bzr commit --exclude a" should/shouldn't that include
>> b/foo?
> It should exclude b (and thus b/foo).
>
> Rob

"bzr commit --exclude a" should exclude b?

I don't quite follow.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkokgZgACgkQJdeBCYSNAANHrACePl1ySWrnXUliBPat4NTLgg6O
RDcAoLIpOV/p8JaO+qOT31vV3a06iUWl
=Bu6T
-----END PGP SIGNATURE-----

Revision history for this message
Robert Collins (lifeless) wrote :

On Tue, 2009-06-02 at 01:36 +0000, John A Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Robert Collins wrote:
> > On Mon, 2009-06-01 at 20:23 +0000, John A Meinel wrote:
> >> I'm not 100% sure how --exclude works, though. But if I did "bzr mv
> >> a/foo b/foo" "bzr commit --exclude a" should/shouldn't that include
> >> b/foo?
> > It should exclude b (and thus b/foo).
> >
> > Rob
>
> "bzr commit --exclude a" should exclude b?
>
> I don't quite follow.

I misread your example.

bzr init
bzr mkdir a
bzr mkdir b
touch a/foo
bzr add
bzr commit -m '1'
bzr mv a/foo b/
bzr commit --exclude a
ERROR: no changes

-Rob

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/test_tree.py'
2--- bzrlib/tests/test_tree.py 2009-08-28 05:00:33 +0000
3+++ bzrlib/tests/test_tree.py 2009-08-31 04:38:22 +0000
4@@ -23,7 +23,7 @@
5 tree as _mod_tree,
6 )
7 from bzrlib.tests import TestCaseWithTransport
8-from bzrlib.tree import InterTree
9+from bzrlib.tree import InterTree, filter_iter_changes_excluding
10
11
12 class TestInterTree(TestCaseWithTransport):
13@@ -417,3 +417,52 @@
14 self.assertPathToKey(([u''], u'a'), u'a')
15 self.assertPathToKey(([u'a'], u'b'), u'a/b')
16 self.assertPathToKey(([u'a', u'b'], u'c'), u'a/b/c')
17+
18+
19+class TestFilterIterChangesExcluding(TestCaseWithTransport):
20+ """Tests for tree.filter_iter_changes_excluding()."""
21+
22+ def make_changes_iter(self, specific_files=None):
23+ wt = self.make_branch_and_tree('.')
24+ b = wt.branch
25+ self.build_tree(['foo/', 'foo/foo1', 'bar/', 'bar/bar1', 'bar/bar2',
26+ 'baz'])
27+ wt.add(['foo', 'foo/foo1', 'bar', 'bar/bar1', 'bar/bar2', 'baz'],
28+ ['foo-id', 'foo1-id', 'bar-id', 'bar1-id', 'bar2-id', 'baz-id'])
29+ wt.commit('bar/bar1', specific_files=['bar/bar1'], rev_id='1')
30+ basis = wt.basis_tree()
31+ wt.lock_read()
32+ self.addCleanup(wt.unlock)
33+ basis.lock_read()
34+ self.addCleanup(basis.unlock)
35+ return wt.iter_changes(basis, specific_files=specific_files)
36+
37+ def test_filter_iter_changes_excluding_no_filtering(self):
38+ original = self.make_changes_iter()
39+ original_list = list(original)
40+ filtered = filter_iter_changes_excluding(iter(original_list), None)
41+ self.assertEqual(original_list, list(filtered))
42+
43+ def test_filter_iter_changes_excluding(self):
44+ original = self.make_changes_iter()
45+ filtered = filter_iter_changes_excluding(original, ['foo'])
46+ self.assertEqual([
47+ ('baz-id', (None, u'baz'), True, (False, True),
48+ (None, 'TREE_ROOT'), (None, u'baz'), (None, 'file'), (None, 0)),
49+ ('bar2-id', (None, u'bar/bar2'), True, (False, True),
50+ (None, 'bar-id'), (None, u'bar2'), (None, 'file'), (None, 0)),
51+ ], list(filtered))
52+
53+ def test_filter_iter_changes_excluding_within_specific(self):
54+ original = self.make_changes_iter(specific_files=['foo'])
55+ filtered = filter_iter_changes_excluding(original, ['foo/foo1'])
56+ self.assertEqual([
57+ ('foo-id', (None, u'foo'), True, (False, True),
58+ (None, 'TREE_ROOT'), (None, u'foo'), (None, 'directory'),
59+ (None, 0)),
60+ ], list(filtered))
61+
62+ def test_filter_iter_changes_excluding_override_specific(self):
63+ original = self.make_changes_iter(specific_files=['bar/bar2'])
64+ filtered = filter_iter_changes_excluding(original, ['bar/bar2'])
65+ self.assertEqual([], list(filtered))
66
67=== modified file 'bzrlib/tree.py'
68--- bzrlib/tree.py 2009-08-28 05:00:33 +0000
69+++ bzrlib/tree.py 2009-08-31 04:38:22 +0000
70@@ -35,7 +35,7 @@
71 from bzrlib import errors
72 from bzrlib.inventory import InventoryFile
73 from bzrlib.inter import InterObject
74-from bzrlib.osutils import fingerprint_file
75+from bzrlib.osutils import fingerprint_file, is_inside_any
76 import bzrlib.revision
77 from bzrlib.symbol_versioning import deprecated_function, deprecated_in
78 from bzrlib.trace import note
79@@ -1461,3 +1461,37 @@
80 other_values.append(self._lookup_by_file_id(
81 alt_extra, alt_tree, file_id))
82 yield other_path, file_id, None, other_values
83+
84+
85+def filter_iter_changes_excluding(changes, exclude_files):
86+ """Filter the results from iter_changes by excluding paths.
87+
88+ This decorator is useful for post-processing the output from
89+ Tree.iter_changes(). It may also be used for post-procesing the output
90+ from result-compatible methods such as Inventory.iter_changes() and
91+ PreviewTree.iter_changes().
92+
93+ :param changes: the iterator of changes to filter
94+ :param exclude_files: paths of files and directories to exclude or
95+ None to disable.
96+ """
97+ if exclude_files:
98+ return _filter_iter_changes_excluding(changes, exclude_files)
99+ else:
100+ return changes
101+
102+
103+def _filter_iter_changes_excluding(changes, exclude_files):
104+ exclude_set = osutils.minimum_path_selection(exclude_files)
105+ for change in changes:
106+ # Decide which path to use
107+ old_path, new_path = change[1]
108+ if new_path is None:
109+ path = old_path
110+ else:
111+ path = new_path
112+
113+ # Do the filtering
114+ if exclude_set and is_inside_any(exclude_set, path):
115+ continue
116+ yield change