Bazaar

Merge lp:~vila/bzr/375898-fix-root-more into lp:bzr/2.0

  1. 375898-fix-root-more
  2. Merge into 2.0
Proposed by Vincent Ladeuil
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~vila/bzr/375898-fix-root-more
Merge into: lp:bzr/2.0
Diff against target: 206 lines (+138/-17)
3 files modified
NEWS (+3/-0)
bzrlib/merge.py (+29/-12)
bzrlib/tests/per_workingtree/test_merge_from_branch.py (+106/-5)
To merge this branch: bzr merge lp:~vila/bzr/375898-fix-root-more
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing
bzr-core Pending
Review via email: mp+22625@code.launchpad.net

Commit message

(vila) Don't crash when merging from an already merged other root

Description of the change

This fixes bug #375898.
The scenario was a bit complex:
- a previously unrelated branch is imported with its history,
- more changes are made to the imported branch,
- in the main branch, a file is deleted (it came from the imported branch),
- subsequent merges fail

Long story short, there were still grey areas in the interactions between merge
and transform when tree root are involved.

Yet, we have a pretty good coverage from a high level that cover all the know
use cases (I had to try numerous different approaches before getting it right
(with jam's help and then some)).

The unit tests are lacking though and we may want to revisit that later (but I'm
out of steam for that right now).

Since the fix is simple but the problem can be encountered quite easily, I've proposed
against 2.0.

The fix itself, while correct and quite small leave me a bit unsatisfied since its
aim is to fix paths that weren't introduced correctly in the first place.

But I couldn't find the right way to do that (I strongly suspect something
wrong with _merge_names, but the lack of unit tests makes it too hard to verify).

Summary: John has been involved enough to get the review token, especially
since, in the end, I didn't use all of his proposed patches :)

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :
Download full text (3.5 KiB)

Cut and paste of my email, since Launchpad aborted with "something wrong":

Vincent Ladeuil wrote:
> > You have been requested to review the proposed merge of lp:~vila/bzr/375898-fix-root-more into lp:bzr/2.0.
 ...

> >
> > Summary: John has been involved enough to get the review token, especially
> > since, in the end, I didn't use all of his proposed patches :)

If I understand correctly you now:

1) Status quo and don't record a path for the other root (didn't change
merge_names)
2) Update fix_root() to handle when other_root isn't present in this,
but there may be children of it anyway.

I still think I'd prefer to update _merge_names, but you've clearly
fixed the immediate cases. As you mentioned, the test coverage seems
sufficient.

+ present = True
         except NoSuchFile:
- return
+ # The other root hasn't been added or versioned, so we're
done for
+ # now
+ present = False

^- The comment here is certainly wrong now. Perhaps something like:

# other_root doesn't have a physical representation. We still need to
# move any references to the actual root of the tree.

+ if not present:
+ try:
+ # If the other root is not in the current tree, we
don't
+ # reparent file and dirs that are expected in the final
+ # tree. These paths where preventively introduced in
+ # _merge_names for possible conflicts. We may want
to find
+ # a better way -- vila 20100401
+ self.tt.final_kind(trans_id)
+ self.tt.final_name(trans_id)
+ continue
+ except (errors.NoFinalPath, errors.NoSuchFile), e:
+ pass

^- This section is a bit ugly. Certainly it seems to be necessary, but
it certainly feels like 'accumulating cruft'. The paragraph is a bit
hard to read. How about:

 rename "present" to "parent_is_present"

Change the block to:

if not parent_is_present:
    # 'ie' is not present in this tree. We are calling reparent so that
    # children which *want* to be present have a place to go.
    # So we check to see which children want to be present but can't,
    # and skip the rest.
    try:
      self.tt.final_kind(trans_id):
      self.tt.final_name(trans_id):
    except (errors.NoSuchFile, errors.NoFinalPath), e:
      pass # The file wants to be present, but doesn't have a location
    else:
      continue
self.tt.adjust_path(self.tt.final_name(trans_id), target, trans_id)

However, in working through this logic, I'm pretty sure it is wrong.
Specifically, consider the case where "final_name" raises an exception.
You then have:

if not present:
  try:
    self.tt.final_name(trans_id) # raises exception
  except ... :
    pass:
self.tt.adjust_path(self.tt.final_name(trans_id), target, trans_id)

Clearly the *second* call to 'self.tt.final_name()' is going to raise an
exception *again*.

So if the code you have is correct, then it is correct if you just
remove the 'self.tt.final_name(trans_id)' from the inner try/except.

I'm hesitant, but okay with landing the patch. You've pretty clearl...

Read more...

review: Needs Fixing
Revision history for this message
Vincent Ladeuil (vila) wrote :
Download full text (8.5 KiB)

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> If I understand correctly you now:

    jam> 1) Status quo and don't record a path for the other root (didn't change
    jam> merge_names)

Yes, funnily (?) I'm in the same position ("doesn't make sense") as you
but on different points.

_merge_names() switch the winners when this_name is None. While this is
"correct" since many tests break if you change that, I still consider it
a bug.

    jam> 2) Update fix_root() to handle when other_root isn't present in this,
    jam> but there may be children of it anyway.

    jam> I still think I'd prefer to update _merge_names, but you've
    jam> clearly fixed the immediate cases.

I reverted that because, while it was making the whole code more
coherent, it was adding cruft and blurry my understanding even more than
the let-switch-the-winners-just-because.

So, if I put back your patch to _merge_names, 6 tests are failing with
CantMoveRoot including:

^^^^[log from bzrlib.tests.per_workingtree.test_merge_from_branch.TestMergedBranch.test_file3_in_root_conflicted(WorkingTreeFormat5)]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/tests/per_workingtree/test_merge_from_branch.py", line 204, in test_file3_in_root_conflicted
    nb_conflicts = outer.merge_from_branch(inner, to_revision='3')
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/decorators.py", line 192, in write_locked
    result = unbound(self, *args, **kwargs)
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/workingtree.py", line 942, in merge_from_branch
    conflicts = merger.do_merge()
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/merge.py", line 508, in do_merge
    self._do_merge_to(merge)
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/merge.py", line 480, in _do_merge_to
    merge.do_merge()
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/merge.py", line 620, in do_merge
    self._compute_transform()
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/merge.py", line 676, in _compute_transform
    lambda t, c: conflict_pass(t, c, self.other_tree))
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/transform.py", line 2742, in resolve_conflicts
    new_conflicts.update(pass_func(tt, conflicts))
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/merge.py", line 676, in <lambda>
    lambda t, c: conflict_pass(t, c, self.other_tree))
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/transform.py", line 2769, in conflict_pass
    tt.adjust_path(new_name, final_parent, existing_file)
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/transform.py", line 1135, in adjust_path
    TreeTransformBase.adjust_path(self, name, parent, trans_id)
  File "/home/vila/src/bzr/bugs/375898-new-root/bzrlib/transform.py", line 167, in adjust_path
    raise CantMoveRoot
CantMoveRoot: Moving the root directory is not supported at this time

Note that the traceback above shows that we have already called
fix_root() at this point and doing so we have tried to move the root
which is wrong. The test doesn't attempt...

Read more...

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

Fed to pqm

star-merge http://bazaar.launchpad.net/~vila/bzr/375898-fix-root-more http://bazaar.launchpad.net/~bzr-pqm/bzr/2.0

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-03-25 02:01:47 +0000
+++ NEWS 2010-04-02 12:47:23 +0000
@@ -22,6 +22,9 @@
22 permissions as ``.bzr`` directory on a POSIX OS.22 permissions as ``.bzr`` directory on a POSIX OS.
23 (Parth Malwankar, #262450)23 (Parth Malwankar, #262450)
2424
25* Additional merges after an unrelated branch has been merged with its
26 history no longer crash when deleted files are involved.
27 (Vincent Ladeuil, John Arbash Meinel, #375898)
2528
26bzr 2.0.529bzr 2.0.5
27#########30#########
2831
=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py 2009-10-27 09:45:35 +0000
+++ bzrlib/merge.py 2010-04-02 12:47:23 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2005, 2006, 2008 Canonical Ltd1# Copyright (C) 2005-2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -901,21 +901,38 @@
901 other_root = self.tt.trans_id_file_id(other_root_file_id)901 other_root = self.tt.trans_id_file_id(other_root_file_id)
902 if other_root == self.tt.root:902 if other_root == self.tt.root:
903 return903 return
904 if self.other_tree.inventory.root.file_id in self.this_tree.inventory:
905 # the other tree's root is a non-root in the current tree (as when
906 # a previously unrelated branch is merged into another)
907 return
904 try:908 try:
905 self.tt.final_kind(other_root)909 self.tt.final_kind(other_root)
910 other_root_is_present = True
906 except NoSuchFile:911 except NoSuchFile:
907 return912 # other_root doesn't have a physical representation. We still need
908 if self.other_tree.inventory.root.file_id in self.this_tree.inventory:913 # to move any references to the actual root of the tree.
909 # the other tree's root is a non-root in the current tree914 other_root_is_present = False
910 return915 # 'other_tree.inventory.root' is not present in this tree. We are
911 self.reparent_children(self.other_tree.inventory.root, self.tt.root)916 # calling adjust_path for children which *want* to be present with a
912 self.tt.cancel_creation(other_root)917 # correct place to go.
913 self.tt.cancel_versioning(other_root)918 for thing, child in self.other_tree.inventory.root.children.iteritems():
914
915 def reparent_children(self, ie, target):
916 for thing, child in ie.children.iteritems():
917 trans_id = self.tt.trans_id_file_id(child.file_id)919 trans_id = self.tt.trans_id_file_id(child.file_id)
918 self.tt.adjust_path(self.tt.final_name(trans_id), target, trans_id)920 if not other_root_is_present:
921 # FIXME: Make final_kind returns None instead of raising
922 # NoSuchFile to avoid the ugly construct below -- vila 20100402
923 try:
924 self.tt.final_kind(trans_id)
925 # The item exist in the final tree and has a defined place
926 # to go already.
927 continue
928 except errors.NoSuchFile, e:
929 pass
930 # Move the item into the root
931 self.tt.adjust_path(self.tt.final_name(trans_id),
932 self.tt.root, trans_id)
933 if other_root_is_present:
934 self.tt.cancel_creation(other_root)
935 self.tt.cancel_versioning(other_root)
919936
920 def write_modified(self, results):937 def write_modified(self, results):
921 modified_hashes = {}938 modified_hashes = {}
922939
=== modified file 'bzrlib/tests/per_workingtree/test_merge_from_branch.py'
--- bzrlib/tests/per_workingtree/test_merge_from_branch.py 2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_workingtree/test_merge_from_branch.py 2010-04-02 12:47:23 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2006 Canonical Ltd1# Copyright (C) 2006-2010 Canonical Ltd
2# Authors: Robert Collins <robert.collins@canonical.com>2# Authors: Robert Collins <robert.collins@canonical.com>
3#3#
4# This program is free software; you can redistribute it and/or modify4# This program is free software; you can redistribute it and/or modify
@@ -20,13 +20,14 @@
20import os20import os
2121
22from bzrlib import (22from bzrlib import (
23 branchbuilder,
23 errors,24 errors,
24 merge25 merge
25 )26 )
26from bzrlib.tests.per_workingtree import TestCaseWithWorkingTree27from bzrlib.tests import per_workingtree
2728
2829
29class TestMergeFromBranch(TestCaseWithWorkingTree):30class TestMergeFromBranch(per_workingtree.TestCaseWithWorkingTree):
3031
31 def create_two_trees_for_merging(self):32 def create_two_trees_for_merging(self):
32 """Create two trees that can be merged from.33 """Create two trees that can be merged from.
@@ -114,3 +115,103 @@
114 self.tt.create_file('qux', trans_id)115 self.tt.create_file('qux', trans_id)
115 this.merge_from_branch(other.branch, merge_type=QuxMerge)116 this.merge_from_branch(other.branch, merge_type=QuxMerge)
116 self.assertEqual('qux', this.get_file_text('foo-id'))117 self.assertEqual('qux', this.get_file_text('foo-id'))
118
119
120class TestMergedBranch(per_workingtree.TestCaseWithWorkingTree):
121
122 def make_branch_builder(self, relpath, format=None):
123 if format is None:
124 format = self.bzrdir_format
125 builder = branchbuilder.BranchBuilder(self.get_transport(),
126 format=format)
127 return builder
128
129 def make_inner_branch(self):
130 bld_inner = self.make_branch_builder('inner')
131 bld_inner.start_series()
132 bld_inner.build_snapshot(
133 '1', None,
134 [('add', ('', 'inner-root-id', 'directory', '')),
135 ('add', ('dir', 'dir-id', 'directory', '')),
136 ('add', ('dir/file1', 'file1-id', 'file', 'file1 content\n')),
137 ('add', ('file3', 'file3-id', 'file', 'file3 content\n')),
138 ])
139 bld_inner.build_snapshot(
140 '3', ['1'], [('modify', ('file3-id', 'new file3 contents\n')),])
141 bld_inner.build_snapshot(
142 '2', ['1'],
143 [('add', ('dir/file2', 'file2-id', 'file', 'file2 content\n')),
144 ])
145 bld_inner.finish_series()
146 br = bld_inner.get_branch()
147 return br
148
149 def assertTreeLayout(self, expected, tree):
150 tree.lock_read()
151 try:
152 actual = [e[0] for e in tree.list_files()]
153 # list_files doesn't guarantee order
154 actual = sorted(actual)
155 self.assertEqual(expected, actual)
156 finally:
157 tree.unlock()
158
159 def make_outer_tree(self):
160 outer = self.make_branch_and_tree('outer')
161 self.build_tree_contents([('outer/foo', 'foo')])
162 outer.add('foo', 'foo-id')
163 outer.commit('added foo')
164 inner = self.make_inner_branch()
165 outer.merge_from_branch(inner, to_revision='1', from_revision='null:')
166 outer.commit('merge inner branch')
167 outer.mkdir('dir-outer', 'dir-outer-id')
168 outer.move(['dir', 'file3'], to_dir='dir-outer')
169 outer.commit('rename imported dir and file3 to dir-outer')
170 return outer, inner
171
172 def test_file1_deleted_in_dir(self):
173 outer, inner = self.make_outer_tree()
174 outer.remove(['dir-outer/dir/file1'], keep_files=False)
175 outer.commit('delete file1')
176 outer.merge_from_branch(inner)
177 outer.commit('merge the rest')
178 self.assertTreeLayout(['dir-outer',
179 'dir-outer/dir',
180 'dir-outer/dir/file2',
181 'dir-outer/file3',
182 'foo'],
183 outer)
184
185 def test_file3_deleted_in_root(self):
186 # Reproduce bug #375898
187 outer, inner = self.make_outer_tree()
188 outer.remove(['dir-outer/file3'], keep_files=False)
189 outer.commit('delete file3')
190 outer.merge_from_branch(inner)
191 outer.commit('merge the rest')
192 self.assertTreeLayout(['dir-outer',
193 'dir-outer/dir',
194 'dir-outer/dir/file1',
195 'dir-outer/dir/file2',
196 'foo'],
197 outer)
198
199
200 def test_file3_in_root_conflicted(self):
201 outer, inner = self.make_outer_tree()
202 outer.remove(['dir-outer/file3'], keep_files=False)
203 outer.commit('delete file3')
204 nb_conflicts = outer.merge_from_branch(inner, to_revision='3')
205 self.assertEqual(4, nb_conflicts)
206 self.assertTreeLayout(['dir-outer',
207 'dir-outer/dir',
208 'dir-outer/dir/file1',
209 # Ideally th conflict helpers should be in
210 # dir-outer/dir but since we can't easily find
211 # back the file3 -> outer-dir/dir rename, root
212 # is good enough -- vila 20100401
213 'file3.BASE',
214 'file3.OTHER',
215 'foo'],
216 outer)
217

Subscribers

People subscribed via source and target branches