Bazaar

Merge lp:~ian-clatworthy/bzr/385879 into lp:bzr/2.0

  1. 385879
  2. Merge into 2.0
Proposed by Ian Clatworthy
Status: Merged
Approved by: Ian Clatworthy
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~ian-clatworthy/bzr/385879
Merge into: lp:bzr/2.0
Diff against target: 260 lines
4 files modified
NEWS (+3/-0)
bzrlib/merge.py (+33/-4)
bzrlib/tests/per_workingtree/test_content_filters.py (+117/-0)
bzrlib/transform.py (+13/-2)
To merge this branch: bzr merge lp:~ian-clatworthy/bzr/385879
Reviewer Review Type Date Requested Status
Ian Clatworthy Approve
Robert Collins (community) Needs Fixing
Review via email: mp+14122@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

This patch fixes pull, merge and switch on trees with content filtering. It covers the most common changes: modifications, additions and deletions. Once this patch lands, content filtering should work for most people most of the time.

There are still potential problems when changes involve a rename which take files into or out of the filtered set. Those issues exist now though and this patch doesn't make them worse.

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

I'm finding it a bit disturbing how much code gets "if wt.supports_content_filtering():" spread through it, it's not suggesting that this is properly factored and as a consquence we will probably keep squashing bugs all over the place. Incremental improvements are still good but could you think about whether this could be done within a tree and opaque to the rest of the code?

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

This really smells wrong: tree transform can figure out the path for things itself, there are layering violations going on here. I also second Poolies comments.

review: Needs Fixing
Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

Martin has agreed this is ok to merge. His comments above were "nice to have", not blockers to landing.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-10-28 01:16:10 +0000
3+++ NEWS 2009-10-28 23:40:29 +0000
4@@ -23,6 +23,9 @@
5 * Avoid "NoneType has no attribute st_mode" error when files disappear
6 from a directory while it's being read. (Martin Pool, #446033)
7
8+* Content filters are now applied correctly after pull, merge and switch.
9+ (Ian Clatworthy, #385879)
10+
11 * Content filters are now applied correctly after revert.
12 (Ian Clatworthy)
13
14
15=== modified file 'bzrlib/merge.py'
16--- bzrlib/merge.py 2009-09-11 21:19:32 +0000
17+++ bzrlib/merge.py 2009-10-28 23:40:29 +0000
18@@ -1162,8 +1162,22 @@
19 self.tt.delete_contents(trans_id)
20 if file_id in self.other_tree:
21 # OTHER changed the file
22+ wt = self.this_tree
23+ if wt.supports_content_filtering():
24+ # We get the path from the working tree if it exists.
25+ # That fails though when OTHER is adding a file, so
26+ # we fall back to the other tree to find the path if
27+ # it doesn't exist locally.
28+ try:
29+ filter_tree_path = wt.id2path(file_id)
30+ except errors.NoSuchId:
31+ filter_tree_path = self.other_tree.id2path(file_id)
32+ else:
33+ # Skip the id2path lookup for older formats
34+ filter_tree_path = None
35 create_from_tree(self.tt, trans_id,
36- self.other_tree, file_id)
37+ self.other_tree, file_id,
38+ filter_tree_path=filter_tree_path)
39 if not file_in_this:
40 self.tt.version_file(file_id, trans_id)
41 return "modified"
42@@ -1256,12 +1270,26 @@
43 ('THIS', self.this_tree, this_lines)]
44 if not no_base:
45 data.append(('BASE', self.base_tree, base_lines))
46+
47+ # We need to use the actual path in the working tree of the file here,
48+ # ignoring the conflict suffixes
49+ wt = self.this_tree
50+ if wt.supports_content_filtering():
51+ try:
52+ filter_tree_path = wt.id2path(file_id)
53+ except errors.NoSuchId:
54+ # file has been deleted
55+ filter_tree_path = None
56+ else:
57+ # Skip the id2path lookup for older formats
58+ filter_tree_path = None
59+
60 versioned = False
61 file_group = []
62 for suffix, tree, lines in data:
63 if file_id in tree:
64 trans_id = self._conflict_file(name, parent_id, tree, file_id,
65- suffix, lines)
66+ suffix, lines, filter_tree_path)
67 file_group.append(trans_id)
68 if set_version and not versioned:
69 self.tt.version_file(file_id, trans_id)
70@@ -1269,11 +1297,12 @@
71 return file_group
72
73 def _conflict_file(self, name, parent_id, tree, file_id, suffix,
74- lines=None):
75+ lines=None, filter_tree_path=None):
76 """Emit a single conflict file."""
77 name = name + '.' + suffix
78 trans_id = self.tt.create_path(name, parent_id)
79- create_from_tree(self.tt, trans_id, tree, file_id, lines)
80+ create_from_tree(self.tt, trans_id, tree, file_id, lines,
81+ filter_tree_path)
82 return trans_id
83
84 def merge_executable(self, file_id, file_status):
85
86=== modified file 'bzrlib/tests/per_workingtree/test_content_filters.py'
87--- bzrlib/tests/per_workingtree/test_content_filters.py 2009-10-28 01:16:10 +0000
88+++ bzrlib/tests/per_workingtree/test_content_filters.py 2009-10-28 23:40:29 +0000
89@@ -18,7 +18,9 @@
90
91 import os
92
93+from bzrlib.bzrdir import BzrDir
94 from bzrlib.filters import ContentFilter
95+from bzrlib.switch import switch
96 from bzrlib.workingtree import WorkingTree
97 from bzrlib.tests.per_workingtree import TestCaseWithWorkingTree
98
99@@ -83,6 +85,38 @@
100 bin_fileid = tree.path2id('file2.bin')
101 return tree, txt_fileid, bin_fileid
102
103+ def create_cf_tree_with_two_revisions(self, txt_reader, txt_writer,
104+ dir='.'):
105+ tree = self.make_branch_and_tree(dir)
106+ def _content_filter_stack(path=None, file_id=None):
107+ if path.endswith('.txt'):
108+ return [ContentFilter(txt_reader, txt_writer)]
109+ else:
110+ return []
111+ tree._content_filter_stack = _content_filter_stack
112+ self.build_tree_contents([
113+ (dir + '/file1.txt', 'Foo Txt'),
114+ (dir + '/file2.bin', 'Foo Bin'),
115+ (dir + '/file3.txt', 'Bar Txt'),
116+ ])
117+ tree.add(['file1.txt', 'file2.bin', 'file3.txt'])
118+ tree.commit('commit raw content')
119+ fileid_1 = tree.path2id('file1.txt')
120+ fileid_2 = tree.path2id('file2.bin')
121+ fileid_3 = tree.path2id('file3.txt')
122+ # Commit another revision with various changes. We make sure
123+ # the change includes a modification, an addition and a deletion.
124+ # Renames are more complex and need a separate set of tests later.
125+ self.build_tree_contents([
126+ (dir + '/file1.txt', 'Foo ROCKS!'),
127+ (dir + '/file4.txt', 'Hello World'),
128+ ])
129+ tree.add(['file4.txt'])
130+ tree.remove(['file3.txt'], keep_files=False)
131+ tree.commit("change, add and rename stuff")
132+ fileid_4 = tree.path2id('file4.txt')
133+ return tree, fileid_1, fileid_2, fileid_3, fileid_4
134+
135 def patch_in_content_filter(self):
136 # Patch in a custom, symmetric content filter stack. It's pretty gross
137 # that we need to monkey-patch a class method to do this, bit it's
138@@ -222,6 +256,89 @@
139 # that will be expensive to compute, so it's acceptable to just return
140 # None.
141
142+ def test_content_filtering_applied_on_pull(self):
143+ # Create a source branch with two revisions
144+ source, fileid_1, fileid_2, fileid_3, fileid_4 = \
145+ self.create_cf_tree_with_two_revisions(txt_reader=None,
146+ txt_writer=None, dir='source')
147+ if not source.supports_content_filtering():
148+ return
149+ self.assertFileEqual("Foo ROCKS!", 'source/file1.txt')
150+ self.assert_basis_content("Foo ROCKS!", source, fileid_1)
151+
152+ # Now patch in content filtering and branch from revision 1
153+ self.patch_in_content_filter()
154+ self.run_bzr('branch -r1 source target')
155+ target = WorkingTree.open('target')
156+ self.assert_basis_content("Foo Txt", target, fileid_1)
157+ self.assertFileEqual("fOO tXT", 'target/file1.txt')
158+ self.assert_basis_content("Foo Bin", target, fileid_2)
159+ self.assertFileEqual("Foo Bin", 'target/file2.bin')
160+ self.assert_basis_content("Bar Txt", target, fileid_3)
161+ self.assertFileEqual("bAR tXT", 'target/file3.txt')
162+
163+ # Pull the latter change and check the target tree is updated
164+ self.run_bzr('pull -d target')
165+ self.assert_basis_content("Foo ROCKS!", target, fileid_1)
166+ self.assertFileEqual("fOO rocks!", 'target/file1.txt')
167+ self.assert_basis_content("Foo Bin", target, fileid_2)
168+ self.assert_basis_content("Hello World", target, fileid_4)
169+ self.assertFileEqual("hELLO wORLD", 'target/file4.txt')
170+
171+ def test_content_filtering_applied_on_merge(self):
172+ # Create a source branch with two revisions
173+ source, fileid_1, fileid_2, fileid_3, fileid_4 = \
174+ self.create_cf_tree_with_two_revisions(txt_reader=None,
175+ txt_writer=None, dir='source')
176+ if not source.supports_content_filtering():
177+ return
178+ self.assert_basis_content("Foo ROCKS!", source, fileid_1)
179+ self.assertFileEqual("Foo ROCKS!", 'source/file1.txt')
180+ self.assert_basis_content("Foo Bin", source, fileid_2)
181+ self.assert_basis_content("Hello World", source, fileid_4)
182+ self.assertFileEqual("Hello World", 'source/file4.txt')
183+
184+ # Now patch in content filtering and branch from revision 1
185+ self.patch_in_content_filter()
186+ self.run_bzr('branch -r1 source target')
187+ target = WorkingTree.open('target')
188+ self.assert_basis_content("Foo Txt", target, fileid_1)
189+ self.assertFileEqual("fOO tXT", 'target/file1.txt')
190+ self.assertFileEqual("Foo Bin", 'target/file2.bin')
191+ self.assertFileEqual("bAR tXT", 'target/file3.txt')
192+
193+ # Merge the latter change and check the target tree is updated
194+ self.run_bzr('merge -d target source')
195+ self.assertFileEqual("fOO rocks!", 'target/file1.txt')
196+ self.assertFileEqual("hELLO wORLD", 'target/file4.txt')
197+
198+ # Commit the merge and check the right content is stored
199+ target.commit("merge file1.txt changes from source")
200+ self.assert_basis_content("Foo ROCKS!", target, fileid_1)
201+ self.assert_basis_content("Hello World", target, fileid_4)
202+
203+ def test_content_filtering_applied_on_switch(self):
204+ # Create a source branch with two revisions
205+ source, fileid_1, fileid_2, fileid_3, fileid_4 = \
206+ self.create_cf_tree_with_two_revisions(txt_reader=None,
207+ txt_writer=None, dir='branch-a')
208+ if not source.supports_content_filtering():
209+ return
210+
211+ # Now patch in content filtering and branch from revision 1
212+ self.patch_in_content_filter()
213+ self.run_bzr('branch -r1 branch-a branch-b')
214+
215+ # Now create a lightweight checkout referring to branch-b
216+ self.run_bzr('checkout --lightweight branch-b checkout')
217+ self.assertFileEqual("fOO tXT", 'checkout/file1.txt')
218+
219+ # Switch it to branch-b and check the tree is updated
220+ checkout_control_dir = BzrDir.open_containing('checkout')[0]
221+ switch(checkout_control_dir, source.branch)
222+ self.assertFileEqual("fOO rocks!", 'checkout/file1.txt')
223+ self.assertFileEqual("hELLO wORLD", 'checkout/file4.txt')
224+
225 def test_content_filtering_applied_on_revert_delete(self):
226 # Create a source branch with content filtering
227 source, txt_fileid, bin_fileid = self.create_cf_tree(
228
229=== modified file 'bzrlib/transform.py'
230--- bzrlib/transform.py 2009-10-27 12:06:56 +0000
231+++ bzrlib/transform.py 2009-10-28 23:40:29 +0000
232@@ -2420,8 +2420,14 @@
233 tt.create_directory(trans_id)
234
235
236-def create_from_tree(tt, trans_id, tree, file_id, bytes=None):
237- """Create new file contents according to tree contents."""
238+def create_from_tree(tt, trans_id, tree, file_id, bytes=None,
239+ filter_tree_path=None):
240+ """Create new file contents according to tree contents.
241+
242+ :param filter_tree_path: the tree path to use to lookup
243+ content filters to apply to the bytes output in the working tree.
244+ This only applies if the working tree supports content filtering.
245+ """
246 kind = tree.kind(file_id)
247 if kind == 'directory':
248 tt.create_directory(trans_id)
249@@ -2432,6 +2438,11 @@
250 bytes = tree_file.readlines()
251 finally:
252 tree_file.close()
253+ wt = tt._tree
254+ if wt.supports_content_filtering() and filter_tree_path is not None:
255+ filters = wt._content_filter_stack(filter_tree_path)
256+ bytes = filtered_output_bytes(bytes, filters,
257+ ContentFilterContext(filter_tree_path, tree))
258 tt.create_file(bytes, trans_id)
259 elif kind == "symlink":
260 tt.create_symlink(tree.get_symlink_target(file_id), trans_id)

Subscribers

People subscribed via source and target branches