Rev 4634: Fix #531967 by creating helpers for PathConflicts when a deletion in file:///home/vila/src/bzr/bugs/531967-unify-name-conflicts/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Tue Mar 9 15:24:27 GMT 2010
At file:///home/vila/src/bzr/bugs/531967-unify-name-conflicts/
------------------------------------------------------------
revno: 4634
revision-id: v.ladeuil+lp at free.fr-20100309152427-kek9vs3hcmdmlu6s
parent: v.ladeuil+lp at free.fr-20100308133702-uyijbgeti22wgqu0
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 531967-unify-name-conflicts
timestamp: Tue 2010-03-09 16:24:27 +0100
message:
Fix #531967 by creating helpers for PathConflicts when a deletion
is involved.
* bzrlib/tests/test_conflicts.py:
(TestParametrizedResolveConflicts.mirror_scenarios): Renamed from
multiply_scenarios to make the intent clearer. Turned into a
classmethod too for the same reason.
(TestParametrizedResolveConflicts.scenarios): Now a classmethod.
* bzrlib/merge.py:
(Merge3Merger._merge_names): 'name conflict' and 'parent conflict'
can (and must) be handled in the same way. If a deletion is
involved we create an unversioned copy of the rejected item so the
user can restore that easily.
(Merge3Merger.cook_conflicts): Get rid of 'name conflict', 'parent
conflict' distinction and just create PathConflicts with a file_id
to address bug #531967.
* bzrlib/conflicts.py:
(PathConflict.associated_filenames): Helpers exist only when a
deletion is involved.
(PathConflict._resolve): We may have to version one path
again. This may happen when a deletion have occurred.
(PathConflict.action_take_this, PathConflict.action_take_other):
As a special case, we may have an helper to use when deletion was
involved.
-------------- next part --------------
=== modified file 'NEWS'
--- a/NEWS 2010-03-02 10:21:39 +0000
+++ b/NEWS 2010-03-09 15:24:27 +0000
@@ -92,6 +92,10 @@
ftp servers while trying to take a lock.
(Martin Pool, #528722)
+* Path conflicts now support --take-this and --take-other even when a
+ deletion is involved.
+ (Vincent Ladeuil, #531967)
+
* Network transfer amounts and rates are now displayed in SI units according
to the Ubuntu Units Policy <https://wiki.ubuntu.com/UnitsPolicy>.
(Gordon Tyler, #514399)
=== modified file 'bzrlib/conflicts.py'
--- a/bzrlib/conflicts.py 2010-03-08 09:31:12 +0000
+++ b/bzrlib/conflicts.py 2010-03-09 15:24:27 +0000
@@ -463,16 +463,30 @@
return s
def associated_filenames(self):
- # No additional files have been generated here
- return []
+ if self.file_id is None:
+ return []
+ else:
+ # FIXME: This may be too precise. associated_filenames() is really
+ # about *potentially* existing files, so we may just avoid the
+ # tests against <deleted>
+ if self.path == '<deleted>':
+ return [self.conflict_path + '.OTHER']
+ elif self.conflict_path == '<deleted>':
+ return [self.path + '.THIS']
+ else:
+ return []
- def _resolve(self, tt, file_id, path):
+ def _resolve(self, tt, file_id, path, helper_path=None):
"""Resolve the conflict.
:param tt: The TreeTransform where the conflict is resolved.
:param file_id: The retained file id.
:param path: The retained path.
+ :param helper_path: The existing but unversioned path that needs to be
+ restored.
"""
+ if helper_path is not None:
+ tt.version_file(file_id, tt.trans_id_tree_path(helper_path))
# Adjust the path for the retained file id
tid = tt.trans_id_file_id(file_id)
parent_tid = tt.get_tree_parent(tid)
@@ -514,7 +528,15 @@
def action_take_this(self, tree):
if self.file_id is not None:
- self._resolve_with_cleanups(tree, self.file_id, self.path)
+ if self.path == '<deleted>':
+ # Nothing to do
+ return
+ if self.conflict_path == '<deleted>':
+ helper_path = self.path + '.THIS'
+ else:
+ helper_path = None
+ self._resolve_with_cleanups(tree, self.file_id, self.path,
+ helper_path=helper_path)
else:
# Prior to bug #531967 we need to find back the file_id and restore
# the content from there
@@ -524,8 +546,16 @@
def action_take_other(self, tree):
if self.file_id is not None:
- # just acccept bzr proposal
- pass
+ if self.conflict_path == '<deleted>':
+ # Nothing to do
+ return
+ if self.path == '<deleted>':
+ helper_path = self.conflict_path + '.OTHER'
+ else:
+ helper_path = None
+ self._resolve_with_cleanups(tree, self.file_id,
+ self.conflict_path,
+ helper_path=helper_path)
else:
# Prior to bug #531967 we need to find back the file_id and restore
# the content from there
=== modified file 'bzrlib/merge.py'
--- a/bzrlib/merge.py 2010-03-04 20:53:46 +0000
+++ b/bzrlib/merge.py 2010-03-09 15:24:27 +0000
@@ -1173,6 +1173,9 @@
return 'conflict'
@staticmethod
+ # FIXME: this looks unused and probably needs to be deprecated, the
+ # parameter order (this, base, other) doesn't match the other methods
+ # (base, other, this) anyway -- vila 20100308
def scalar_three_way(this_tree, base_tree, other_tree, file_id, key):
"""Do a three-way test on a scalar.
Return "this", "other" or "conflict", depending whether a value wins.
@@ -1228,25 +1231,34 @@
parent_id_winner = "other"
if name_winner == "this" and parent_id_winner == "this":
return
- if name_winner == "conflict":
- trans_id = self.tt.trans_id_file_id(file_id)
- self._raw_conflicts.append(('name conflict', trans_id,
- this_name, other_name))
- if parent_id_winner == "conflict":
- trans_id = self.tt.trans_id_file_id(file_id)
- self._raw_conflicts.append(('parent conflict', trans_id,
- this_parent, other_parent))
+ if name_winner == 'conflict' or parent_id_winner == 'conflict':
+ if other_name is None or other_parent is None:
+ # 'other' has been deleted, leave a .THIS
+ parent_id = self.tt.trans_id_file_id(this_parent)
+ trans_id = self.tt.create_path(this_name + '.THIS', parent_id)
+ transform.create_from_tree(self.tt, trans_id, self.this_tree,
+ file_id)
+ elif this_name is None or this_parent is None:
+ # 'this' has been deleted, leave a .OTHER
+ parent_id = self.tt.trans_id_file_id(other_parent)
+ trans_id = self.tt.create_path(other_name + '.OTHER', parent_id)
+ transform.create_from_tree(self.tt, trans_id, self.other_tree,
+ file_id)
+ trans_id = self.tt.trans_id_file_id(file_id)
+ self._raw_conflicts.append(('path conflict', trans_id, file_id,
+ this_parent, this_name,
+ other_parent, other_name))
if other_name is None:
# it doesn't matter whether the result was 'other' or
# 'conflict'-- if there's no 'other', we leave it alone.
return
- # if we get here, name_winner and parent_winner are set to safe values.
- trans_id = self.tt.trans_id_file_id(file_id)
parent_id = parents[self.winner_idx[parent_id_winner]]
if parent_id is not None:
- parent_trans_id = self.tt.trans_id_file_id(parent_id)
+ # if we get here, name_winner and parent_winner are set to safe
+ # values.
self.tt.adjust_path(names[self.winner_idx[name_winner]],
- parent_trans_id, trans_id)
+ self.tt.trans_id_file_id(parent_id),
+ self.tt.trans_id_file_id(file_id))
def _do_merge_contents(self, file_id):
"""Performs a merge on file_id contents."""
@@ -1531,20 +1543,32 @@
def cook_conflicts(self, fs_conflicts):
"""Convert all conflicts into a form that doesn't depend on trans_id"""
- name_conflicts = {}
self.cooked_conflicts.extend(transform.cook_conflicts(
fs_conflicts, self.tt))
fp = transform.FinalPaths(self.tt)
for conflict in self._raw_conflicts:
conflict_type = conflict[0]
- if conflict_type in ('name conflict', 'parent conflict'):
- trans_id = conflict[1]
- conflict_args = conflict[2:]
- if trans_id not in name_conflicts:
- name_conflicts[trans_id] = {}
- transform.unique_add(name_conflicts[trans_id], conflict_type,
- conflict_args)
- if conflict_type == 'contents conflict':
+ if conflict_type == 'path conflict':
+ (trans_id, file_id,
+ this_parent, this_name,
+ other_parent, other_name) = conflict[1:]
+ if this_parent is None or this_name is None:
+ this_path = '<deleted>'
+ else:
+ parent_path = fp.get_path(
+ self.tt.trans_id_file_id(this_parent))
+ this_path = osutils.pathjoin(parent_path, this_name)
+ if other_parent is None or other_name is None:
+ other_path = '<deleted>'
+ else:
+ parent_path = fp.get_path(
+ self.tt.trans_id_file_id(other_parent))
+ other_path = osutils.pathjoin(parent_path, other_name)
+ c = _mod_conflicts.Conflict.factory(
+ 'path conflict', path=this_path,
+ conflict_path=other_path,
+ file_id=file_id)
+ elif conflict_type == 'contents conflict':
for trans_id in conflict[1]:
file_id = self.tt.final_file_id(trans_id)
if file_id is not None:
@@ -1556,43 +1580,16 @@
break
c = _mod_conflicts.Conflict.factory(conflict_type,
path=path, file_id=file_id)
- self.cooked_conflicts.append(c)
- if conflict_type == 'text conflict':
+ elif conflict_type == 'text conflict':
trans_id = conflict[1]
path = fp.get_path(trans_id)
file_id = self.tt.final_file_id(trans_id)
c = _mod_conflicts.Conflict.factory(conflict_type,
path=path, file_id=file_id)
- self.cooked_conflicts.append(c)
-
- for trans_id, conflicts in name_conflicts.iteritems():
- try:
- this_parent, other_parent = conflicts['parent conflict']
- if this_parent == other_parent:
- raise AssertionError()
- except KeyError:
- this_parent = other_parent = \
- self.tt.final_file_id(self.tt.final_parent(trans_id))
- try:
- this_name, other_name = conflicts['name conflict']
- if this_name == other_name:
- raise AssertionError()
- except KeyError:
- this_name = other_name = self.tt.final_name(trans_id)
- other_path = fp.get_path(trans_id)
- if this_parent is not None and this_name is not None:
- this_parent_path = \
- fp.get_path(self.tt.trans_id_file_id(this_parent))
- this_path = osutils.pathjoin(this_parent_path, this_name)
else:
- this_path = "<deleted>"
- file_id = self.tt.final_file_id(trans_id)
- # FIXME: PathConflicts objects are created with other/this
- # path/conflict_path paths reversed -- vila 20100304
- c = _mod_conflicts.Conflict.factory('path conflict', path=this_path,
- conflict_path=other_path,
- file_id=file_id)
+ raise AssertionError()
self.cooked_conflicts.append(c)
+
self.cooked_conflicts.sort(key=_mod_conflicts.Conflict.sort_key)
=== modified file 'bzrlib/tests/test_conflicts.py'
--- a/bzrlib/tests/test_conflicts.py 2010-03-08 13:37:02 +0000
+++ b/bzrlib/tests/test_conflicts.py 2010-03-09 15:24:27 +0000
@@ -198,6 +198,8 @@
# FIXME: The shell-like tests should be converted to real whitebox tests... or
# moved to a blackbox module -- vila 20100205
+# FIXME: test missing for multiple conflicts
+
# FIXME: Tests missing for DuplicateID conflict type
class TestResolveConflicts(script.TestCaseWithTransportAndScript):
@@ -231,7 +233,8 @@
_other_path = None
_other_id = None
- def multiply_scenarios(self, base_scenarios, common_params):
+ @classmethod
+ def mirror_scenarios(klass, base_scenarios, common_params):
scenarios = []
def adapt(d, side):
"""Modify dict to apply to the given side.
@@ -246,18 +249,22 @@
return t
# Each base scenario is duplicated switching the roles of 'this' and
# 'other'
- scenarios.extend(tests.multiply_scenarios(
- [(name, adapt(d, 'this')) for (name, d), r in base_scenarios],
- [(name, adapt(d, 'other')) for l, (name, d) in base_scenarios]))
- scenarios.extend(tests.multiply_scenarios(
- [(name, adapt(d, 'other')) for (name, d), r in base_scenarios],
- [(name, adapt(d, 'this')) for l, (name, d) in base_scenarios]))
+ left = [l for l,r in base_scenarios]
+ right = [r for l,r in base_scenarios]
+ for (lname, ldict), (rname, rdict) in zip(left, right):
+ scenarios.extend(tests.multiply_scenarios(
+ [(lname, adapt(ldict, 'this'))],
+ [(rname, adapt(rdict, 'other'))]))
+ scenarios.extend(tests.multiply_scenarios(
+ [(rname, adapt(rdict, 'this'))],
+ [(lname, adapt(ldict, 'other'))]))
# Inject the common parameters in all scenarios
for name, d in scenarios:
d.update(common_params)
return scenarios
- def scenarios(self):
+ @classmethod
+ def scenarios(klass):
# Only concrete classes return actual scenarios
return []
@@ -371,7 +378,8 @@
class TestResolveContentsConflict(TestParametrizedResolveConflicts):
- def scenarios(self):
+ @classmethod
+ def scenarios(klass):
base_scenarios = [
(('file_modified', dict(actions='modify_file',
check='file_has_more_content')),
@@ -383,7 +391,7 @@
_assert_conflict='assertContentsConflict',
_item_path='file', item_id='file-id',
)
- return self.multiply_scenarios(base_scenarios, common)
+ return klass.mirror_scenarios(base_scenarios, common)
def assertContentsConflict(self, c):
self.assertEqual(self._other_id, c.file_id)
@@ -392,7 +400,8 @@
class TestResolvePathConflict(TestParametrizedResolveConflicts):
- def scenarios(self):
+ @classmethod
+ def scenarios(klass):
base_scenarios = [
(('dir_renamed', dict(actions='rename_dir', check='dir_renamed')),
('dir_deleted', dict(actions='delete_dir', check='dir_doesnt_exist'))),
@@ -403,15 +412,10 @@
_assert_conflict='assert_PathConflict',
_actions_base='create_dir',
_item_path='new-dir', _item_id='dir-id',)
- return self.multiply_scenarios(base_scenarios, common)
+ return klass.mirror_scenarios(base_scenarios, common)
def assert_PathConflict(self, c):
- # bug #531967 is about file_id not being set in some cases
self.assertEqual(self._item_id, c.file_id)
- # FIXME: PathConflicts objects are created with other/this
- # path/conflict_path paths reversed -- vila 20100304
- # self.assertEqual(self._other_path, c.path)
- # self.assertEqual(self._this_path, c.conflict_path)
self.assertEqual(self._this_path, c.path)
self.assertEqual(self._other_path, c.conflict_path)
@@ -420,6 +424,9 @@
"""Same as TestResolvePathConflict but a specific conflict object.
"""
+ # FIXME: Now that bug #531697 is fixed, we need to inject a conflict object
+ # as it existed before the fix.
+
def assert_PathConflict(self, c):
# bug #531967 is about file_id not being set in some cases
self.assertIs(None, c.file_id)
@@ -428,7 +435,6 @@
self.assertEqual(self._item_path, c.conflict_path)
-
class TestResolveDuplicateEntry(TestResolveConflicts):
preamble = """
More information about the bazaar-commits
mailing list