Merge lp:~ian-clatworthy/bzr/eol-st-ci-fix into lp:~bzr/bzr/trunk-old
- eol-st-ci-fix
- Merge into trunk-old
Proposed by
Ian Clatworthy
| Status: | Merged |
|---|---|
| Merged at revision: | not available |
| Proposed branch: | lp:~ian-clatworthy/bzr/eol-st-ci-fix |
| Merge into: | lp:~bzr/bzr/trunk-old |
| Diff against target: | 555 lines |
| To merge this branch: | bzr merge lp:~ian-clatworthy/bzr/eol-st-ci-fix |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| John A Meinel | Approve | ||
|
Review via email:
|
|||
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
| Ian Clatworthy (ian-clatworthy) wrote : | # |
Revision history for this message
| John A Meinel (jameinel) wrote : | # |
Download full text (4.9 KiB)
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Ian Clatworthy wrote:
> Ian Clatworthy has proposed merging lp:~ian-clatworthy/bzr/eol-st-ci-fix into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This patch fixes some very bad bugs in content filtering. There are actually 2 separate issues fixed:
>
> 1. Branching from a non-content-
>
> 2. Status detection was relying on size matching to determine equivalence when that assumption doesn't hold in the presence of content filtering.
>
> Both code changes are quite straightforward, though the latter one requires fixes in both Python and Pyrex code. Testing all of this is far more complex though. I've added a heap more units tests and I'm comfortable that status on content filtered trees now has pretty good coverage.
>
> I'm yet to add detailed tests for the branch/commit issue, because the monkey-patching approach used to test status isn't at the right level to work for branch. I have tested it manually though and one of the bug reporters (Frits) has confirmed that the code fixes works correctly for him. Given the related code change - switching off an optional optimisation - is very straight forward, I think this patch can land as is for 1.16rc1.
>
> FWIW, my plan is to add those additional tests soon. (Some pending changes to the monkey-patching used here to fix some other bugs will make that easier, but they aren't ready to go, and this really needs to be landed for content filtering to be unbroken for users.)
>
+ # Check the sha. We can't just rely on the size as
+ # content filtering may mean differ sizes actually
+ # map to the same content
+ if link_or_sha1 is None:
^- It would be nice if this was only done when there were actually
filters in play, rather than disabling the size check universally.
This change will cause us to sha1 the content of everything we are
committing 2 times. Once during iter_changes to make sure it really has
changed, and then once during the insert-
its final sha, and ensure that it really has changed.
Note that ideally we would have an iter_changes that could provide
"maybe changed" and *never* sha content. Perhaps we can address that
when we re-visit dirstate. (So I'm saying it is probably ok for now, but
it is not the way we want to leave the code in the long run.)
...
# Note: do NOT move this logic up higher - using the
basis from
# the accelerator tree is still desirable because that
can save
# a minute or more of processing on large trees!
+ # The original tree may not have the same content filters
+ # applied so we can't safely build the inventory delta from
+ # the source tree.
if wt.supports_
+ delta_from_tree = False
+ else:
+ delta...
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-06-11 00:47:41 +0000 | |||
| 3 | +++ NEWS 2009-06-11 04:35:14 +0000 | |||
| 4 | @@ -81,6 +81,9 @@ | |||
| 5 | 81 | the fetched revisions, not in the stacked-on ancestry. | 81 | the fetched revisions, not in the stacked-on ancestry. |
| 6 | 82 | (John Arbash Meinel) | 82 | (John Arbash Meinel) |
| 7 | 83 | 83 | ||
| 8 | 84 | * Fix status and commit to work with content filtered trees, addressing | ||
| 9 | 85 | numerous bad bugs with line-ending support. (Ian Clatworthy, #362030) | ||
| 10 | 86 | |||
| 11 | 84 | * Fix problem of "directory not empty" when contending for a lock over | 87 | * Fix problem of "directory not empty" when contending for a lock over |
| 12 | 85 | sftp. (Martin Pool, #340352) | 88 | sftp. (Martin Pool, #340352) |
| 13 | 86 | 89 | ||
| 14 | 87 | 90 | ||
| 15 | === modified file 'bzrlib/_dirstate_helpers_c.pyx' | |||
| 16 | --- bzrlib/_dirstate_helpers_c.pyx 2009-03-23 14:59:43 +0000 | |||
| 17 | +++ bzrlib/_dirstate_helpers_c.pyx 2009-06-11 04:35:14 +0000 | |||
| 18 | @@ -1140,19 +1140,17 @@ | |||
| 19 | 1140 | if source_minikind != c'f': | 1140 | if source_minikind != c'f': |
| 20 | 1141 | content_change = 1 | 1141 | content_change = 1 |
| 21 | 1142 | else: | 1142 | else: |
| 35 | 1143 | # If the size is the same, check the sha: | 1143 | # Check the sha. We can't just rely on the size as |
| 36 | 1144 | if target_details[2] == source_details[2]: | 1144 | # content filtering may mean differ sizes actually |
| 37 | 1145 | if link_or_sha1 is None: | 1145 | # map to the same content |
| 38 | 1146 | # Stat cache miss: | 1146 | if link_or_sha1 is None: |
| 39 | 1147 | statvalue, link_or_sha1 = \ | 1147 | # Stat cache miss: |
| 40 | 1148 | self.state._sha1_provider.stat_and_sha1( | 1148 | statvalue, link_or_sha1 = \ |
| 41 | 1149 | path_info[4]) | 1149 | self.state._sha1_provider.stat_and_sha1( |
| 42 | 1150 | self.state._observed_sha1(entry, link_or_sha1, | 1150 | path_info[4]) |
| 43 | 1151 | statvalue) | 1151 | self.state._observed_sha1(entry, link_or_sha1, |
| 44 | 1152 | content_change = (link_or_sha1 != source_details[1]) | 1152 | statvalue) |
| 45 | 1153 | else: | 1153 | content_change = (link_or_sha1 != source_details[1]) |
| 33 | 1154 | # Size changed, so must be different | ||
| 34 | 1155 | content_change = 1 | ||
| 46 | 1156 | # Target details is updated at update_entry time | 1154 | # Target details is updated at update_entry time |
| 47 | 1157 | if self.use_filesystem_for_exec: | 1155 | if self.use_filesystem_for_exec: |
| 48 | 1158 | # We don't need S_ISREG here, because we are sure | 1156 | # We don't need S_ISREG here, because we are sure |
| 49 | 1159 | 1157 | ||
| 50 | === modified file 'bzrlib/builtins.py' | |||
| 51 | --- bzrlib/builtins.py 2009-06-10 03:56:49 +0000 | |||
| 52 | +++ bzrlib/builtins.py 2009-06-11 04:35:14 +0000 | |||
| 53 | @@ -1112,6 +1112,9 @@ | |||
| 54 | 1112 | 1112 | ||
| 55 | 1113 | accelerator_tree, br_from = bzrdir.BzrDir.open_tree_or_branch( | 1113 | accelerator_tree, br_from = bzrdir.BzrDir.open_tree_or_branch( |
| 56 | 1114 | from_location) | 1114 | from_location) |
| 57 | 1115 | if (accelerator_tree is not None and | ||
| 58 | 1116 | accelerator_tree.supports_content_filtering()): | ||
| 59 | 1117 | accelerator_tree = None | ||
| 60 | 1115 | revision = _get_one_revision('branch', revision) | 1118 | revision = _get_one_revision('branch', revision) |
| 61 | 1116 | br_from.lock_read() | 1119 | br_from.lock_read() |
| 62 | 1117 | try: | 1120 | try: |
| 63 | 1118 | 1121 | ||
| 64 | === modified file 'bzrlib/dirstate.py' | |||
| 65 | --- bzrlib/dirstate.py 2009-05-06 05:36:28 +0000 | |||
| 66 | +++ bzrlib/dirstate.py 2009-06-11 04:35:14 +0000 | |||
| 67 | @@ -3062,19 +3062,17 @@ | |||
| 68 | 3062 | if source_minikind != 'f': | 3062 | if source_minikind != 'f': |
| 69 | 3063 | content_change = True | 3063 | content_change = True |
| 70 | 3064 | else: | 3064 | else: |
| 84 | 3065 | # If the size is the same, check the sha: | 3065 | # Check the sha. We can't just rely on the size as |
| 85 | 3066 | if target_details[2] == source_details[2]: | 3066 | # content filtering may mean differ sizes actually |
| 86 | 3067 | if link_or_sha1 is None: | 3067 | # map to the same content |
| 87 | 3068 | # Stat cache miss: | 3068 | if link_or_sha1 is None: |
| 88 | 3069 | statvalue, link_or_sha1 = \ | 3069 | # Stat cache miss: |
| 89 | 3070 | self.state._sha1_provider.stat_and_sha1( | 3070 | statvalue, link_or_sha1 = \ |
| 90 | 3071 | path_info[4]) | 3071 | self.state._sha1_provider.stat_and_sha1( |
| 91 | 3072 | self.state._observed_sha1(entry, link_or_sha1, | 3072 | path_info[4]) |
| 92 | 3073 | statvalue) | 3073 | self.state._observed_sha1(entry, link_or_sha1, |
| 93 | 3074 | content_change = (link_or_sha1 != source_details[1]) | 3074 | statvalue) |
| 94 | 3075 | else: | 3075 | content_change = (link_or_sha1 != source_details[1]) |
| 82 | 3076 | # Size changed, so must be different | ||
| 83 | 3077 | content_change = True | ||
| 95 | 3078 | # Target details is updated at update_entry time | 3076 | # Target details is updated at update_entry time |
| 96 | 3079 | if self.use_filesystem_for_exec: | 3077 | if self.use_filesystem_for_exec: |
| 97 | 3080 | # We don't need S_ISREG here, because we are sure | 3078 | # We don't need S_ISREG here, because we are sure |
| 98 | 3081 | 3079 | ||
| 99 | === modified file 'bzrlib/tests/workingtree_implementations/test_content_filters.py' | |||
| 100 | --- bzrlib/tests/workingtree_implementations/test_content_filters.py 2009-03-23 14:59:43 +0000 | |||
| 101 | +++ bzrlib/tests/workingtree_implementations/test_content_filters.py 2009-06-11 04:35:14 +0000 | |||
| 102 | @@ -17,6 +17,7 @@ | |||
| 103 | 17 | """Tests for content filtering conformance""" | 17 | """Tests for content filtering conformance""" |
| 104 | 18 | 18 | ||
| 105 | 19 | from bzrlib.filters import ContentFilter | 19 | from bzrlib.filters import ContentFilter |
| 106 | 20 | from bzrlib.workingtree import WorkingTree | ||
| 107 | 20 | from bzrlib.tests.workingtree_implementations import TestCaseWithWorkingTree | 21 | from bzrlib.tests.workingtree_implementations import TestCaseWithWorkingTree |
| 108 | 21 | 22 | ||
| 109 | 22 | 23 | ||
| 110 | @@ -44,8 +45,8 @@ | |||
| 111 | 44 | 45 | ||
| 112 | 45 | class TestWorkingTreeWithContentFilters(TestCaseWithWorkingTree): | 46 | class TestWorkingTreeWithContentFilters(TestCaseWithWorkingTree): |
| 113 | 46 | 47 | ||
| 116 | 47 | def create_cf_tree(self, txt_reader, txt_writer): | 48 | def create_cf_tree(self, txt_reader, txt_writer, dir='.'): |
| 117 | 48 | tree = self.make_branch_and_tree('.') | 49 | tree = self.make_branch_and_tree(dir) |
| 118 | 49 | def _content_filter_stack(path=None, file_id=None): | 50 | def _content_filter_stack(path=None, file_id=None): |
| 119 | 50 | if path.endswith('.txt'): | 51 | if path.endswith('.txt'): |
| 120 | 51 | return [ContentFilter(txt_reader, txt_writer)] | 52 | return [ContentFilter(txt_reader, txt_writer)] |
| 121 | @@ -53,8 +54,8 @@ | |||
| 122 | 53 | return [] | 54 | return [] |
| 123 | 54 | tree._content_filter_stack = _content_filter_stack | 55 | tree._content_filter_stack = _content_filter_stack |
| 124 | 55 | self.build_tree_contents([ | 56 | self.build_tree_contents([ |
| 127 | 56 | ('file1.txt', 'Foo Txt'), | 57 | (dir + '/file1.txt', 'Foo Txt'), |
| 128 | 57 | ('file2.bin', 'Foo Bin')]) | 58 | (dir + '/file2.bin', 'Foo Bin')]) |
| 129 | 58 | tree.add(['file1.txt', 'file2.bin']) | 59 | tree.add(['file1.txt', 'file2.bin']) |
| 130 | 59 | tree.commit('commit raw content') | 60 | tree.commit('commit raw content') |
| 131 | 60 | txt_fileid = tree.path2id('file1.txt') | 61 | txt_fileid = tree.path2id('file1.txt') |
| 132 | @@ -104,3 +105,57 @@ | |||
| 133 | 104 | filtered=False).read()) | 105 | filtered=False).read()) |
| 134 | 105 | self.assertEqual('Foo Bin', tree.get_file(bin_fileid, | 106 | self.assertEqual('Foo Bin', tree.get_file(bin_fileid, |
| 135 | 106 | filtered=False).read()) | 107 | filtered=False).read()) |
| 136 | 108 | |||
| 137 | 109 | def test_branch_source_filtered_target_not(self): | ||
| 138 | 110 | # Create a source branch with content filtering | ||
| 139 | 111 | source, txt_fileid, bin_fileid = self.create_cf_tree( | ||
| 140 | 112 | txt_reader=_uppercase, txt_writer=_lowercase, dir='source') | ||
| 141 | 113 | if not source.supports_content_filtering(): | ||
| 142 | 114 | return | ||
| 143 | 115 | self.assertFileEqual("Foo Txt", 'source/file1.txt') | ||
| 144 | 116 | basis = source.basis_tree() | ||
| 145 | 117 | basis.lock_read() | ||
| 146 | 118 | self.addCleanup(basis.unlock) | ||
| 147 | 119 | self.assertEqual("FOO TXT", basis.get_file_text(txt_fileid)) | ||
| 148 | 120 | |||
| 149 | 121 | # Now branch it | ||
| 150 | 122 | self.run_bzr('branch source target') | ||
| 151 | 123 | target = WorkingTree.open('target') | ||
| 152 | 124 | # Even though the content in source and target are different | ||
| 153 | 125 | # due to different filters, iter_changes should be clean | ||
| 154 | 126 | self.assertFileEqual("FOO TXT", 'target/file1.txt') | ||
| 155 | 127 | changes = target.changes_from(source.basis_tree()) | ||
| 156 | 128 | self.assertFalse(changes.has_changed()) | ||
| 157 | 129 | |||
| 158 | 130 | def test_branch_source_not_filtered_target_is(self): | ||
| 159 | 131 | # Create a source branch with content filtering | ||
| 160 | 132 | source, txt_fileid, bin_fileid = self.create_cf_tree( | ||
| 161 | 133 | txt_reader=None, txt_writer=None, dir='source') | ||
| 162 | 134 | if not source.supports_content_filtering(): | ||
| 163 | 135 | return | ||
| 164 | 136 | self.assertFileEqual("Foo Txt", 'source/file1.txt') | ||
| 165 | 137 | basis = source.basis_tree() | ||
| 166 | 138 | basis.lock_read() | ||
| 167 | 139 | self.addCleanup(basis.unlock) | ||
| 168 | 140 | self.assertEqual("Foo Txt", basis.get_file_text(txt_fileid)) | ||
| 169 | 141 | |||
| 170 | 142 | # Patch in a custom, symmetric content filter stack | ||
| 171 | 143 | self.real_content_filter_stack = WorkingTree._content_filter_stack | ||
| 172 | 144 | def restore_real_content_filter_stack(): | ||
| 173 | 145 | WorkingTree._content_filter_stack = self.real_content_filter_stack | ||
| 174 | 146 | self.addCleanup(restore_real_content_filter_stack) | ||
| 175 | 147 | def _content_filter_stack(tree, path=None, file_id=None): | ||
| 176 | 148 | if path.endswith('.txt'): | ||
| 177 | 149 | return [ContentFilter(_swapcase, _swapcase)] | ||
| 178 | 150 | else: | ||
| 179 | 151 | return [] | ||
| 180 | 152 | WorkingTree._content_filter_stack = _content_filter_stack | ||
| 181 | 153 | |||
| 182 | 154 | # Now branch it | ||
| 183 | 155 | self.run_bzr('branch source target') | ||
| 184 | 156 | target = WorkingTree.open('target') | ||
| 185 | 157 | # Even though the content in source and target are different | ||
| 186 | 158 | # due to different filters, iter_changes should be clean | ||
| 187 | 159 | self.assertFileEqual("fOO tXT", 'target/file1.txt') | ||
| 188 | 160 | changes = target.changes_from(source.basis_tree()) | ||
| 189 | 161 | self.assertFalse(changes.has_changed()) | ||
| 190 | 107 | 162 | ||
| 191 | === modified file 'bzrlib/tests/workingtree_implementations/test_eol_conversion.py' | |||
| 192 | --- bzrlib/tests/workingtree_implementations/test_eol_conversion.py 2009-04-02 04:12:11 +0000 | |||
| 193 | +++ bzrlib/tests/workingtree_implementations/test_eol_conversion.py 2009-06-11 04:35:14 +0000 | |||
| 194 | @@ -17,8 +17,9 @@ | |||
| 195 | 17 | """Tests for eol conversion.""" | 17 | """Tests for eol conversion.""" |
| 196 | 18 | 18 | ||
| 197 | 19 | import sys | 19 | import sys |
| 198 | 20 | from cStringIO import StringIO | ||
| 199 | 20 | 21 | ||
| 201 | 21 | from bzrlib import rules | 22 | from bzrlib import rules, status |
| 202 | 22 | from bzrlib.tests import TestSkipped | 23 | from bzrlib.tests import TestSkipped |
| 203 | 23 | from bzrlib.tests.workingtree_implementations import TestCaseWithWorkingTree | 24 | from bzrlib.tests.workingtree_implementations import TestCaseWithWorkingTree |
| 204 | 24 | from bzrlib.workingtree import WorkingTree | 25 | from bzrlib.workingtree import WorkingTree |
| 205 | @@ -29,6 +30,13 @@ | |||
| 206 | 29 | _sample_text_on_win = """hello\r\nworld\r\n""" | 30 | _sample_text_on_win = """hello\r\nworld\r\n""" |
| 207 | 30 | _sample_text_on_unix = """hello\nworld\n""" | 31 | _sample_text_on_unix = """hello\nworld\n""" |
| 208 | 31 | _sample_binary = """hello\nworld\r\n\x00""" | 32 | _sample_binary = """hello\nworld\r\n\x00""" |
| 209 | 33 | _sample_clean_lf = _sample_text_on_unix | ||
| 210 | 34 | _sample_clean_crlf = _sample_text_on_win | ||
| 211 | 35 | |||
| 212 | 36 | |||
| 213 | 37 | # Lists of formats for each storage policy | ||
| 214 | 38 | _LF_IN_REPO = ['native', 'lf', 'crlf'] | ||
| 215 | 39 | _CRLF_IN_REPO = [ '%s-with-crlf-in-repo' % (f,) for f in _LF_IN_REPO] | ||
| 216 | 32 | 40 | ||
| 217 | 33 | 41 | ||
| 218 | 34 | class TestEolConversion(TestCaseWithWorkingTree): | 42 | class TestEolConversion(TestCaseWithWorkingTree): |
| 219 | @@ -74,8 +82,11 @@ | |||
| 220 | 74 | return t, basis | 82 | return t, basis |
| 221 | 75 | 83 | ||
| 222 | 76 | def assertNewContentForSetting(self, wt, eol, expected_unix, | 84 | def assertNewContentForSetting(self, wt, eol, expected_unix, |
| 225 | 77 | expected_win=None): | 85 | expected_win, roundtrip): |
| 226 | 78 | """Clone a working tree and check the convenience content.""" | 86 | """Clone a working tree and check the convenience content. |
| 227 | 87 | |||
| 228 | 88 | If roundtrip is True, status and commit should see no changes. | ||
| 229 | 89 | """ | ||
| 230 | 79 | if expected_win is None: | 90 | if expected_win is None: |
| 231 | 80 | expected_win = expected_unix | 91 | expected_win = expected_unix |
| 232 | 81 | self.patch_rules_searcher(eol) | 92 | self.patch_rules_searcher(eol) |
| 233 | @@ -86,100 +97,241 @@ | |||
| 234 | 86 | self.assertEqual(expected_win, content) | 97 | self.assertEqual(expected_win, content) |
| 235 | 87 | else: | 98 | else: |
| 236 | 88 | self.assertEqual(expected_unix, content) | 99 | self.assertEqual(expected_unix, content) |
| 237 | 100 | # Confirm that status thinks nothing has changed if the text roundtrips | ||
| 238 | 101 | if roundtrip: | ||
| 239 | 102 | status_io = StringIO() | ||
| 240 | 103 | status.show_tree_status(wt2, to_file=status_io) | ||
| 241 | 104 | self.assertEqual('', status_io.getvalue()) | ||
| 242 | 89 | 105 | ||
| 243 | 90 | def assertContent(self, wt, basis, expected_raw, expected_unix, | 106 | def assertContent(self, wt, basis, expected_raw, expected_unix, |
| 246 | 91 | expected_win): | 107 | expected_win, roundtrip_to=None): |
| 247 | 92 | """Check the committed content and content in cloned trees.""" | 108 | """Check the committed content and content in cloned trees. |
| 248 | 109 | |||
| 249 | 110 | :param roundtrip_to: the set of formats (excluding exact) we | ||
| 250 | 111 | can round-trip to or None for all | ||
| 251 | 112 | """ | ||
| 252 | 93 | basis_content = basis.get_file('file1-id').read() | 113 | basis_content = basis.get_file('file1-id').read() |
| 253 | 94 | self.assertEqual(expected_raw, basis_content) | 114 | self.assertEqual(expected_raw, basis_content) |
| 255 | 95 | self.assertNewContentForSetting(wt, None, expected_raw) | 115 | |
| 256 | 116 | # No setting and exact should always roundtrip | ||
| 257 | 117 | self.assertNewContentForSetting(wt, None, | ||
| 258 | 118 | expected_raw, expected_raw, roundtrip=True) | ||
| 259 | 119 | self.assertNewContentForSetting(wt, 'exact', | ||
| 260 | 120 | expected_raw, expected_raw, roundtrip=True) | ||
| 261 | 121 | |||
| 262 | 122 | # Roundtripping is otherwise dependent on whether the original | ||
| 263 | 123 | # text is clean - mixed line endings will prevent it. It also | ||
| 264 | 124 | # depends on whether the format in the repository is being changed. | ||
| 265 | 125 | if roundtrip_to is None: | ||
| 266 | 126 | roundtrip_to = _LF_IN_REPO + _CRLF_IN_REPO | ||
| 267 | 96 | self.assertNewContentForSetting(wt, 'native', | 127 | self.assertNewContentForSetting(wt, 'native', |
| 269 | 97 | expected_unix, expected_win) | 128 | expected_unix, expected_win, 'native' in roundtrip_to) |
| 270 | 98 | self.assertNewContentForSetting(wt, 'lf', | 129 | self.assertNewContentForSetting(wt, 'lf', |
| 272 | 99 | expected_unix, expected_unix) | 130 | expected_unix, expected_unix, 'lf' in roundtrip_to) |
| 273 | 100 | self.assertNewContentForSetting(wt, 'crlf', | 131 | self.assertNewContentForSetting(wt, 'crlf', |
| 275 | 101 | expected_win, expected_win) | 132 | expected_win, expected_win, 'crlf' in roundtrip_to) |
| 276 | 102 | self.assertNewContentForSetting(wt, 'native-with-crlf-in-repo', | 133 | self.assertNewContentForSetting(wt, 'native-with-crlf-in-repo', |
| 278 | 103 | expected_unix, expected_win) | 134 | expected_unix, expected_win, |
| 279 | 135 | 'native-with-crlf-in-repo' in roundtrip_to) | ||
| 280 | 104 | self.assertNewContentForSetting(wt, 'lf-with-crlf-in-repo', | 136 | self.assertNewContentForSetting(wt, 'lf-with-crlf-in-repo', |
| 282 | 105 | expected_unix, expected_unix) | 137 | expected_unix, expected_unix, |
| 283 | 138 | 'lf-with-crlf-in-repo' in roundtrip_to) | ||
| 284 | 106 | self.assertNewContentForSetting(wt, 'crlf-with-crlf-in-repo', | 139 | self.assertNewContentForSetting(wt, 'crlf-with-crlf-in-repo', |
| 297 | 107 | expected_win, expected_win) | 140 | expected_win, expected_win, |
| 298 | 108 | self.assertNewContentForSetting(wt, 'exact', expected_raw) | 141 | 'crlf-with-crlf-in-repo' in roundtrip_to) |
| 299 | 109 | 142 | ||
| 300 | 110 | def test_eol_no_rules(self): | 143 | # Test binary files. These always roundtrip. |
| 301 | 111 | wt, basis = self.prepare_tree(_sample_text) | 144 | |
| 302 | 112 | self.assertContent(wt, basis, _sample_text, | 145 | def test_eol_no_rules_binary(self): |
| 303 | 113 | _sample_text_on_unix, _sample_text_on_win) | 146 | wt, basis = self.prepare_tree(_sample_binary) |
| 304 | 114 | 147 | self.assertContent(wt, basis, _sample_binary, _sample_binary, | |
| 305 | 115 | def test_eol_native(self): | 148 | _sample_binary) |
| 306 | 116 | wt, basis = self.prepare_tree(_sample_text, eol='native') | 149 | |
| 307 | 117 | self.assertContent(wt, basis, _sample_text_on_unix, | 150 | def test_eol_exact_binary(self): |
| 308 | 118 | _sample_text_on_unix, _sample_text_on_win) | 151 | wt, basis = self.prepare_tree(_sample_binary, eol='exact') |
| 309 | 152 | self.assertContent(wt, basis, _sample_binary, _sample_binary, | ||
| 310 | 153 | _sample_binary) | ||
| 311 | 119 | 154 | ||
| 312 | 120 | def test_eol_native_binary(self): | 155 | def test_eol_native_binary(self): |
| 313 | 121 | wt, basis = self.prepare_tree(_sample_binary, eol='native') | 156 | wt, basis = self.prepare_tree(_sample_binary, eol='native') |
| 314 | 122 | self.assertContent(wt, basis, _sample_binary, _sample_binary, | 157 | self.assertContent(wt, basis, _sample_binary, _sample_binary, |
| 315 | 123 | _sample_binary) | 158 | _sample_binary) |
| 316 | 124 | 159 | ||
| 317 | 125 | def test_eol_lf(self): | ||
| 318 | 126 | wt, basis = self.prepare_tree(_sample_text, eol='lf') | ||
| 319 | 127 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 320 | 128 | _sample_text_on_unix, _sample_text_on_win) | ||
| 321 | 129 | |||
| 322 | 130 | def test_eol_lf_binary(self): | 160 | def test_eol_lf_binary(self): |
| 323 | 131 | wt, basis = self.prepare_tree(_sample_binary, eol='lf') | 161 | wt, basis = self.prepare_tree(_sample_binary, eol='lf') |
| 324 | 132 | self.assertContent(wt, basis, _sample_binary, _sample_binary, | 162 | self.assertContent(wt, basis, _sample_binary, _sample_binary, |
| 325 | 133 | _sample_binary) | 163 | _sample_binary) |
| 326 | 134 | 164 | ||
| 327 | 135 | def test_eol_crlf(self): | ||
| 328 | 136 | wt, basis = self.prepare_tree(_sample_text, eol='crlf') | ||
| 329 | 137 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 330 | 138 | _sample_text_on_unix, _sample_text_on_win) | ||
| 331 | 139 | |||
| 332 | 140 | def test_eol_crlf_binary(self): | 165 | def test_eol_crlf_binary(self): |
| 333 | 141 | wt, basis = self.prepare_tree(_sample_binary, eol='crlf') | 166 | wt, basis = self.prepare_tree(_sample_binary, eol='crlf') |
| 334 | 142 | self.assertContent(wt, basis, _sample_binary, _sample_binary, | 167 | self.assertContent(wt, basis, _sample_binary, _sample_binary, |
| 335 | 143 | _sample_binary) | 168 | _sample_binary) |
| 336 | 144 | 169 | ||
| 337 | 145 | def test_eol_native_with_crlf_in_repo(self): | ||
| 338 | 146 | wt, basis = self.prepare_tree(_sample_text, | ||
| 339 | 147 | eol='native-with-crlf-in-repo') | ||
| 340 | 148 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 341 | 149 | _sample_text_on_unix, _sample_text_on_win) | ||
| 342 | 150 | |||
| 343 | 151 | def test_eol_native_with_crlf_in_repo_binary(self): | 170 | def test_eol_native_with_crlf_in_repo_binary(self): |
| 344 | 152 | wt, basis = self.prepare_tree(_sample_binary, | 171 | wt, basis = self.prepare_tree(_sample_binary, |
| 345 | 153 | eol='native-with-crlf-in-repo') | 172 | eol='native-with-crlf-in-repo') |
| 346 | 154 | self.assertContent(wt, basis, _sample_binary, _sample_binary, | 173 | self.assertContent(wt, basis, _sample_binary, _sample_binary, |
| 347 | 155 | _sample_binary) | 174 | _sample_binary) |
| 348 | 156 | 175 | ||
| 349 | 157 | def test_eol_lf_with_crlf_in_repo(self): | ||
| 350 | 158 | wt, basis = self.prepare_tree(_sample_text, eol='lf-with-crlf-in-repo') | ||
| 351 | 159 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 352 | 160 | _sample_text_on_unix, _sample_text_on_win) | ||
| 353 | 161 | |||
| 354 | 162 | def test_eol_lf_with_crlf_in_repo_binary(self): | 176 | def test_eol_lf_with_crlf_in_repo_binary(self): |
| 356 | 163 | wt, basis = self.prepare_tree(_sample_binary, eol='lf-with-crlf-in-repo') | 177 | wt, basis = self.prepare_tree(_sample_binary, |
| 357 | 178 | eol='lf-with-crlf-in-repo') | ||
| 358 | 164 | self.assertContent(wt, basis, _sample_binary, _sample_binary, | 179 | self.assertContent(wt, basis, _sample_binary, _sample_binary, |
| 359 | 165 | _sample_binary) | 180 | _sample_binary) |
| 360 | 166 | 181 | ||
| 361 | 167 | def test_eol_crlf_with_crlf_in_repo(self): | ||
| 362 | 168 | wt, basis = self.prepare_tree(_sample_text, eol='crlf-with-crlf-in-repo') | ||
| 363 | 169 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 364 | 170 | _sample_text_on_unix, _sample_text_on_win) | ||
| 365 | 171 | |||
| 366 | 172 | def test_eol_crlf_with_crlf_in_repo_binary(self): | 182 | def test_eol_crlf_with_crlf_in_repo_binary(self): |
| 368 | 173 | wt, basis = self.prepare_tree(_sample_binary, eol='crlf-with-crlf-in-repo') | 183 | wt, basis = self.prepare_tree(_sample_binary, |
| 369 | 184 | eol='crlf-with-crlf-in-repo') | ||
| 370 | 174 | self.assertContent(wt, basis, _sample_binary, _sample_binary, | 185 | self.assertContent(wt, basis, _sample_binary, _sample_binary, |
| 371 | 175 | _sample_binary) | 186 | _sample_binary) |
| 372 | 176 | 187 | ||
| 374 | 177 | def test_eol_exact(self): | 188 | # Test text with mixed line endings ("dirty text"). |
| 375 | 189 | # This doesn't roundtrip so status always thinks something has changed. | ||
| 376 | 190 | |||
| 377 | 191 | def test_eol_no_rules_dirty(self): | ||
| 378 | 192 | wt, basis = self.prepare_tree(_sample_text) | ||
| 379 | 193 | self.assertContent(wt, basis, _sample_text, | ||
| 380 | 194 | _sample_text_on_unix, _sample_text_on_win, roundtrip_to=[]) | ||
| 381 | 195 | |||
| 382 | 196 | def test_eol_exact_dirty(self): | ||
| 383 | 178 | wt, basis = self.prepare_tree(_sample_text, eol='exact') | 197 | wt, basis = self.prepare_tree(_sample_text, eol='exact') |
| 384 | 179 | self.assertContent(wt, basis, _sample_text, | 198 | self.assertContent(wt, basis, _sample_text, |
| 391 | 180 | _sample_text_on_unix, _sample_text_on_win) | 199 | _sample_text_on_unix, _sample_text_on_win, roundtrip_to=[]) |
| 392 | 181 | 200 | ||
| 393 | 182 | def test_eol_exact_binary(self): | 201 | def test_eol_native_dirty(self): |
| 394 | 183 | wt, basis = self.prepare_tree(_sample_binary, eol='exact') | 202 | wt, basis = self.prepare_tree(_sample_text, eol='native') |
| 395 | 184 | self.assertContent(wt, basis, _sample_binary, _sample_binary, | 203 | self.assertContent(wt, basis, _sample_text_on_unix, |
| 396 | 185 | _sample_binary) | 204 | _sample_text_on_unix, _sample_text_on_win, roundtrip_to=[]) |
| 397 | 205 | |||
| 398 | 206 | def test_eol_lf_dirty(self): | ||
| 399 | 207 | wt, basis = self.prepare_tree(_sample_text, eol='lf') | ||
| 400 | 208 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 401 | 209 | _sample_text_on_unix, _sample_text_on_win, roundtrip_to=[]) | ||
| 402 | 210 | |||
| 403 | 211 | def test_eol_crlf_dirty(self): | ||
| 404 | 212 | wt, basis = self.prepare_tree(_sample_text, eol='crlf') | ||
| 405 | 213 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 406 | 214 | _sample_text_on_unix, _sample_text_on_win, roundtrip_to=[]) | ||
| 407 | 215 | |||
| 408 | 216 | def test_eol_native_with_crlf_in_repo_dirty(self): | ||
| 409 | 217 | wt, basis = self.prepare_tree(_sample_text, | ||
| 410 | 218 | eol='native-with-crlf-in-repo') | ||
| 411 | 219 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 412 | 220 | _sample_text_on_unix, _sample_text_on_win, roundtrip_to=[]) | ||
| 413 | 221 | |||
| 414 | 222 | def test_eol_lf_with_crlf_in_repo_dirty(self): | ||
| 415 | 223 | wt, basis = self.prepare_tree(_sample_text, | ||
| 416 | 224 | eol='lf-with-crlf-in-repo') | ||
| 417 | 225 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 418 | 226 | _sample_text_on_unix, _sample_text_on_win, roundtrip_to=[]) | ||
| 419 | 227 | |||
| 420 | 228 | def test_eol_crlf_with_crlf_in_repo_dirty(self): | ||
| 421 | 229 | wt, basis = self.prepare_tree(_sample_text, | ||
| 422 | 230 | eol='crlf-with-crlf-in-repo') | ||
| 423 | 231 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 424 | 232 | _sample_text_on_unix, _sample_text_on_win, roundtrip_to=[]) | ||
| 425 | 233 | |||
| 426 | 234 | # Test text with clean line endings, either always lf or always crlf. | ||
| 427 | 235 | # This selectively roundtrips (based on what's stored in the repo). | ||
| 428 | 236 | |||
| 429 | 237 | def test_eol_no_rules_clean_lf(self): | ||
| 430 | 238 | wt, basis = self.prepare_tree(_sample_clean_lf) | ||
| 431 | 239 | self.assertContent(wt, basis, _sample_clean_lf, | ||
| 432 | 240 | _sample_text_on_unix, _sample_text_on_win, | ||
| 433 | 241 | roundtrip_to=_LF_IN_REPO) | ||
| 434 | 242 | |||
| 435 | 243 | def test_eol_no_rules_clean_crlf(self): | ||
| 436 | 244 | wt, basis = self.prepare_tree(_sample_clean_crlf) | ||
| 437 | 245 | self.assertContent(wt, basis, _sample_clean_crlf, | ||
| 438 | 246 | _sample_text_on_unix, _sample_text_on_win, | ||
| 439 | 247 | roundtrip_to=_CRLF_IN_REPO) | ||
| 440 | 248 | |||
| 441 | 249 | def test_eol_exact_clean_lf(self): | ||
| 442 | 250 | wt, basis = self.prepare_tree(_sample_clean_lf, eol='exact') | ||
| 443 | 251 | self.assertContent(wt, basis, _sample_clean_lf, | ||
| 444 | 252 | _sample_text_on_unix, _sample_text_on_win, | ||
| 445 | 253 | roundtrip_to=_LF_IN_REPO) | ||
| 446 | 254 | |||
| 447 | 255 | def test_eol_exact_clean_crlf(self): | ||
| 448 | 256 | wt, basis = self.prepare_tree(_sample_clean_crlf, eol='exact') | ||
| 449 | 257 | self.assertContent(wt, basis, _sample_clean_crlf, | ||
| 450 | 258 | _sample_text_on_unix, _sample_text_on_win, | ||
| 451 | 259 | roundtrip_to=_CRLF_IN_REPO) | ||
| 452 | 260 | |||
| 453 | 261 | def test_eol_native_clean_lf(self): | ||
| 454 | 262 | wt, basis = self.prepare_tree(_sample_clean_lf, eol='native') | ||
| 455 | 263 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 456 | 264 | _sample_text_on_unix, _sample_text_on_win, | ||
| 457 | 265 | roundtrip_to=_LF_IN_REPO) | ||
| 458 | 266 | |||
| 459 | 267 | def test_eol_native_clean_crlf(self): | ||
| 460 | 268 | wt, basis = self.prepare_tree(_sample_clean_crlf, eol='native') | ||
| 461 | 269 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 462 | 270 | _sample_text_on_unix, _sample_text_on_win, | ||
| 463 | 271 | roundtrip_to=_LF_IN_REPO) | ||
| 464 | 272 | |||
| 465 | 273 | def test_eol_lf_clean_lf(self): | ||
| 466 | 274 | wt, basis = self.prepare_tree(_sample_clean_lf, eol='lf') | ||
| 467 | 275 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 468 | 276 | _sample_text_on_unix, _sample_text_on_win, | ||
| 469 | 277 | roundtrip_to=_LF_IN_REPO) | ||
| 470 | 278 | |||
| 471 | 279 | def test_eol_lf_clean_crlf(self): | ||
| 472 | 280 | wt, basis = self.prepare_tree(_sample_clean_crlf, eol='lf') | ||
| 473 | 281 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 474 | 282 | _sample_text_on_unix, _sample_text_on_win, | ||
| 475 | 283 | roundtrip_to=_LF_IN_REPO) | ||
| 476 | 284 | |||
| 477 | 285 | def test_eol_crlf_clean_lf(self): | ||
| 478 | 286 | wt, basis = self.prepare_tree(_sample_clean_lf, eol='crlf') | ||
| 479 | 287 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 480 | 288 | _sample_text_on_unix, _sample_text_on_win, | ||
| 481 | 289 | roundtrip_to=_LF_IN_REPO) | ||
| 482 | 290 | |||
| 483 | 291 | def test_eol_crlf_clean_crlf(self): | ||
| 484 | 292 | wt, basis = self.prepare_tree(_sample_clean_crlf, eol='crlf') | ||
| 485 | 293 | self.assertContent(wt, basis, _sample_text_on_unix, | ||
| 486 | 294 | _sample_text_on_unix, _sample_text_on_win, | ||
| 487 | 295 | roundtrip_to=_LF_IN_REPO) | ||
| 488 | 296 | |||
| 489 | 297 | def test_eol_native_with_crlf_in_repo_clean_lf(self): | ||
| 490 | 298 | wt, basis = self.prepare_tree(_sample_clean_lf, | ||
| 491 | 299 | eol='native-with-crlf-in-repo') | ||
| 492 | 300 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 493 | 301 | _sample_text_on_unix, _sample_text_on_win, | ||
| 494 | 302 | roundtrip_to=_CRLF_IN_REPO) | ||
| 495 | 303 | |||
| 496 | 304 | def test_eol_native_with_crlf_in_repo_clean_crlf(self): | ||
| 497 | 305 | wt, basis = self.prepare_tree(_sample_clean_crlf, | ||
| 498 | 306 | eol='native-with-crlf-in-repo') | ||
| 499 | 307 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 500 | 308 | _sample_text_on_unix, _sample_text_on_win, | ||
| 501 | 309 | roundtrip_to=_CRLF_IN_REPO) | ||
| 502 | 310 | |||
| 503 | 311 | def test_eol_lf_with_crlf_in_repo_clean_lf(self): | ||
| 504 | 312 | wt, basis = self.prepare_tree(_sample_clean_lf, | ||
| 505 | 313 | eol='lf-with-crlf-in-repo') | ||
| 506 | 314 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 507 | 315 | _sample_text_on_unix, _sample_text_on_win, | ||
| 508 | 316 | roundtrip_to=_CRLF_IN_REPO) | ||
| 509 | 317 | |||
| 510 | 318 | def test_eol_lf_with_crlf_in_repo_clean_crlf(self): | ||
| 511 | 319 | wt, basis = self.prepare_tree(_sample_clean_crlf, | ||
| 512 | 320 | eol='lf-with-crlf-in-repo') | ||
| 513 | 321 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 514 | 322 | _sample_text_on_unix, _sample_text_on_win, | ||
| 515 | 323 | roundtrip_to=_CRLF_IN_REPO) | ||
| 516 | 324 | |||
| 517 | 325 | def test_eol_crlf_with_crlf_in_repo_clean_lf(self): | ||
| 518 | 326 | wt, basis = self.prepare_tree(_sample_clean_lf, | ||
| 519 | 327 | eol='crlf-with-crlf-in-repo') | ||
| 520 | 328 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 521 | 329 | _sample_text_on_unix, _sample_text_on_win, | ||
| 522 | 330 | roundtrip_to=_CRLF_IN_REPO) | ||
| 523 | 331 | |||
| 524 | 332 | def test_eol_crlf_with_crlf_in_repo_clean_crlf(self): | ||
| 525 | 333 | wt, basis = self.prepare_tree(_sample_clean_crlf, | ||
| 526 | 334 | eol='crlf-with-crlf-in-repo') | ||
| 527 | 335 | self.assertContent(wt, basis, _sample_text_on_win, | ||
| 528 | 336 | _sample_text_on_unix, _sample_text_on_win, | ||
| 529 | 337 | roundtrip_to=_CRLF_IN_REPO) | ||
| 530 | 186 | 338 | ||
| 531 | === modified file 'bzrlib/workingtree_4.py' | |||
| 532 | --- bzrlib/workingtree_4.py 2009-06-10 03:56:49 +0000 | |||
| 533 | +++ bzrlib/workingtree_4.py 2009-06-11 04:35:14 +0000 | |||
| 534 | @@ -1440,13 +1440,20 @@ | |||
| 535 | 1440 | # Note: do NOT move this logic up higher - using the basis from | 1440 | # Note: do NOT move this logic up higher - using the basis from |
| 536 | 1441 | # the accelerator tree is still desirable because that can save | 1441 | # the accelerator tree is still desirable because that can save |
| 537 | 1442 | # a minute or more of processing on large trees! | 1442 | # a minute or more of processing on large trees! |
| 538 | 1443 | # The original tree may not have the same content filters | ||
| 539 | 1444 | # applied so we can't safely build the inventory delta from | ||
| 540 | 1445 | # the source tree. | ||
| 541 | 1443 | if wt.supports_content_filtering(): | 1446 | if wt.supports_content_filtering(): |
| 542 | 1444 | accelerator_tree = None | 1447 | accelerator_tree = None |
| 543 | 1448 | delta_from_tree = False | ||
| 544 | 1449 | else: | ||
| 545 | 1450 | delta_from_tree = True | ||
| 546 | 1445 | # delta_from_tree is safe even for DirStateRevisionTrees, | 1451 | # delta_from_tree is safe even for DirStateRevisionTrees, |
| 547 | 1446 | # because wt4.apply_inventory_delta does not mutate the input | 1452 | # because wt4.apply_inventory_delta does not mutate the input |
| 548 | 1447 | # inventory entries. | 1453 | # inventory entries. |
| 549 | 1448 | transform.build_tree(basis, wt, accelerator_tree, | 1454 | transform.build_tree(basis, wt, accelerator_tree, |
| 551 | 1449 | hardlink=hardlink, delta_from_tree=True) | 1455 | hardlink=hardlink, |
| 552 | 1456 | delta_from_tree=delta_from_tree) | ||
| 553 | 1450 | finally: | 1457 | finally: |
| 554 | 1451 | basis.unlock() | 1458 | basis.unlock() |
| 555 | 1452 | finally: | 1459 | finally: |
This patch fixes some very bad bugs in content filtering. There are actually 2 separate issues fixed:
1. Branching from a non-content-
2. Status detection was relying on size matching to determine equivalence when that assumption doesn't hold in the presence of content filtering.
Both code changes are quite straightforward, though the latter one requires fixes in both Python and Pyrex code. Testing all of this is far more complex though. I've added a heap more units tests and I'm comfortable that status on content filtered trees now has pretty good coverage.
I'm yet to add detailed tests for the branch/commit issue, because the monkey-patching approach used to test status isn't at the right level to work for branch. I have tested it manually though and one of the bug reporters (Frits) has confirmed that the code fixes works correctly for him. Given the related code change - switching off an optional optimisation - is very straight forward, I think this patch can land as is for 1.16rc1.
FWIW, my plan is to add those additional tests soon. (Some pending changes to the monkey-patching used here to fix some other bugs will make that easier, but they aren't ready to go, and this really needs to be landed for content filtering to be unbroken for users.)