Rev 4630: merge fix for 531967 in file:///home/vila/src/bzr/experimental/conflict-manager/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Wed Mar 10 13:24:34 GMT 2010
At file:///home/vila/src/bzr/experimental/conflict-manager/
------------------------------------------------------------
revno: 4630 [merge]
revision-id: v.ladeuil+lp at free.fr-20100310132434-3va4vvdvzh2pyicu
parent: v.ladeuil+lp at free.fr-20100310132301-cdjj64niv48yjc7u
parent: v.ladeuil+lp at free.fr-20100310110521-008vduq0zk81vwep
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 531967-unify-name-conflicts
timestamp: Wed 2010-03-10 14:24:34 +0100
message:
merge fix for 531967
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
bzrlib/conflicts.py conflicts.py-20051001061850-78ef952ba63d2b42
bzrlib/merge.py merge.py-20050513021216-953b65a438527106
bzrlib/tests/test_conflicts.py test_conflicts.py-20051006031059-e2dad9bbeaa5891f
-------------- 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-10 13:23:01 +0000
+++ b/bzrlib/conflicts.py 2010-03-10 13:24:34 +0000
@@ -436,6 +436,12 @@
def action_take_other(self, tree):
raise NotImplementedError(self.action_take_other)
+ def _resolve_with_cleanups(self, tree, *args, **kwargs):
+ tt = transform.TreeTransform(tree)
+ op = cleanup.OperationWithCleanups(self._resolve)
+ op.add_cleanup(tt.finalize)
+ op.run_simple(tt, *args, **kwargs)
+
class PathConflict(Conflict):
"""A conflict was encountered merging file paths"""
@@ -460,12 +466,89 @@
# No additional files have been generated here
return []
+ def _resolve(self, tt, file_id, path, winner):
+ """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 winner: 'this' or 'other' indicates which side is the winner.
+ """
+ path_to_create = None
+ if winner == 'this':
+ if self.path == '<deleted>':
+ return # Nothing to do
+ if self.conflict_path == '<deleted>':
+ path_to_create = self.path
+ revid = tt._tree.get_parent_ids()[0]
+ elif winner == 'other':
+ if self.conflict_path == '<deleted>':
+ return # Nothing to do
+ if self.path == '<deleted>':
+ path_to_create = self.conflict_path
+ # FIXME: If there are more than two parents we may need to
+ # iterate. Taking the last parent is the safer bet in the mean
+ # time. -- vila 20100309
+ revid = tt._tree.get_parent_ids()[-1]
+ if path_to_create is not None:
+ tid = tt.trans_id_tree_path(path_to_create)
+ transform.create_from_tree(
+ tt, tt.trans_id_tree_path(path_to_create),
+ self._revision_tree(tt._tree, revid), file_id)
+ tt.version_file(file_id, tid)
+
+ # Adjust the path for the retained file id
+ tid = tt.trans_id_file_id(file_id)
+ parent_tid = tt.get_tree_parent(tid)
+ tt.adjust_path(path, parent_tid, tid)
+ tt.apply()
+
+ def _revision_tree(self, tree, revid):
+ return tree.branch.repository.revision_tree(revid)
+
+ def _infer_file_id(self, tree):
+ # Prior to bug #531967, file_id wasn't always set, there may still be
+ # conflict files in the wild so we need to cope with them
+ # Establish which path we should use to find back the file-id
+ possible_paths = []
+ for p in (self.path, self.conflict_path):
+ if p == '<deleted>':
+ # special hard-coded path
+ continue
+ if p is not None:
+ possible_paths.append(p)
+ # Search the file-id in the parents with any path available
+ file_id = None
+ for revid in tree.get_parent_ids():
+ revtree = self._revision_tree(tree, revid)
+ for p in possible_paths:
+ file_id = revtree.path2id(p)
+ if file_id is not None:
+ return revtree, file_id
+ return None, None
+
def action_take_this(self, tree):
- tree.rename_one(self.conflict_path, self.path)
+ if self.file_id is not None:
+ self._resolve_with_cleanups(tree, self.file_id, self.path,
+ winner='this')
+ else:
+ # Prior to bug #531967 we need to find back the file_id and restore
+ # the content from there
+ revtree, file_id = self._infer_file_id(tree)
+ tree.revert([revtree.id2path(file_id)],
+ old_tree=revtree, backups=False)
def action_take_other(self, tree):
- # just acccept bzr proposal
- pass
+ if self.file_id is not None:
+ self._resolve_with_cleanups(tree, self.file_id,
+ self.conflict_path,
+ winner='other')
+ else:
+ # Prior to bug #531967 we need to find back the file_id and restore
+ # the content from there
+ revtree, file_id = self._infer_file_id(tree)
+ tree.revert([revtree.id2path(file_id)],
+ old_tree=revtree, backups=False)
class ContentsConflict(PathConflict):
@@ -480,7 +563,7 @@
def associated_filenames(self):
return [self.path + suffix for suffix in ('.BASE', '.OTHER')]
- def _take_it(self, tt, suffix_to_remove):
+ def _resolve(self, tt, suffix_to_remove):
"""Resolve the conflict.
:param tt: The TreeTransform where the conflict is resolved.
@@ -506,17 +589,11 @@
tt.adjust_path(self.path, parent_tid, this_tid)
tt.apply()
- def _take_it_with_cleanups(self, tree, suffix_to_remove):
- tt = transform.TreeTransform(tree)
- op = cleanup.OperationWithCleanups(self._take_it)
- op.add_cleanup(tt.finalize)
- op.run_simple(tt, suffix_to_remove)
-
def action_take_this(self, tree):
- self._take_it_with_cleanups(tree, 'OTHER')
+ self._resolve_with_cleanups(tree, 'OTHER')
def action_take_other(self, tree):
- self._take_it_with_cleanups(tree, 'THIS')
+ self._resolve_with_cleanups(tree, 'THIS')
# FIXME: TextConflict is about a single file-id, there never is a conflict_path
=== modified file 'bzrlib/merge.py'
--- a/bzrlib/merge.py 2010-02-23 07:43:11 +0000
+++ b/bzrlib/merge.py 2010-03-10 09:44:36 +0000
@@ -1228,25 +1228,24 @@
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':
+ # Creating helpers here cause problems down the road if a
+ # ContentConflict needs to be created so we should not do that
+ 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 +1530,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,41 +1567,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)
- 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-04 09:37:23 +0000
+++ b/bzrlib/tests/test_conflicts.py 2010-03-10 11:05:21 +0000
@@ -36,7 +36,11 @@
standard_tests, tests.condition_isinstance((
TestParametrizedResolveConflicts,
)))
- tests.multiply_tests(sp_tests, resolve_conflict_scenarios(), result)
+ # Each test class defines its own scenarios. This is needed for
+ # TestResolvePathConflictBefore531967 that verifies that the same tests as
+ # TestResolvePathConflict still pass.
+ for test in tests.iter_suite_tests(sp_tests):
+ tests.apply_scenarios(test, test.scenarios(), result)
# No parametrization for the remaining tests
result.addTests(remaining_tests)
@@ -194,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):
@@ -209,61 +215,84 @@
pass
-def resolve_conflict_scenarios():
- base_scenarios = [
- (dict(_conflict_type=conflicts.ContentsConflict,
- _item_path='file', _item_id='file-id',),
- ('file_modified', dict(actions='modify_file',
- check='file_has_more_content')),
- ('file_deleted', dict(actions='delete_file',
- check='file_doesnt_exist'))),
- (dict(_conflict_type=conflicts.PathConflict,
- _item_path='new-dir', _item_id='dir-id',),
- ('dir_renamed', dict(actions='rename_dir', check='dir_renamed')),
- ('dir_deleted', dict(actions='delete_dir', check='dir_doesnt_exist'))),
- ]
- # Each base scenario is duplicated switching the roles of this and other
- scenarios = []
- for common, (tname, tdict), (oname, odict) in base_scenarios:
- d = common.copy()
- d.update(_this_actions=tdict['actions'], _check_this=tdict['check'],
- _other_actions=odict['actions'], _check_other=odict['check'])
- scenarios.append(('%s,%s' % (tname, oname), d))
- d = common.copy()
- d.update(_this_actions=odict['actions'], _check_this=odict['check'],
- _other_actions=tdict['actions'], _check_other=tdict['check'])
- scenarios.append(('%s,%s' % (oname, tname), d))
- return scenarios
-
-# FIXME: Get rid of parametrized once we delete TestResolveConflicts
+# FIXME: Get rid of parametrized (in the class name) once we delete
+# TestResolveConflicts -- vila 20100308
class TestParametrizedResolveConflicts(tests.TestCaseWithTransport):
+ # Set by daughter classes
+ _conflict_type = None
+ _assert_conflict = None
+
# Set by load_tests
+ _base_actions = None
_this_actions = None
_other_actions = None
- _conflict_type = None
_item_path = None
_item_id = None
+ # Set by _this_actions and other_actions
+ _this_path = None
+ _this_id = None
+ _other_path = None
+ _other_id = None
+
+ @classmethod
+ def mirror_scenarios(klass, base_scenarios):
+ scenarios = []
+ def adapt(d, side):
+ """Modify dict to apply to the given side.
+
+ 'actions' key is turned into '_actions_this' if side is 'this' for
+ example.
+ """
+ t = {}
+ # Turn each key into _side_key
+ for k,v in d.iteritems():
+ t['_%s_%s' % (k, side)] = v
+ return t
+ # Each base scenario is duplicated switching the roles of 'this' and
+ # 'other'
+ left = [l for l, r, c in base_scenarios]
+ right = [r for l, r, c in base_scenarios]
+ common = [c for l, r, c in base_scenarios]
+ for (lname, ldict), (rname, rdict), common in zip(left, right, common):
+ a = tests.multiply_scenarios([(lname, adapt(ldict, 'this'))],
+ [(rname, adapt(rdict, 'other'))])
+ b = tests.multiply_scenarios(
+ [(rname, adapt(rdict, 'this'))],
+ [(lname, adapt(ldict, 'other'))])
+ # Inject the common parameters in all scenarios
+ for name, d in a + b:
+ d.update(common)
+ scenarios.extend(a + b)
+ return scenarios
+
+ @classmethod
+ def scenarios(klass):
+ # Only concrete classes return actual scenarios
+ return []
+
def setUp(self):
super(TestParametrizedResolveConflicts, self).setUp()
builder = self.make_branch_builder('trunk')
builder.start_series()
+
# Create an empty trunk
builder.build_snapshot('start', None, [
('add', ('', 'root-id', 'directory', ''))])
# Add a minimal base content
- builder.build_snapshot(
- 'base', ['start'], [
- ('add', ('file', 'file-id', 'file', 'trunk content\n')),
- ('add', ('dir', 'dir-id', 'directory', '')),
- ])
+ _, _, actions_base = self._get_actions(self._actions_base)()
+ builder.build_snapshot('base', ['start'], actions_base)
# Modify the base content in branch
- other_actions = self._get_actions(self._other_actions)
- builder.build_snapshot('other', ['base'], other_actions())
+ (self._other_path, self._other_id,
+ actions_other) = self._get_actions(self._actions_other)()
+ builder.build_snapshot('other', ['base'], actions_other)
# Modify the base content in trunk
- this_actions = self._get_actions(self._this_actions)
- builder.build_snapshot('this', ['base'], this_actions())
+ (self._this_path, self._this_id,
+ actions_this) = self._get_actions(self._actions_this)()
+ builder.build_snapshot('this', ['base'], actions_this)
+ # builder.get_branch() tip is now 'this'
+
builder.finish_series()
self.builder = builder
@@ -273,42 +302,62 @@
def _get_check(self, name):
return getattr(self, 'check_%s' % name)
- def assertConflict(self, wt, **kwargs):
+ def assertConflict(self, wt):
confs = wt.conflicts()
self.assertLength(1, confs)
c = confs[0]
self.assertIsInstance(c, self._conflict_type)
- sentinel = object() # An impossible value
- for k, v in kwargs.iteritems():
- self.assertEqual(v, getattr(c, k, sentinel), "for key '%s'" % k)
+ self._assert_conflict(wt, c)
- def check_resolved(self, wt, item, action):
- conflicts.resolve(wt, [item], action=action)
+ def check_resolved(self, wt, path, action):
+ conflicts.resolve(wt, [path], action=action)
# Check that we don't have any conflicts nor unknown left
self.assertLength(0, wt.conflicts())
self.assertLength(0, list(wt.unknowns()))
+ def do_create_file(self):
+ return ('file', 'file-id',
+ [('add', ('file', 'file-id', 'file', 'trunk content\n'))])
+
+ def do_create_dir(self):
+ return ('dir', 'dir-id', [('add', ('dir', 'dir-id', 'directory', ''))])
+
def do_modify_file(self):
- return [('modify', ('file-id', 'trunk content\nmore content\n'))]
+ return ('file', 'file-id',
+ [('modify', ('file-id', 'trunk content\nmore content\n'))])
def check_file_has_more_content(self):
self.assertFileEqual('trunk content\nmore content\n', 'branch/file')
def do_delete_file(self):
- return [('unversion', 'file-id')]
+ return ('file', 'file-id', [('unversion', 'file-id')])
def check_file_doesnt_exist(self):
self.failIfExists('branch/file')
+ def do_rename_file(self):
+ return ('new-file', 'file-id', [('rename', ('file', 'new-file'))])
+
+ def check_file_renamed(self):
+ self.failIfExists('branch/file')
+ self.failUnlessExists('branch/new-file')
+
def do_rename_dir(self):
- return [('rename', ('dir', 'new-dir'))]
+ return ('new-dir', 'dir-id', [('rename', ('dir', 'new-dir'))])
def check_dir_renamed(self):
self.failIfExists('branch/dir')
self.failUnlessExists('branch/new-dir')
+ def do_rename_dir2(self):
+ return ('new-dir2', 'dir-id', [('rename', ('dir', 'new-dir2'))])
+
+ def check_dir_renamed2(self):
+ self.failIfExists('branch/dir')
+ self.failUnlessExists('branch/new-dir2')
+
def do_delete_dir(self):
- return [('unversion', 'dir-id')]
+ return ('<deleted>', 'dir-id', [('unversion', 'dir-id')])
def check_dir_doesnt_exist(self):
self.failIfExists('branch/dir')
@@ -321,19 +370,98 @@
def test_resolve_taking_this(self):
wt = self._merge_other_into_this()
- self.assertConflict(wt, path=self._item_path, file_id=self._item_id)
+ self.assertConflict(wt)
self.check_resolved(wt, self._item_path, 'take_this')
check_this = self._get_check(self._check_this)
check_this()
def test_resolve_taking_other(self):
wt = self._merge_other_into_this()
- self.assertConflict(wt, path=self._item_path, file_id=self._item_id)
+ self.assertConflict(wt)
self.check_resolved(wt, self._item_path, 'take_other')
check_other = self._get_check(self._check_other)
check_other()
+class TestResolveContentsConflict(TestParametrizedResolveConflicts):
+
+ _conflict_type = conflicts.ContentsConflict,
+ @classmethod
+ def scenarios(klass):
+ common = dict(_actions_base='create_file',
+ _item_path='file', item_id='file-id',
+ )
+ base_scenarios = [
+ (('file_modified', dict(actions='modify_file',
+ check='file_has_more_content')),
+ ('file_deleted', dict(actions='delete_file',
+ check='file_doesnt_exist')),
+ dict(_actions_base='create_file',
+ _item_path='file', item_id='file-id',)),
+ ]
+ return klass.mirror_scenarios(base_scenarios)
+
+ def assertContentsConflict(self, wt, c):
+ self.assertEqual(self._other_id, c.file_id)
+ self.assertEqual(self._other_path, c.path)
+ _assert_conflict = assertContentsConflict
+
+
+
+class TestResolvePathConflict(TestParametrizedResolveConflicts):
+
+ _conflict_type = conflicts.PathConflict,
+
+ @classmethod
+ def scenarios(klass):
+ for_dirs = dict(_actions_base='create_dir',
+ _item_path='new-dir', _item_id='dir-id',)
+ base_scenarios = [
+ (('file_renamed',
+ dict(actions='rename_file', check='file_renamed')),
+ ('file_deleted',
+ dict(actions='delete_file', check='file_doesnt_exist')),
+ dict(_actions_base='create_file',
+ _item_path='new-file', _item_id='file-id',)),
+ (('dir_renamed',
+ dict(actions='rename_dir', check='dir_renamed')),
+ ('dir_deleted',
+ dict(actions='delete_dir', check='dir_doesnt_exist')),
+ for_dirs),
+ (('dir_renamed',
+ dict(actions='rename_dir', check='dir_renamed')),
+ ('dir_renamed2',
+ dict(actions='rename_dir2', check='dir_renamed2')),
+ for_dirs),
+ ]
+ return klass.mirror_scenarios(base_scenarios)
+
+ def do_delete_file(self):
+ sup = super(TestResolvePathConflict, self).do_delete_file()
+ # PathConflicts handle deletion differently and requires a special
+ # hard-coded value
+ return ('<deleted>',) + sup[1:]
+
+ def assertPathConflict(self, wt, c):
+ self.assertEqual(self._item_id, c.file_id)
+ self.assertEqual(self._this_path, c.path)
+ self.assertEqual(self._other_path, c.conflict_path)
+ _assert_conflict = assertPathConflict
+
+
+class TestResolvePathConflictBefore531967(TestResolvePathConflict):
+ """Same as TestResolvePathConflict but a specific conflict object.
+ """
+
+ def assertPathConflict(self, c):
+ # We create a conflict object as it was created before the fix and
+ # inject it into the working tree, the test will exercise the
+ # compatibility code.
+ old_c = conflicts.PathConflict('<deleted>', self._item_path,
+ file_id=None)
+ wt.set_conflicts(conflicts.ConflictList([c]))
+
+
class TestResolveDuplicateEntry(TestResolveConflicts):
preamble = """
@@ -557,7 +685,7 @@
""")
-class TestResolvePathConflict(TestResolveConflicts):
+class OldTestResolvePathConflict(TestResolveConflicts):
preamble = """
$ bzr init trunk
More information about the bazaar-commits
mailing list