Bazaar

Merge lp:~abentley/bzr/ascii-preview into lp:bzr/2.0

  1. ascii-preview
  2. Merge into 2.0
Proposed by Aaron Bentley
Status: Merged
Merged at revision: not available
Proposed branch: lp:~abentley/bzr/ascii-preview
Merge into: lp:bzr/2.0
Diff against target: 152 lines
3 files modified
NEWS (+3/-0)
bzrlib/tests/test_transform.py (+10/-0)
bzrlib/transform.py (+60/-39)
To merge this branch: bzr merge lp:~abentley/bzr/ascii-preview
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Review via email: mp+13371@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

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

Hi all,

This patch fixes a bug with PreviewTrees that the filenames they can
represent are limited by the encoding of the temp directory's filesystem.

It does this by moving the limbo name generation into
_generate_limbo_name, and then providing implementations on
TreeTransform and DiskTreeTransform. The DiskTreeTransform version uses
ascii-only names based on trans-id, which was already the fallback
strategy for TreeTransform. The TreeTransform version is optimized to
avoid unnecessary renames when apply is invoked. This optimization is
only relevant to TreeTransform, because only TreeTransforms can be applied.

I've targetted this to 2.0 because it's a bugfix.

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

iEYEARECAAYFAkrWG2sACgkQ0F+nu1YWqI1p9QCghDdSC4K/t5ZEq+lBY2gLjSLM
OHsAn2LMlSJVAeYJvN/iiko88oJnLd6i
=Yvof
-----END PGP SIGNATURE-----

Revision history for this message
Vincent Ladeuil (vila) wrote :

My only nit is that the test could have been closer to _generate_limbo_path,
but since it's private, that's a small nit.

Thanks for the fix.

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-10-15 19:47:18 +0000
3+++ NEWS 2009-10-16 15:17:12 +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+* PreviewTree file names are not limited by the encoding of the temp
9+ directory's filesystem. (Aaron Bentley, #436794)
10+
11 Improvements
12 ************
13
14
15=== modified file 'bzrlib/tests/test_transform.py'
16--- bzrlib/tests/test_transform.py 2009-09-29 20:35:55 +0000
17+++ bzrlib/tests/test_transform.py 2009-10-16 15:17:12 +0000
18@@ -2727,6 +2727,16 @@
19 rev2_tree = tree.branch.repository.revision_tree(rev2_id)
20 self.assertEqual('contents', rev2_tree.get_file_text('file_id'))
21
22+ def test_ascii_limbo_paths(self):
23+ self.requireFeature(tests.UnicodeFilenameFeature)
24+ branch = self.make_branch('any')
25+ tree = branch.repository.revision_tree(_mod_revision.NULL_REVISION)
26+ tt = TransformPreview(tree)
27+ foo_id = tt.new_directory('', ROOT_PARENT)
28+ bar_id = tt.new_file(u'\u1234bar', foo_id, 'contents')
29+ limbo_path = tt._limbo_name(bar_id)
30+ self.assertEqual(limbo_path.encode('ascii', 'replace'), limbo_path)
31+
32
33 class FakeSerializer(object):
34 """Serializer implementation that simply returns the input.
35
36=== modified file 'bzrlib/transform.py'
37--- bzrlib/transform.py 2009-09-29 21:08:38 +0000
38+++ bzrlib/transform.py 2009-10-16 15:17:12 +0000
39@@ -1054,47 +1054,20 @@
40 def _limbo_name(self, trans_id):
41 """Generate the limbo name of a file"""
42 limbo_name = self._limbo_files.get(trans_id)
43- if limbo_name is not None:
44- return limbo_name
45- parent = self._new_parent.get(trans_id)
46- # if the parent directory is already in limbo (e.g. when building a
47- # tree), choose a limbo name inside the parent, to reduce further
48- # renames.
49- use_direct_path = False
50- if self._new_contents.get(parent) == 'directory':
51- filename = self._new_name.get(trans_id)
52- if filename is not None:
53- if parent not in self._limbo_children:
54- self._limbo_children[parent] = set()
55- self._limbo_children_names[parent] = {}
56- use_direct_path = True
57- # the direct path can only be used if no other file has
58- # already taken this pathname, i.e. if the name is unused, or
59- # if it is already associated with this trans_id.
60- elif self._case_sensitive_target:
61- if (self._limbo_children_names[parent].get(filename)
62- in (trans_id, None)):
63- use_direct_path = True
64- else:
65- for l_filename, l_trans_id in\
66- self._limbo_children_names[parent].iteritems():
67- if l_trans_id == trans_id:
68- continue
69- if l_filename.lower() == filename.lower():
70- break
71- else:
72- use_direct_path = True
73-
74- if use_direct_path:
75- limbo_name = pathjoin(self._limbo_files[parent], filename)
76- self._limbo_children[parent].add(trans_id)
77- self._limbo_children_names[parent][filename] = trans_id
78- else:
79- limbo_name = pathjoin(self._limbodir, trans_id)
80- self._needs_rename.add(trans_id)
81- self._limbo_files[trans_id] = limbo_name
82+ if limbo_name is None:
83+ limbo_name = self._generate_limbo_path(trans_id)
84+ self._limbo_files[trans_id] = limbo_name
85 return limbo_name
86
87+ def _generate_limbo_path(self, trans_id):
88+ """Generate a limbo path using the trans_id as the relative path.
89+
90+ This is suitable as a fallback, and when the transform should not be
91+ sensitive to the path encoding of the limbo directory.
92+ """
93+ self._needs_rename.add(trans_id)
94+ return pathjoin(self._limbodir, trans_id)
95+
96 def adjust_path(self, name, parent, trans_id):
97 previous_parent = self._new_parent.get(trans_id)
98 previous_name = self._new_name.get(trans_id)
99@@ -1396,6 +1369,54 @@
100 continue
101 yield self.trans_id_tree_path(childpath)
102
103+ def _generate_limbo_path(self, trans_id):
104+ """Generate a limbo path using the final path if possible.
105+
106+ This optimizes the performance of applying the tree transform by
107+ avoiding renames. These renames can be avoided only when the parent
108+ directory is already scheduled for creation.
109+
110+ If the final path cannot be used, falls back to using the trans_id as
111+ the relpath.
112+ """
113+ parent = self._new_parent.get(trans_id)
114+ # if the parent directory is already in limbo (e.g. when building a
115+ # tree), choose a limbo name inside the parent, to reduce further
116+ # renames.
117+ use_direct_path = False
118+ if self._new_contents.get(parent) == 'directory':
119+ filename = self._new_name.get(trans_id)
120+ if filename is not None:
121+ if parent not in self._limbo_children:
122+ self._limbo_children[parent] = set()
123+ self._limbo_children_names[parent] = {}
124+ use_direct_path = True
125+ # the direct path can only be used if no other file has
126+ # already taken this pathname, i.e. if the name is unused, or
127+ # if it is already associated with this trans_id.
128+ elif self._case_sensitive_target:
129+ if (self._limbo_children_names[parent].get(filename)
130+ in (trans_id, None)):
131+ use_direct_path = True
132+ else:
133+ for l_filename, l_trans_id in\
134+ self._limbo_children_names[parent].iteritems():
135+ if l_trans_id == trans_id:
136+ continue
137+ if l_filename.lower() == filename.lower():
138+ break
139+ else:
140+ use_direct_path = True
141+
142+ if not use_direct_path:
143+ return DiskTreeTransform._generate_limbo_path(self, trans_id)
144+
145+ limbo_name = pathjoin(self._limbo_files[parent], filename)
146+ self._limbo_children[parent].add(trans_id)
147+ self._limbo_children_names[parent][filename] = trans_id
148+ return limbo_name
149+
150+
151 def apply(self, no_conflicts=False, precomputed_delta=None, _mover=None):
152 """Apply all changes to the inventory and filesystem.
153

Subscribers

People subscribed via source and target branches