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
=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py 2010-10-15 14:15:59 +0000
+++ bzrlib/tests/test_transform.py 2010-10-15 14:16:00 +0000
@@ -27,6 +27,7 @@
27 osutils,27 osutils,
28 revision as _mod_revision,28 revision as _mod_revision,
29 rules,29 rules,
30 symbol_versioning,
30 tests,31 tests,
31 transform,32 transform,
32 urlutils,33 urlutils,
@@ -2196,37 +2197,29 @@
2196 self.assertEqual('tree', revision.properties['branch-nick'])2197 self.assertEqual('tree', revision.properties['branch-nick'])
21972198
21982199
2199class MockTransform(object):2200class TestBackupName(tests.TestCase):
22002201
2201 def has_named_child(self, by_parent, parent_id, name):2202 def test_deprecations(self):
2202 for child_id in by_parent[parent_id]:2203 class MockTransform(object):
2203 if child_id == '0':2204
2204 if name == "name~":2205 def has_named_child(self, by_parent, parent_id, name):
2205 return True2206 return name in by_parent.get(parent_id, [])
2206 elif name == "name.~%s~" % child_id:2207
2207 return True2208 class MockEntry(object):
2208 return False2209
22092210 def __init__(self):
22102211 object.__init__(self)
2211class MockEntry(object):2212 self.name = "name"
2212 def __init__(self):2213
2213 object.__init__(self)
2214 self.name = "name"
2215
2216
2217class TestGetBackupName(TestCase):
2218 def test_get_backup_name(self):
2219 tt = MockTransform()2214 tt = MockTransform()
2220 name = get_backup_name(MockEntry(), {'a':[]}, 'a', tt)2215 name1 = self.applyDeprecated(
2221 self.assertEqual(name, 'name.~1~')2216 symbol_versioning.deprecated_in((2, 3, 0)),
2222 name = get_backup_name(MockEntry(), {'a':['1']}, 'a', tt)2217 transform.get_backup_name, MockEntry(), {'a':[]}, 'a', tt)
2223 self.assertEqual(name, 'name.~2~')2218 self.assertEqual('name.~1~', name1)
2224 name = get_backup_name(MockEntry(), {'a':['2']}, 'a', tt)2219 name2 = self.applyDeprecated(
2225 self.assertEqual(name, 'name.~1~')2220 symbol_versioning.deprecated_in((2, 3, 0)),
2226 name = get_backup_name(MockEntry(), {'a':['2'], 'b':[]}, 'b', tt)2221 transform._get_backup_name, 'name', {'a':['name.~1~']}, 'a', tt)
2227 self.assertEqual(name, 'name.~1~')2222 self.assertEqual('name.~2~', name2)
2228 name = get_backup_name(MockEntry(), {'a':['1', '2', '3']}, 'a', tt)
2229 self.assertEqual(name, 'name.~4~')
22302223
22312224
2232class TestFileMover(tests.TestCaseWithTransport):2225class TestFileMover(tests.TestCaseWithTransport):
22332226
=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py 2010-10-15 14:15:59 +0000
+++ bzrlib/transform.py 2010-10-15 14:16:00 +0000
@@ -59,6 +59,7 @@
59from bzrlib.symbol_versioning import (59from bzrlib.symbol_versioning import (
60 deprecated_function,60 deprecated_function,
61 deprecated_in,61 deprecated_in,
62 deprecated_method,
62 )63 )
63from bzrlib.trace import mutter, warning64from bzrlib.trace import mutter, warning
64from bzrlib import tree65from bzrlib import tree
@@ -534,30 +535,53 @@
534 # ensure that all children are registered with the transaction535 # ensure that all children are registered with the transaction
535 list(self.iter_tree_children(parent_id))536 list(self.iter_tree_children(parent_id))
536537
538 @deprecated_method(deprecated_in((2, 3, 0)))
537 def has_named_child(self, by_parent, parent_id, name):539 def has_named_child(self, by_parent, parent_id, name):
538 try:540 return self._has_named_child(
539 children = by_parent[parent_id]541 name, parent_id, known_children=by_parent.get(parent_id, []))
540 except KeyError:542
541 children = []543 def _has_named_child(self, name, parent_id, known_children):
542 for child in children:544 """Does a parent already have a name child.
545
546 :param name: The searched for name.
547
548 :param parent_id: The parent for which the check is made.
549
550 :param known_children: The already known children. This should have
551 been recently obtained from `self.by_parent.get(parent_id)`
552 (or will be if None is passed).
553 """
554 if known_children is None:
555 known_children = self.by_parent().get(parent_id, [])
556 for child in known_children:
543 if self.final_name(child) == name:557 if self.final_name(child) == name:
544 return True558 return True
545 try:559 parent_path = self._tree_id_paths.get(parent_id, None)
546 path = self._tree_id_paths[parent_id]560 if parent_path is None:
547 except KeyError:561 # No parent... no children
548 return False562 return False
549 childpath = joinpath(path, name)563 child_path = joinpath(parent_path, name)
550 child_id = self._tree_path_ids.get(childpath)564 child_id = self._tree_path_ids.get(child_path, None)
551 if child_id is None:565 if child_id is None:
552 return lexists(self._tree.abspath(childpath))566 # Not known by the tree transform yet, check the filesystem
567 return osutils.lexists(self._tree.abspath(child_path))
553 else:568 else:
554 if self.final_parent(child_id) != parent_id:569 raise AssertionError('child_id is missing: %s, %s, %s'
555 return False570 % (name, parent_id, child_id))
556 if child_id in self._removed_contents:571
557 # XXX What about dangling file-ids?572 def _available_backup_name(self, name, target_id):
558 return False573 """Find an available backup name.
559 else:574
560 return True575 :param name: The basename of the file.
576
577 :param target_id: The directory trans_id where the backup should
578 be placed.
579 """
580 known_children = self.by_parent().get(target_id, [])
581 return osutils.available_backup_name(
582 name,
583 lambda base: self._has_named_child(
584 base, target_id, known_children))
561585
562 def _parent_loops(self):586 def _parent_loops(self):
563 """No entry should be its own ancestor"""587 """No entry should be its own ancestor"""
@@ -1295,7 +1319,7 @@
1295 parent_path = self._tree_id_paths[parent_id]1319 parent_path = self._tree_id_paths[parent_id]
1296 # Find a name that doesn't exist yet in the orphan dir1320 # Find a name that doesn't exist yet in the orphan dir
1297 actual_name = self.final_name(trans_id)1321 actual_name = self.final_name(trans_id)
1298 new_name = _get_backup_name(actual_name, self.by_parent(), od_id, self)1322 new_name = self._available_backup_name(actual_name, od_id)
1299 self.adjust_path(new_name, od_id, trans_id)1323 self.adjust_path(new_name, od_id, trans_id)
1300 trace.warning('%s has been orphaned in %s'1324 trace.warning('%s has been orphaned in %s'
1301 % (joinpath(parent_path, actual_name), orphan_dir))1325 % (joinpath(parent_path, actual_name), orphan_dir))
@@ -2572,10 +2596,12 @@
2572 tt.set_executability(entry.executable, trans_id)2596 tt.set_executability(entry.executable, trans_id)
25732597
25742598
2599@deprecated_function(deprecated_in((2, 3, 0)))
2575def get_backup_name(entry, by_parent, parent_trans_id, tt):2600def get_backup_name(entry, by_parent, parent_trans_id, tt):
2576 return _get_backup_name(entry.name, by_parent, parent_trans_id, tt)2601 return _get_backup_name(entry.name, by_parent, parent_trans_id, tt)
25772602
25782603
2604@deprecated_function(deprecated_in((2, 3, 0)))
2579def _get_backup_name(name, by_parent, parent_trans_id, tt):2605def _get_backup_name(name, by_parent, parent_trans_id, tt):
2580 """Produce a backup-style name that appears to be available"""2606 """Produce a backup-style name that appears to be available"""
2581 def name_gen():2607 def name_gen():
@@ -2702,9 +2728,8 @@
2702 tt.delete_contents(trans_id)2728 tt.delete_contents(trans_id)
2703 elif kind[1] is not None:2729 elif kind[1] is not None:
2704 parent_trans_id = tt.trans_id_file_id(parent[0])2730 parent_trans_id = tt.trans_id_file_id(parent[0])
2705 by_parent = tt.by_parent()2731 backup_name = tt._available_backup_name(
2706 backup_name = _get_backup_name(name[0], by_parent,2732 name[0], parent_trans_id)
2707 parent_trans_id, tt)
2708 tt.adjust_path(backup_name, parent_trans_id, trans_id)2733 tt.adjust_path(backup_name, parent_trans_id, trans_id)
2709 new_trans_id = tt.create_path(name[0], parent_trans_id)2734 new_trans_id = tt.create_path(name[0], parent_trans_id)
2710 if versioned == (True, True):2735 if versioned == (True, True):
27112736
=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt 2010-10-15 14:15:59 +0000
+++ doc/en/release-notes/bzr-2.3.txt 2010-10-15 14:16:00 +0000
@@ -105,7 +105,11 @@
105105
106* ``BzrDir.generate_backup_name`` has been deprecated and replaced by a106* ``BzrDir.generate_backup_name`` has been deprecated and replaced by a
107 private method. ``osutils.available_backup_name`` provides an extensible107 private method. ``osutils.available_backup_name`` provides an extensible
108 replacement. (Vincent Ladeuil)108 replacement. This allowed the deprecation of
109 ``bzrlib.transform.get_backup_name``,
110 ``bzrlib.transform._get_backup_name`` and
111 ``bzrlib.transform.TreeTransformBase.has_named_child``.
112 (Vincent Ladeuil)
109113
110New Features114New Features
111************115************