Bazaar

Merge lp:~mbp/bzr/368931-rename-case-2.0 into lp:bzr/2.0

  1. 368931-rename-case-2.0
  2. Merge into 2.0
Proposed by Martin Pool
Status: Merged
Merged at revision: not available
Proposed branch: lp:~mbp/bzr/368931-rename-case-2.0
Merge into: lp:bzr/2.0
Diff against target: 147 lines (+67/-6)
4 files modified
NEWS (+3/-0)
bzrlib/tests/__init__.py (+18/-1)
bzrlib/tests/per_tree/test_inv.py (+25/-2)
bzrlib/tree.py (+21/-3)
To merge this branch: bzr merge lp:~mbp/bzr/368931-rename-case-2.0
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing
bzr-core Pending
Review via email: mp+19079@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

This finishes off Chris Jones's patch in https://bugs.edge.launchpad.net/bzr/+bug/368931 by adding a test.

This function could be faster but I'm not sure if it's actually a hot spot.

This finishes our last high-priority bug with a patch! There are still 34 more with lower importance.

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

32 + self.build_tree(['test/', 'test/file', 'Test'])

This is not valid on Windows.

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

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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/368931-rename-case-2.0 into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #368931 Rename may fail when file and directory have the same name differing by case
> https://bugs.launchpad.net/bugs/368931
>
>
> This finishes off Chris Jones's patch in https://bugs.edge.launchpad.net/bzr/+bug/368931 by adding a test.
>
> This function could be faster but I'm not sure if it's actually a hot spot.
>
> This finishes our last high-priority bug with a patch! There are still 34 more with lower importance.
>

I agree with your TODO and NOTE comments.

Also using "iter_children()" throws out all sorts of information. It
returns only file_ids, when it already looked up everything as a
InventoryEntry. Also, falling back to 'id2path' is wasteful given that
we already have the containing directory, and all we need is the child
name. (Which is on the IE we just through away from iter_children.)

I would also guess that there is still a bug if "Tree" is versioned but
"tree" is not (or vice versa).

It sort of depends if this is only supposed to work on versioned paths,
or work on both versioned and unversioned files.

(If tree is versioned but Tree is not, and you do "bzr status Tree",
what should it match?)

John
=:->

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

iEYEARECAAYFAkt0iSoACgkQJdeBCYSNAANrWgCgxtR1cfkQPV0VEEKqD6P186ad
uxgAn1rpE4/PtDIKhNPxC4joAiVBjTW2
=PvyE
-----END PGP SIGNATURE-----

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

This makes it conditional on having a case-insensitive filesystem.

We can't get away from that because some of the tree classes we want to test must exist on disk.

I tested under winepython and it works.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-02-05 06:30:42 +0000
+++ NEWS 2010-02-12 03:41:12 +0000
@@ -14,6 +14,9 @@
14Bug Fixes14Bug Fixes
15*********15*********
1616
17* Handle renames correctly when there are files or directories that
18 differ only in case. (Chris Jones, Martin Pool, #368931)
19
17* If ``bzr push --create-prefix`` triggers an unexpected ``NoSuchFile``20* If ``bzr push --create-prefix`` triggers an unexpected ``NoSuchFile``
18 error, report that error rather than failing with an unhelpful21 error, report that error rather than failing with an unhelpful
19 ``UnboundLocalError``.22 ``UnboundLocalError``.
2023
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py 2009-10-29 04:19:13 +0000
+++ bzrlib/tests/__init__.py 2010-02-12 03:41:12 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd1# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -4060,6 +4060,23 @@
4060CaseInsensitiveFilesystemFeature = _CaseInsensitiveFilesystemFeature()4060CaseInsensitiveFilesystemFeature = _CaseInsensitiveFilesystemFeature()
40614061
40624062
4063class _CaseSensitiveFilesystemFeature(Feature):
4064
4065 def _probe(self):
4066 if CaseInsCasePresFilenameFeature.available():
4067 return False
4068 elif CaseInsensitiveFilesystemFeature.available():
4069 return False
4070 else:
4071 return True
4072
4073 def feature_name(self):
4074 return 'case-sensitive filesystem'
4075
4076# new coding style is for feature instances to be lowercase
4077case_sensitive_filesystem_feature = _CaseSensitiveFilesystemFeature()
4078
4079
4063class _SubUnitFeature(Feature):4080class _SubUnitFeature(Feature):
4064 """Check if subunit is available."""4081 """Check if subunit is available."""
40654082
40664083
=== modified file 'bzrlib/tests/per_tree/test_inv.py'
--- bzrlib/tests/per_tree/test_inv.py 2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_tree/test_inv.py 2010-02-12 03:41:12 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2007, 2008 Canonical Ltd1# Copyright (C) 2007, 2008, 2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -23,7 +23,10 @@
23from bzrlib import (23from bzrlib import (
24 tests,24 tests,
25 )25 )
26from bzrlib.tests import per_tree26from bzrlib.tests import (
27 features,
28 per_tree,
29 )
27from bzrlib.mutabletree import MutableTree30from bzrlib.mutabletree import MutableTree
28from bzrlib.tests import SymlinkFeature, TestSkipped31from bzrlib.tests import SymlinkFeature, TestSkipped
29from bzrlib.transform import _PreviewTree32from bzrlib.transform import _PreviewTree
@@ -131,6 +134,8 @@
131 work_tree.add(['dir', 'dir/file'])134 work_tree.add(['dir', 'dir/file'])
132 if commit:135 if commit:
133 work_tree.commit('commit 1')136 work_tree.commit('commit 1')
137 # XXX: this isn't actually guaranteed to return the class we want to
138 # test -- mbp 2010-02-12
134 return work_tree139 return work_tree
135140
136 def test_canonical_path(self):141 def test_canonical_path(self):
@@ -163,3 +168,21 @@
163 work_tree = self._make_canonical_test_tree()168 work_tree = self._make_canonical_test_tree()
164 self.assertEqual('dir/None',169 self.assertEqual('dir/None',
165 work_tree.get_canonical_inventory_path('Dir/None'))170 work_tree.get_canonical_inventory_path('Dir/None'))
171
172 def test_canonical_tree_name_mismatch(self):
173 # see <https://bugs.edge.launchpad.net/bzr/+bug/368931>
174 # some of the trees we want to use can only exist on a disk, not in
175 # memory - therefore we can only test this if the filesystem is
176 # case-sensitive.
177 self.requireFeature(tests.case_sensitive_filesystem_feature)
178 work_tree = self.make_branch_and_tree('.')
179 self.build_tree(['test/', 'test/file', 'Test'])
180 work_tree.add(['test/', 'test/file', 'Test'])
181
182 test_tree = self._convert_tree(work_tree)
183 test_tree.lock_read()
184 self.addCleanup(test_tree.unlock)
185
186 self.assertEqual(['test', 'test/file', 'Test', 'test/foo', 'Test/foo'],
187 test_tree.get_canonical_inventory_paths(
188 ['test', 'test/file', 'Test', 'test/foo', 'Test/foo']))
166189
=== modified file 'bzrlib/tree.py'
--- bzrlib/tree.py 2009-08-26 05:38:16 +0000
+++ bzrlib/tree.py 2010-02-12 03:41:12 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2005, 2009 Canonical Ltd1# Copyright (C) 2005, 2009, 2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -404,16 +404,34 @@
404 bit_iter = iter(path.split("/"))404 bit_iter = iter(path.split("/"))
405 for elt in bit_iter:405 for elt in bit_iter:
406 lelt = elt.lower()406 lelt = elt.lower()
407 new_path = None
407 for child in self.iter_children(cur_id):408 for child in self.iter_children(cur_id):
408 try:409 try:
410 # XXX: it seem like if the child is known to be in the
411 # tree, we shouldn't need to go from its id back to
412 # its path -- mbp 2010-02-11
413 #
414 # XXX: it seems like we could be more efficient
415 # by just directly looking up the original name and
416 # only then searching all children; also by not
417 # chopping paths so much. -- mbp 2010-02-11
409 child_base = os.path.basename(self.id2path(child))418 child_base = os.path.basename(self.id2path(child))
410 if child_base.lower() == lelt:419 if (child_base == elt):
420 # if we found an exact match, we can stop now; if
421 # we found an approximate match we need to keep
422 # searching because there might be an exact match
423 # later.
411 cur_id = child424 cur_id = child
412 cur_path = osutils.pathjoin(cur_path, child_base)425 new_path = osutils.pathjoin(cur_path, child_base)
413 break426 break
427 elif child_base.lower() == lelt:
428 cur_id = child
429 new_path = osutils.pathjoin(cur_path, child_base)
414 except NoSuchId:430 except NoSuchId:
415 # before a change is committed we can see this error...431 # before a change is committed we can see this error...
416 continue432 continue
433 if new_path:
434 cur_path = new_path
417 else:435 else:
418 # got to the end of this directory and no entries matched.436 # got to the end of this directory and no entries matched.
419 # Return what matched so far, plus the rest as specified.437 # Return what matched so far, plus the rest as specified.

Subscribers

People subscribed via source and target branches