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
1=== modified file 'NEWS'
2--- NEWS 2009-10-21 01:05:40 +0000
3+++ NEWS 2009-10-27 12:15:24 +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 revert.
9+ (Ian Clatworthy)
10+
11 * Diff parsing handles "Binary files differ" hunks. (Aaron Bentley, #436325)
12
13 * Fetching from stacked pre-2a repository via a smart server no longer
14
15=== modified file 'bzrlib/tests/per_workingtree/test_content_filters.py'
16--- bzrlib/tests/per_workingtree/test_content_filters.py 2009-08-25 04:43:21 +0000
17+++ bzrlib/tests/per_workingtree/test_content_filters.py 2009-10-27 12:15:24 +0000
18@@ -16,6 +16,8 @@
19
20 """Tests for content filtering conformance"""
21
22+import os
23+
24 from bzrlib.filters import ContentFilter
25 from bzrlib.workingtree import WorkingTree
26 from bzrlib.tests.per_workingtree import TestCaseWithWorkingTree
27@@ -81,6 +83,29 @@
28 bin_fileid = tree.path2id('file2.bin')
29 return tree, txt_fileid, bin_fileid
30
31+ def patch_in_content_filter(self):
32+ # Patch in a custom, symmetric content filter stack
33+ self.real_content_filter_stack = WorkingTree._content_filter_stack
34+ def restore_real_content_filter_stack():
35+ WorkingTree._content_filter_stack = self.real_content_filter_stack
36+ self.addCleanup(restore_real_content_filter_stack)
37+ def _content_filter_stack(tree, path=None, file_id=None):
38+ if path.endswith('.txt'):
39+ return [ContentFilter(_swapcase, _swapcase)]
40+ else:
41+ return []
42+ WorkingTree._content_filter_stack = _content_filter_stack
43+
44+ def assert_basis_content(self, expected_content, branch, file_id):
45+ # Note: We need to use try/finally here instead of addCleanup()
46+ # as the latter leaves the read lock in place too long
47+ basis = branch.basis_tree()
48+ basis.lock_read()
49+ try:
50+ self.assertEqual(expected_content, basis.get_file_text(file_id))
51+ finally:
52+ basis.unlock()
53+
54 def test_symmetric_content_filtering(self):
55 # test handling when read then write gives back the initial content
56 tree, txt_fileid, bin_fileid = self.create_cf_tree(
57@@ -132,10 +157,7 @@
58 if not source.supports_content_filtering():
59 return
60 self.assertFileEqual("Foo Txt", 'source/file1.txt')
61- basis = source.basis_tree()
62- basis.lock_read()
63- self.addCleanup(basis.unlock)
64- self.assertEqual("FOO TXT", basis.get_file_text(txt_fileid))
65+ self.assert_basis_content("FOO TXT", source, txt_fileid)
66
67 # Now branch it
68 self.run_bzr('branch source target')
69@@ -153,24 +175,10 @@
70 if not source.supports_content_filtering():
71 return
72 self.assertFileEqual("Foo Txt", 'source/file1.txt')
73- basis = source.basis_tree()
74- basis.lock_read()
75- self.addCleanup(basis.unlock)
76- self.assertEqual("Foo Txt", basis.get_file_text(txt_fileid))
77-
78- # Patch in a custom, symmetric content filter stack
79- self.real_content_filter_stack = WorkingTree._content_filter_stack
80- def restore_real_content_filter_stack():
81- WorkingTree._content_filter_stack = self.real_content_filter_stack
82- self.addCleanup(restore_real_content_filter_stack)
83- def _content_filter_stack(tree, path=None, file_id=None):
84- if path.endswith('.txt'):
85- return [ContentFilter(_swapcase, _swapcase)]
86- else:
87- return []
88- WorkingTree._content_filter_stack = _content_filter_stack
89-
90- # Now branch it
91+ self.assert_basis_content("Foo Txt", source, txt_fileid)
92+
93+ # Now patch in content filtering and branch the source
94+ self.patch_in_content_filter()
95 self.run_bzr('branch source target')
96 target = WorkingTree.open('target')
97 # Even though the content in source and target are different
98@@ -209,3 +217,44 @@
99 # we could give back the length of the canonical form, but in general
100 # that will be expensive to compute, so it's acceptable to just return
101 # None.
102+
103+ def test_content_filtering_applied_on_revert_delete(self):
104+ # Create a source branch with content filtering
105+ source, txt_fileid, bin_fileid = self.create_cf_tree(
106+ txt_reader=_uppercase, txt_writer=_lowercase, dir='source')
107+ if not source.supports_content_filtering():
108+ return
109+ self.assertFileEqual("Foo Txt", 'source/file1.txt')
110+ self.assert_basis_content("FOO TXT", source, txt_fileid)
111+
112+ # Now delete the file, revert it and check the content
113+ os.unlink('source/file1.txt')
114+ self.assertFalse(os.path.exists('source/file1.txt'))
115+ source.revert(['file1.txt'])
116+ self.assertTrue(os.path.exists('source/file1.txt'))
117+ # Note: we don't get back exactly what was in the tree
118+ # previously because lower(upper(text)) is a lossy transformation
119+ self.assertFileEqual("foo txt", 'source/file1.txt')
120+
121+ def test_content_filtering_applied_on_revert_rename(self):
122+ # Create a source branch with content filtering
123+ source, txt_fileid, bin_fileid = self.create_cf_tree(
124+ txt_reader=_uppercase, txt_writer=_lowercase, dir='source')
125+ if not source.supports_content_filtering():
126+ return
127+ self.assertFileEqual("Foo Txt", 'source/file1.txt')
128+ self.assert_basis_content("FOO TXT", source, txt_fileid)
129+
130+ # Now modify & rename a file, revert it and check the content
131+ self.build_tree_contents([
132+ ('source/file1.txt', 'Foo Txt with new content')])
133+ source.rename_one('file1.txt', 'file1.bin')
134+ self.assertTrue(os.path.exists('source/file1.bin'))
135+ self.assertFalse(os.path.exists('source/file1.txt'))
136+ self.assertFileEqual("Foo Txt with new content", 'source/file1.bin')
137+ source.revert(['file1.bin'])
138+ self.assertFalse(os.path.exists('source/file1.bin'))
139+ self.assertTrue(os.path.exists('source/file1.txt'))
140+ # Note: we don't get back exactly what was in the tree
141+ # previously because lower(upper(text)) is a lossy transformation
142+ self.assertFileEqual("foo txt", 'source/file1.txt')
143
144=== modified file 'bzrlib/transform.py'
145--- bzrlib/transform.py 2009-10-14 18:27:23 +0000
146+++ bzrlib/transform.py 2009-10-27 12:15:24 +0000
147@@ -2628,9 +2628,22 @@
148 tt.adjust_path(name[1], parent_trans, trans_id)
149 if executable[0] != executable[1] and kind[1] == "file":
150 tt.set_executability(executable[1], trans_id)
151- for (trans_id, mode_id), bytes in target_tree.iter_files_bytes(
152- deferred_files):
153- tt.create_file(bytes, trans_id, mode_id)
154+ if working_tree.supports_content_filtering():
155+ for index, ((trans_id, mode_id), bytes) in enumerate(
156+ target_tree.iter_files_bytes(deferred_files)):
157+ file_id = deferred_files[index][0]
158+ # We're reverting a tree to the target tree so using the
159+ # target tree to find the file path seems the best choice
160+ # here IMO - Ian C 27/Oct/2009
161+ filter_tree_path = target_tree.id2path(file_id)
162+ filters = working_tree._content_filter_stack(filter_tree_path)
163+ bytes = filtered_output_bytes(bytes, filters,
164+ ContentFilterContext(filter_tree_path, working_tree))
165+ tt.create_file(bytes, trans_id, mode_id)
166+ else:
167+ for (trans_id, mode_id), bytes in target_tree.iter_files_bytes(
168+ deferred_files):
169+ tt.create_file(bytes, trans_id, mode_id)
170 finally:
171 if basis_tree is not None:
172 basis_tree.unlock()

Subscribers

People subscribed via source and target branches