Bazaar

Merge lp:~vila/bzr/323111-deprecate-get-backup-name into lp:bzr

  1. 323111-deprecate-get-backup-name
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Merged at revision: 5504
Proposed branch: lp:~vila/bzr/323111-deprecate-get-backup-name
Merge into: lp:bzr
Prerequisite: lp:~vila/bzr/323111-orphan-non-versioned-files
Diff against target: 206 lines (+75/-53) (has conflicts)
3 files modified
bzrlib/tests/test_transform.py (+23/-30)
bzrlib/transform.py (+47/-22)
doc/en/release-notes/bzr-2.3.txt (+5/-1)
Text conflict in doc/en/whats-new/whats-new-in-2.3.txt
To merge this branch: bzr merge lp:~vila/bzr/323111-deprecate-get-backup-name
Reviewer Review Type Date Requested Status
Martin Pool Approve
Review via email: mp+35111@code.launchpad.net

Description of the change

This makes all features requiring a back name use
`osutils.available_backup_name` and deprecate some code in
bzrlib.transform that was used only for backups.

I simplified the code replacing
bzrlib.transform.TreeTransformBase.has_named_child has some of it
wasn't needed for the use case (no tests were harmed (failed :)
during this refactoring.

bzrlib.transform.get_backup_name was not used in the code base.
bzrlib.transform._get_backup_name was used only once.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

Updated, please review.

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

On 11 September 2010 00:44, Vincent Ladeuil <email address hidden> wrote:
> -class TestGetBackupName(TestCase):
> -    def test_get_backup_name(self):
> +class TestBackupName(tests.TestCase):
> +
> +    def test_deprecations(self):
> +        class MockTransform(object):
> +
> +            def has_named_child(self, by_parent, parent_id, name):
> +                return name in by_parent.get(parent_id, [])
> +
> +        class MockEntry(object):
> +
> +            def __init__(self):
> +                object.__init__(self)
> +                self.name = "name"
> +

Generally I'd only put the mock object inside the method if I was sure
it would only be used from there, which is probably true now this is
deprecated.

> +        name2 = self.applyDeprecated(
> +                symbol_versioning.deprecated_in((2, 3, 0)),
> +                transform._get_backup_name, 'name', {'a':['name.~1~']}, 'a', tt)
> +        self.assertEqual('name.~2~', name2)

I like the style insisted upon by Launchpad of using only 4-character
indents for continuation lines... I think we say it's not a big deal
and it's not.

> +@deprecated_function(deprecated_in((2, 3, 0)))
>  def _get_backup_name(name, by_parent, parent_trans_id, tt):
>     """Produce a backup-style name that appears to be available"""
>     def name_gen():
> @@ -2697,9 +2722,8 @@
>                         tt.delete_contents(trans_id)
>                     elif kind[1] is not None:
>                         parent_trans_id = tt.trans_id_file_id(parent[0])
> -                        by_parent = tt.by_parent()
> -                        backup_name = _get_backup_name(name[0], by_parent,
> -                                                       parent_trans_id, tt)
> +                        backup_name = tt._available_backup_name(name[0],
> +                                                                parent_trans_id)

This to me is a clear case where the other indent rule is cleaner.
<https://dev.launchpad.net/PythonStyleGuide#Multiline+function+calls>

The substance seems fine.

--
Martin

Revision history for this message
Martin Pool (mbp) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/test_transform.py'
2--- bzrlib/tests/test_transform.py 2010-10-15 14:15:59 +0000
3+++ bzrlib/tests/test_transform.py 2010-10-15 14:16:00 +0000
4@@ -27,6 +27,7 @@
5 osutils,
6 revision as _mod_revision,
7 rules,
8+ symbol_versioning,
9 tests,
10 transform,
11 urlutils,
12@@ -2196,37 +2197,29 @@
13 self.assertEqual('tree', revision.properties['branch-nick'])
14
15
16-class MockTransform(object):
17-
18- def has_named_child(self, by_parent, parent_id, name):
19- for child_id in by_parent[parent_id]:
20- if child_id == '0':
21- if name == "name~":
22- return True
23- elif name == "name.~%s~" % child_id:
24- return True
25- return False
26-
27-
28-class MockEntry(object):
29- def __init__(self):
30- object.__init__(self)
31- self.name = "name"
32-
33-
34-class TestGetBackupName(TestCase):
35- def test_get_backup_name(self):
36+class TestBackupName(tests.TestCase):
37+
38+ def test_deprecations(self):
39+ class MockTransform(object):
40+
41+ def has_named_child(self, by_parent, parent_id, name):
42+ return name in by_parent.get(parent_id, [])
43+
44+ class MockEntry(object):
45+
46+ def __init__(self):
47+ object.__init__(self)
48+ self.name = "name"
49+
50 tt = MockTransform()
51- name = get_backup_name(MockEntry(), {'a':[]}, 'a', tt)
52- self.assertEqual(name, 'name.~1~')
53- name = get_backup_name(MockEntry(), {'a':['1']}, 'a', tt)
54- self.assertEqual(name, 'name.~2~')
55- name = get_backup_name(MockEntry(), {'a':['2']}, 'a', tt)
56- self.assertEqual(name, 'name.~1~')
57- name = get_backup_name(MockEntry(), {'a':['2'], 'b':[]}, 'b', tt)
58- self.assertEqual(name, 'name.~1~')
59- name = get_backup_name(MockEntry(), {'a':['1', '2', '3']}, 'a', tt)
60- self.assertEqual(name, 'name.~4~')
61+ name1 = self.applyDeprecated(
62+ symbol_versioning.deprecated_in((2, 3, 0)),
63+ transform.get_backup_name, MockEntry(), {'a':[]}, 'a', tt)
64+ self.assertEqual('name.~1~', name1)
65+ name2 = self.applyDeprecated(
66+ symbol_versioning.deprecated_in((2, 3, 0)),
67+ transform._get_backup_name, 'name', {'a':['name.~1~']}, 'a', tt)
68+ self.assertEqual('name.~2~', name2)
69
70
71 class TestFileMover(tests.TestCaseWithTransport):
72
73=== modified file 'bzrlib/transform.py'
74--- bzrlib/transform.py 2010-10-15 14:15:59 +0000
75+++ bzrlib/transform.py 2010-10-15 14:16:00 +0000
76@@ -59,6 +59,7 @@
77 from bzrlib.symbol_versioning import (
78 deprecated_function,
79 deprecated_in,
80+ deprecated_method,
81 )
82 from bzrlib.trace import mutter, warning
83 from bzrlib import tree
84@@ -534,30 +535,53 @@
85 # ensure that all children are registered with the transaction
86 list(self.iter_tree_children(parent_id))
87
88+ @deprecated_method(deprecated_in((2, 3, 0)))
89 def has_named_child(self, by_parent, parent_id, name):
90- try:
91- children = by_parent[parent_id]
92- except KeyError:
93- children = []
94- for child in children:
95+ return self._has_named_child(
96+ name, parent_id, known_children=by_parent.get(parent_id, []))
97+
98+ def _has_named_child(self, name, parent_id, known_children):
99+ """Does a parent already have a name child.
100+
101+ :param name: The searched for name.
102+
103+ :param parent_id: The parent for which the check is made.
104+
105+ :param known_children: The already known children. This should have
106+ been recently obtained from `self.by_parent.get(parent_id)`
107+ (or will be if None is passed).
108+ """
109+ if known_children is None:
110+ known_children = self.by_parent().get(parent_id, [])
111+ for child in known_children:
112 if self.final_name(child) == name:
113 return True
114- try:
115- path = self._tree_id_paths[parent_id]
116- except KeyError:
117+ parent_path = self._tree_id_paths.get(parent_id, None)
118+ if parent_path is None:
119+ # No parent... no children
120 return False
121- childpath = joinpath(path, name)
122- child_id = self._tree_path_ids.get(childpath)
123+ child_path = joinpath(parent_path, name)
124+ child_id = self._tree_path_ids.get(child_path, None)
125 if child_id is None:
126- return lexists(self._tree.abspath(childpath))
127+ # Not known by the tree transform yet, check the filesystem
128+ return osutils.lexists(self._tree.abspath(child_path))
129 else:
130- if self.final_parent(child_id) != parent_id:
131- return False
132- if child_id in self._removed_contents:
133- # XXX What about dangling file-ids?
134- return False
135- else:
136- return True
137+ raise AssertionError('child_id is missing: %s, %s, %s'
138+ % (name, parent_id, child_id))
139+
140+ def _available_backup_name(self, name, target_id):
141+ """Find an available backup name.
142+
143+ :param name: The basename of the file.
144+
145+ :param target_id: The directory trans_id where the backup should
146+ be placed.
147+ """
148+ known_children = self.by_parent().get(target_id, [])
149+ return osutils.available_backup_name(
150+ name,
151+ lambda base: self._has_named_child(
152+ base, target_id, known_children))
153
154 def _parent_loops(self):
155 """No entry should be its own ancestor"""
156@@ -1295,7 +1319,7 @@
157 parent_path = self._tree_id_paths[parent_id]
158 # Find a name that doesn't exist yet in the orphan dir
159 actual_name = self.final_name(trans_id)
160- new_name = _get_backup_name(actual_name, self.by_parent(), od_id, self)
161+ new_name = self._available_backup_name(actual_name, od_id)
162 self.adjust_path(new_name, od_id, trans_id)
163 trace.warning('%s has been orphaned in %s'
164 % (joinpath(parent_path, actual_name), orphan_dir))
165@@ -2572,10 +2596,12 @@
166 tt.set_executability(entry.executable, trans_id)
167
168
169+@deprecated_function(deprecated_in((2, 3, 0)))
170 def get_backup_name(entry, by_parent, parent_trans_id, tt):
171 return _get_backup_name(entry.name, by_parent, parent_trans_id, tt)
172
173
174+@deprecated_function(deprecated_in((2, 3, 0)))
175 def _get_backup_name(name, by_parent, parent_trans_id, tt):
176 """Produce a backup-style name that appears to be available"""
177 def name_gen():
178@@ -2702,9 +2728,8 @@
179 tt.delete_contents(trans_id)
180 elif kind[1] is not None:
181 parent_trans_id = tt.trans_id_file_id(parent[0])
182- by_parent = tt.by_parent()
183- backup_name = _get_backup_name(name[0], by_parent,
184- parent_trans_id, tt)
185+ backup_name = tt._available_backup_name(
186+ name[0], parent_trans_id)
187 tt.adjust_path(backup_name, parent_trans_id, trans_id)
188 new_trans_id = tt.create_path(name[0], parent_trans_id)
189 if versioned == (True, True):
190
191=== modified file 'doc/en/release-notes/bzr-2.3.txt'
192--- doc/en/release-notes/bzr-2.3.txt 2010-10-15 14:15:59 +0000
193+++ doc/en/release-notes/bzr-2.3.txt 2010-10-15 14:16:00 +0000
194@@ -105,7 +105,11 @@
195
196 * ``BzrDir.generate_backup_name`` has been deprecated and replaced by a
197 private method. ``osutils.available_backup_name`` provides an extensible
198- replacement. (Vincent Ladeuil)
199+ replacement. This allowed the deprecation of
200+ ``bzrlib.transform.get_backup_name``,
201+ ``bzrlib.transform._get_backup_name`` and
202+ ``bzrlib.transform.TreeTransformBase.has_named_child``.
203+ (Vincent Ladeuil)
204
205 New Features
206 ************