Bazaar

Merge lp:~ian-clatworthy/bzr/filtering-revert-bug into lp:bzr/2.0

  1. filtering-revert-bug
  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/filtering-revert-bug
Merge into: lp:bzr/2.0
Diff against target: 172 lines
3 files modified
NEWS (+3/-0)
bzrlib/tests/per_workingtree/test_content_filters.py (+71/-22)
bzrlib/transform.py (+16/-3)
To merge this branch: bzr merge lp:~ian-clatworthy/bzr/filtering-revert-bug
Reviewer Review Type Date Requested Status
Martin Pool Approve
Review via email: mp+14022@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

This patch fixes revert on trees with content filters. I've split this bit out from an earlier, more complex patch. This version explains the small code change in a bit more detail and adds a test for reverting a rename in addition to the one previously proposed (reverting a delete).

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

> This patch fixes revert on trees with content filters. I've split this bit out
> from an earlier, more complex patch. This version explains the small code
> change in a bit more detail and adds a test for reverting a rename in addition
> to the one previously proposed (reverting a delete).

This seems like a correctness improvement but also a bit kludgey. Maybe we should merge it now but look to clean it up later.

Storing the content filters in a class-wide variable seems very strange.

Surely there should be some object that takes care of doing filtering if appropriate, rather than this being inline within tt?

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

(after irc discussion)

What I'm referring to with the comment about globals is the way the tests save and restore WorkingTree._content_filter_stack. They're relying on the way that class variables underlie instance variables, which (while it works) has been shown to be fragile or confusing.

However, this isn't new code in this patch so I won't insist it be fixed here. I do think it would be good to add a comment since we've talked about it.

Thanks.

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

Hi,

As part of the SRU process, we need to attribute changes in the branch
to bugs. However, there was no bug for this particular change. For
things going into the stable series, please (everyone) make sure there
is one.

--
Martin <http://launchpad.net/~mbp/>

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

On Mon, 2009-12-14 at 08:27 +0000, Martin Pool wrote:
> Hi,
>
> As part of the SRU process, we need to attribute changes in the branch
> to bugs. However, there was no bug for this particular change. For
> things going into the stable series, please (everyone) make sure there
> is one.

I've replied on-list. I don't think this, as currently proposed, will
really do what the Ubuntu folk are looking for.

If you see https://wiki.ubuntu.com/StableReleaseUpdates, you will see
that 'new upstreams' are explicitly rejected as reasons for SRU's - and
that is perhaps where the tension is coming from.

The wiki page does not explicitly say, because its written from an
Ubuntu dev perspective, but the bugs referenced there may need to be
*Ubuntu* bugs, not upstream bugs. However,
https://wiki.ubuntu.com/LandscapeUpdates and
https://lists.ubuntu.com/archives/ubuntu-devel-announce/2009-March/000550.html have more detail on a blanket exception prepare for landscape-client, and they want upstream bugs. So its possible that things could go either way. Note that landscape also needs a branch per bug, and so forth.

All that said, I suspect that its mostly paranoia and the changes are
not examined in the degree of detail that policy suggests might be the
case. (though I am sure they are examined).

-Rob

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2009-10-21 01:05:40 +0000
+++ NEWS 2009-10-27 12:15:24 +0000
@@ -23,6 +23,9 @@
23* Avoid "NoneType has no attribute st_mode" error when files disappear23* Avoid "NoneType has no attribute st_mode" error when files disappear
24 from a directory while it's being read. (Martin Pool, #446033)24 from a directory while it's being read. (Martin Pool, #446033)
2525
26* Content filters are now applied correctly after revert.
27 (Ian Clatworthy)
28
26* Diff parsing handles "Binary files differ" hunks. (Aaron Bentley, #436325)29* Diff parsing handles "Binary files differ" hunks. (Aaron Bentley, #436325)
2730
28* Fetching from stacked pre-2a repository via a smart server no longer31* Fetching from stacked pre-2a repository via a smart server no longer
2932
=== modified file 'bzrlib/tests/per_workingtree/test_content_filters.py'
--- bzrlib/tests/per_workingtree/test_content_filters.py 2009-08-25 04:43:21 +0000
+++ bzrlib/tests/per_workingtree/test_content_filters.py 2009-10-27 12:15:24 +0000
@@ -16,6 +16,8 @@
1616
17"""Tests for content filtering conformance"""17"""Tests for content filtering conformance"""
1818
19import os
20
19from bzrlib.filters import ContentFilter21from bzrlib.filters import ContentFilter
20from bzrlib.workingtree import WorkingTree22from bzrlib.workingtree import WorkingTree
21from bzrlib.tests.per_workingtree import TestCaseWithWorkingTree23from bzrlib.tests.per_workingtree import TestCaseWithWorkingTree
@@ -81,6 +83,29 @@
81 bin_fileid = tree.path2id('file2.bin')83 bin_fileid = tree.path2id('file2.bin')
82 return tree, txt_fileid, bin_fileid84 return tree, txt_fileid, bin_fileid
8385
86 def patch_in_content_filter(self):
87 # Patch in a custom, symmetric content filter stack
88 self.real_content_filter_stack = WorkingTree._content_filter_stack
89 def restore_real_content_filter_stack():
90 WorkingTree._content_filter_stack = self.real_content_filter_stack
91 self.addCleanup(restore_real_content_filter_stack)
92 def _content_filter_stack(tree, path=None, file_id=None):
93 if path.endswith('.txt'):
94 return [ContentFilter(_swapcase, _swapcase)]
95 else:
96 return []
97 WorkingTree._content_filter_stack = _content_filter_stack
98
99 def assert_basis_content(self, expected_content, branch, file_id):
100 # Note: We need to use try/finally here instead of addCleanup()
101 # as the latter leaves the read lock in place too long
102 basis = branch.basis_tree()
103 basis.lock_read()
104 try:
105 self.assertEqual(expected_content, basis.get_file_text(file_id))
106 finally:
107 basis.unlock()
108
84 def test_symmetric_content_filtering(self):109 def test_symmetric_content_filtering(self):
85 # test handling when read then write gives back the initial content110 # test handling when read then write gives back the initial content
86 tree, txt_fileid, bin_fileid = self.create_cf_tree(111 tree, txt_fileid, bin_fileid = self.create_cf_tree(
@@ -132,10 +157,7 @@
132 if not source.supports_content_filtering():157 if not source.supports_content_filtering():
133 return158 return
134 self.assertFileEqual("Foo Txt", 'source/file1.txt')159 self.assertFileEqual("Foo Txt", 'source/file1.txt')
135 basis = source.basis_tree()160 self.assert_basis_content("FOO TXT", source, txt_fileid)
136 basis.lock_read()
137 self.addCleanup(basis.unlock)
138 self.assertEqual("FOO TXT", basis.get_file_text(txt_fileid))
139161
140 # Now branch it162 # Now branch it
141 self.run_bzr('branch source target')163 self.run_bzr('branch source target')
@@ -153,24 +175,10 @@
153 if not source.supports_content_filtering():175 if not source.supports_content_filtering():
154 return176 return
155 self.assertFileEqual("Foo Txt", 'source/file1.txt')177 self.assertFileEqual("Foo Txt", 'source/file1.txt')
156 basis = source.basis_tree()178 self.assert_basis_content("Foo Txt", source, txt_fileid)
157 basis.lock_read()179
158 self.addCleanup(basis.unlock)180 # Now patch in content filtering and branch the source
159 self.assertEqual("Foo Txt", basis.get_file_text(txt_fileid))181 self.patch_in_content_filter()
160
161 # Patch in a custom, symmetric content filter stack
162 self.real_content_filter_stack = WorkingTree._content_filter_stack
163 def restore_real_content_filter_stack():
164 WorkingTree._content_filter_stack = self.real_content_filter_stack
165 self.addCleanup(restore_real_content_filter_stack)
166 def _content_filter_stack(tree, path=None, file_id=None):
167 if path.endswith('.txt'):
168 return [ContentFilter(_swapcase, _swapcase)]
169 else:
170 return []
171 WorkingTree._content_filter_stack = _content_filter_stack
172
173 # Now branch it
174 self.run_bzr('branch source target')182 self.run_bzr('branch source target')
175 target = WorkingTree.open('target')183 target = WorkingTree.open('target')
176 # Even though the content in source and target are different184 # Even though the content in source and target are different
@@ -209,3 +217,44 @@
209 # we could give back the length of the canonical form, but in general217 # we could give back the length of the canonical form, but in general
210 # that will be expensive to compute, so it's acceptable to just return218 # that will be expensive to compute, so it's acceptable to just return
211 # None.219 # None.
220
221 def test_content_filtering_applied_on_revert_delete(self):
222 # Create a source branch with content filtering
223 source, txt_fileid, bin_fileid = self.create_cf_tree(
224 txt_reader=_uppercase, txt_writer=_lowercase, dir='source')
225 if not source.supports_content_filtering():
226 return
227 self.assertFileEqual("Foo Txt", 'source/file1.txt')
228 self.assert_basis_content("FOO TXT", source, txt_fileid)
229
230 # Now delete the file, revert it and check the content
231 os.unlink('source/file1.txt')
232 self.assertFalse(os.path.exists('source/file1.txt'))
233 source.revert(['file1.txt'])
234 self.assertTrue(os.path.exists('source/file1.txt'))
235 # Note: we don't get back exactly what was in the tree
236 # previously because lower(upper(text)) is a lossy transformation
237 self.assertFileEqual("foo txt", 'source/file1.txt')
238
239 def test_content_filtering_applied_on_revert_rename(self):
240 # Create a source branch with content filtering
241 source, txt_fileid, bin_fileid = self.create_cf_tree(
242 txt_reader=_uppercase, txt_writer=_lowercase, dir='source')
243 if not source.supports_content_filtering():
244 return
245 self.assertFileEqual("Foo Txt", 'source/file1.txt')
246 self.assert_basis_content("FOO TXT", source, txt_fileid)
247
248 # Now modify & rename a file, revert it and check the content
249 self.build_tree_contents([
250 ('source/file1.txt', 'Foo Txt with new content')])
251 source.rename_one('file1.txt', 'file1.bin')
252 self.assertTrue(os.path.exists('source/file1.bin'))
253 self.assertFalse(os.path.exists('source/file1.txt'))
254 self.assertFileEqual("Foo Txt with new content", 'source/file1.bin')
255 source.revert(['file1.bin'])
256 self.assertFalse(os.path.exists('source/file1.bin'))
257 self.assertTrue(os.path.exists('source/file1.txt'))
258 # Note: we don't get back exactly what was in the tree
259 # previously because lower(upper(text)) is a lossy transformation
260 self.assertFileEqual("foo txt", 'source/file1.txt')
212261
=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py 2009-10-14 18:27:23 +0000
+++ bzrlib/transform.py 2009-10-27 12:15:24 +0000
@@ -2628,9 +2628,22 @@
2628 tt.adjust_path(name[1], parent_trans, trans_id)2628 tt.adjust_path(name[1], parent_trans, trans_id)
2629 if executable[0] != executable[1] and kind[1] == "file":2629 if executable[0] != executable[1] and kind[1] == "file":
2630 tt.set_executability(executable[1], trans_id)2630 tt.set_executability(executable[1], trans_id)
2631 for (trans_id, mode_id), bytes in target_tree.iter_files_bytes(2631 if working_tree.supports_content_filtering():
2632 deferred_files):2632 for index, ((trans_id, mode_id), bytes) in enumerate(
2633 tt.create_file(bytes, trans_id, mode_id)2633 target_tree.iter_files_bytes(deferred_files)):
2634 file_id = deferred_files[index][0]
2635 # We're reverting a tree to the target tree so using the
2636 # target tree to find the file path seems the best choice
2637 # here IMO - Ian C 27/Oct/2009
2638 filter_tree_path = target_tree.id2path(file_id)
2639 filters = working_tree._content_filter_stack(filter_tree_path)
2640 bytes = filtered_output_bytes(bytes, filters,
2641 ContentFilterContext(filter_tree_path, working_tree))
2642 tt.create_file(bytes, trans_id, mode_id)
2643 else:
2644 for (trans_id, mode_id), bytes in target_tree.iter_files_bytes(
2645 deferred_files):
2646 tt.create_file(bytes, trans_id, mode_id)
2634 finally:2647 finally:
2635 if basis_tree is not None:2648 if basis_tree is not None:
2636 basis_tree.unlock()2649 basis_tree.unlock()

Subscribers

People subscribed via source and target branches