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
1=== modified file 'NEWS'
2--- NEWS 2010-02-05 06:30:42 +0000
3+++ NEWS 2010-02-12 03:41:12 +0000
4@@ -14,6 +14,9 @@
5 Bug Fixes
6 *********
7
8+* Handle renames correctly when there are files or directories that
9+ differ only in case. (Chris Jones, Martin Pool, #368931)
10+
11 * If ``bzr push --create-prefix`` triggers an unexpected ``NoSuchFile``
12 error, report that error rather than failing with an unhelpful
13 ``UnboundLocalError``.
14
15=== modified file 'bzrlib/tests/__init__.py'
16--- bzrlib/tests/__init__.py 2009-10-29 04:19:13 +0000
17+++ bzrlib/tests/__init__.py 2010-02-12 03:41:12 +0000
18@@ -1,4 +1,4 @@
19-# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd
20+# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
21 #
22 # This program is free software; you can redistribute it and/or modify
23 # it under the terms of the GNU General Public License as published by
24@@ -4060,6 +4060,23 @@
25 CaseInsensitiveFilesystemFeature = _CaseInsensitiveFilesystemFeature()
26
27
28+class _CaseSensitiveFilesystemFeature(Feature):
29+
30+ def _probe(self):
31+ if CaseInsCasePresFilenameFeature.available():
32+ return False
33+ elif CaseInsensitiveFilesystemFeature.available():
34+ return False
35+ else:
36+ return True
37+
38+ def feature_name(self):
39+ return 'case-sensitive filesystem'
40+
41+# new coding style is for feature instances to be lowercase
42+case_sensitive_filesystem_feature = _CaseSensitiveFilesystemFeature()
43+
44+
45 class _SubUnitFeature(Feature):
46 """Check if subunit is available."""
47
48
49=== modified file 'bzrlib/tests/per_tree/test_inv.py'
50--- bzrlib/tests/per_tree/test_inv.py 2009-07-10 07:14:02 +0000
51+++ bzrlib/tests/per_tree/test_inv.py 2010-02-12 03:41:12 +0000
52@@ -1,4 +1,4 @@
53-# Copyright (C) 2007, 2008 Canonical Ltd
54+# Copyright (C) 2007, 2008, 2010 Canonical Ltd
55 #
56 # This program is free software; you can redistribute it and/or modify
57 # it under the terms of the GNU General Public License as published by
58@@ -23,7 +23,10 @@
59 from bzrlib import (
60 tests,
61 )
62-from bzrlib.tests import per_tree
63+from bzrlib.tests import (
64+ features,
65+ per_tree,
66+ )
67 from bzrlib.mutabletree import MutableTree
68 from bzrlib.tests import SymlinkFeature, TestSkipped
69 from bzrlib.transform import _PreviewTree
70@@ -131,6 +134,8 @@
71 work_tree.add(['dir', 'dir/file'])
72 if commit:
73 work_tree.commit('commit 1')
74+ # XXX: this isn't actually guaranteed to return the class we want to
75+ # test -- mbp 2010-02-12
76 return work_tree
77
78 def test_canonical_path(self):
79@@ -163,3 +168,21 @@
80 work_tree = self._make_canonical_test_tree()
81 self.assertEqual('dir/None',
82 work_tree.get_canonical_inventory_path('Dir/None'))
83+
84+ def test_canonical_tree_name_mismatch(self):
85+ # see <https://bugs.edge.launchpad.net/bzr/+bug/368931>
86+ # some of the trees we want to use can only exist on a disk, not in
87+ # memory - therefore we can only test this if the filesystem is
88+ # case-sensitive.
89+ self.requireFeature(tests.case_sensitive_filesystem_feature)
90+ work_tree = self.make_branch_and_tree('.')
91+ self.build_tree(['test/', 'test/file', 'Test'])
92+ work_tree.add(['test/', 'test/file', 'Test'])
93+
94+ test_tree = self._convert_tree(work_tree)
95+ test_tree.lock_read()
96+ self.addCleanup(test_tree.unlock)
97+
98+ self.assertEqual(['test', 'test/file', 'Test', 'test/foo', 'Test/foo'],
99+ test_tree.get_canonical_inventory_paths(
100+ ['test', 'test/file', 'Test', 'test/foo', 'Test/foo']))
101
102=== modified file 'bzrlib/tree.py'
103--- bzrlib/tree.py 2009-08-26 05:38:16 +0000
104+++ bzrlib/tree.py 2010-02-12 03:41:12 +0000
105@@ -1,4 +1,4 @@
106-# Copyright (C) 2005, 2009 Canonical Ltd
107+# Copyright (C) 2005, 2009, 2010 Canonical Ltd
108 #
109 # This program is free software; you can redistribute it and/or modify
110 # it under the terms of the GNU General Public License as published by
111@@ -404,16 +404,34 @@
112 bit_iter = iter(path.split("/"))
113 for elt in bit_iter:
114 lelt = elt.lower()
115+ new_path = None
116 for child in self.iter_children(cur_id):
117 try:
118+ # XXX: it seem like if the child is known to be in the
119+ # tree, we shouldn't need to go from its id back to
120+ # its path -- mbp 2010-02-11
121+ #
122+ # XXX: it seems like we could be more efficient
123+ # by just directly looking up the original name and
124+ # only then searching all children; also by not
125+ # chopping paths so much. -- mbp 2010-02-11
126 child_base = os.path.basename(self.id2path(child))
127- if child_base.lower() == lelt:
128+ if (child_base == elt):
129+ # if we found an exact match, we can stop now; if
130+ # we found an approximate match we need to keep
131+ # searching because there might be an exact match
132+ # later.
133 cur_id = child
134- cur_path = osutils.pathjoin(cur_path, child_base)
135+ new_path = osutils.pathjoin(cur_path, child_base)
136 break
137+ elif child_base.lower() == lelt:
138+ cur_id = child
139+ new_path = osutils.pathjoin(cur_path, child_base)
140 except NoSuchId:
141 # before a change is committed we can see this error...
142 continue
143+ if new_path:
144+ cur_path = new_path
145 else:
146 # got to the end of this directory and no entries matched.
147 # Return what matched so far, plus the rest as specified.

Subscribers

People subscribed via source and target branches