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
1=== modified file 'NEWS'
2--- NEWS 2010-03-25 02:01:47 +0000
3+++ NEWS 2010-04-02 12:47:23 +0000
4@@ -22,6 +22,9 @@
5 permissions as ``.bzr`` directory on a POSIX OS.
6 (Parth Malwankar, #262450)
7
8+* Additional merges after an unrelated branch has been merged with its
9+ history no longer crash when deleted files are involved.
10+ (Vincent Ladeuil, John Arbash Meinel, #375898)
11
12 bzr 2.0.5
13 #########
14
15=== modified file 'bzrlib/merge.py'
16--- bzrlib/merge.py 2009-10-27 09:45:35 +0000
17+++ bzrlib/merge.py 2010-04-02 12:47:23 +0000
18@@ -1,4 +1,4 @@
19-# Copyright (C) 2005, 2006, 2008 Canonical Ltd
20+# Copyright (C) 2005-2010 Canonical Ltd
21 #
22 # This program is free software; you can redistribute it and/or modify
23 # it under the terms of the GNU General Public License as published by
24@@ -901,21 +901,38 @@
25 other_root = self.tt.trans_id_file_id(other_root_file_id)
26 if other_root == self.tt.root:
27 return
28+ if self.other_tree.inventory.root.file_id in self.this_tree.inventory:
29+ # the other tree's root is a non-root in the current tree (as when
30+ # a previously unrelated branch is merged into another)
31+ return
32 try:
33 self.tt.final_kind(other_root)
34+ other_root_is_present = True
35 except NoSuchFile:
36- return
37- if self.other_tree.inventory.root.file_id in self.this_tree.inventory:
38- # the other tree's root is a non-root in the current tree
39- return
40- self.reparent_children(self.other_tree.inventory.root, self.tt.root)
41- self.tt.cancel_creation(other_root)
42- self.tt.cancel_versioning(other_root)
43-
44- def reparent_children(self, ie, target):
45- for thing, child in ie.children.iteritems():
46+ # other_root doesn't have a physical representation. We still need
47+ # to move any references to the actual root of the tree.
48+ other_root_is_present = False
49+ # 'other_tree.inventory.root' is not present in this tree. We are
50+ # calling adjust_path for children which *want* to be present with a
51+ # correct place to go.
52+ for thing, child in self.other_tree.inventory.root.children.iteritems():
53 trans_id = self.tt.trans_id_file_id(child.file_id)
54- self.tt.adjust_path(self.tt.final_name(trans_id), target, trans_id)
55+ if not other_root_is_present:
56+ # FIXME: Make final_kind returns None instead of raising
57+ # NoSuchFile to avoid the ugly construct below -- vila 20100402
58+ try:
59+ self.tt.final_kind(trans_id)
60+ # The item exist in the final tree and has a defined place
61+ # to go already.
62+ continue
63+ except errors.NoSuchFile, e:
64+ pass
65+ # Move the item into the root
66+ self.tt.adjust_path(self.tt.final_name(trans_id),
67+ self.tt.root, trans_id)
68+ if other_root_is_present:
69+ self.tt.cancel_creation(other_root)
70+ self.tt.cancel_versioning(other_root)
71
72 def write_modified(self, results):
73 modified_hashes = {}
74
75=== modified file 'bzrlib/tests/per_workingtree/test_merge_from_branch.py'
76--- bzrlib/tests/per_workingtree/test_merge_from_branch.py 2009-07-10 07:14:02 +0000
77+++ bzrlib/tests/per_workingtree/test_merge_from_branch.py 2010-04-02 12:47:23 +0000
78@@ -1,4 +1,4 @@
79-# Copyright (C) 2006 Canonical Ltd
80+# Copyright (C) 2006-2010 Canonical Ltd
81 # Authors: Robert Collins <robert.collins@canonical.com>
82 #
83 # This program is free software; you can redistribute it and/or modify
84@@ -20,13 +20,14 @@
85 import os
86
87 from bzrlib import (
88+ branchbuilder,
89 errors,
90 merge
91 )
92-from bzrlib.tests.per_workingtree import TestCaseWithWorkingTree
93-
94-
95-class TestMergeFromBranch(TestCaseWithWorkingTree):
96+from bzrlib.tests import per_workingtree
97+
98+
99+class TestMergeFromBranch(per_workingtree.TestCaseWithWorkingTree):
100
101 def create_two_trees_for_merging(self):
102 """Create two trees that can be merged from.
103@@ -114,3 +115,103 @@
104 self.tt.create_file('qux', trans_id)
105 this.merge_from_branch(other.branch, merge_type=QuxMerge)
106 self.assertEqual('qux', this.get_file_text('foo-id'))
107+
108+
109+class TestMergedBranch(per_workingtree.TestCaseWithWorkingTree):
110+
111+ def make_branch_builder(self, relpath, format=None):
112+ if format is None:
113+ format = self.bzrdir_format
114+ builder = branchbuilder.BranchBuilder(self.get_transport(),
115+ format=format)
116+ return builder
117+
118+ def make_inner_branch(self):
119+ bld_inner = self.make_branch_builder('inner')
120+ bld_inner.start_series()
121+ bld_inner.build_snapshot(
122+ '1', None,
123+ [('add', ('', 'inner-root-id', 'directory', '')),
124+ ('add', ('dir', 'dir-id', 'directory', '')),
125+ ('add', ('dir/file1', 'file1-id', 'file', 'file1 content\n')),
126+ ('add', ('file3', 'file3-id', 'file', 'file3 content\n')),
127+ ])
128+ bld_inner.build_snapshot(
129+ '3', ['1'], [('modify', ('file3-id', 'new file3 contents\n')),])
130+ bld_inner.build_snapshot(
131+ '2', ['1'],
132+ [('add', ('dir/file2', 'file2-id', 'file', 'file2 content\n')),
133+ ])
134+ bld_inner.finish_series()
135+ br = bld_inner.get_branch()
136+ return br
137+
138+ def assertTreeLayout(self, expected, tree):
139+ tree.lock_read()
140+ try:
141+ actual = [e[0] for e in tree.list_files()]
142+ # list_files doesn't guarantee order
143+ actual = sorted(actual)
144+ self.assertEqual(expected, actual)
145+ finally:
146+ tree.unlock()
147+
148+ def make_outer_tree(self):
149+ outer = self.make_branch_and_tree('outer')
150+ self.build_tree_contents([('outer/foo', 'foo')])
151+ outer.add('foo', 'foo-id')
152+ outer.commit('added foo')
153+ inner = self.make_inner_branch()
154+ outer.merge_from_branch(inner, to_revision='1', from_revision='null:')
155+ outer.commit('merge inner branch')
156+ outer.mkdir('dir-outer', 'dir-outer-id')
157+ outer.move(['dir', 'file3'], to_dir='dir-outer')
158+ outer.commit('rename imported dir and file3 to dir-outer')
159+ return outer, inner
160+
161+ def test_file1_deleted_in_dir(self):
162+ outer, inner = self.make_outer_tree()
163+ outer.remove(['dir-outer/dir/file1'], keep_files=False)
164+ outer.commit('delete file1')
165+ outer.merge_from_branch(inner)
166+ outer.commit('merge the rest')
167+ self.assertTreeLayout(['dir-outer',
168+ 'dir-outer/dir',
169+ 'dir-outer/dir/file2',
170+ 'dir-outer/file3',
171+ 'foo'],
172+ outer)
173+
174+ def test_file3_deleted_in_root(self):
175+ # Reproduce bug #375898
176+ outer, inner = self.make_outer_tree()
177+ outer.remove(['dir-outer/file3'], keep_files=False)
178+ outer.commit('delete file3')
179+ outer.merge_from_branch(inner)
180+ outer.commit('merge the rest')
181+ self.assertTreeLayout(['dir-outer',
182+ 'dir-outer/dir',
183+ 'dir-outer/dir/file1',
184+ 'dir-outer/dir/file2',
185+ 'foo'],
186+ outer)
187+
188+
189+ def test_file3_in_root_conflicted(self):
190+ outer, inner = self.make_outer_tree()
191+ outer.remove(['dir-outer/file3'], keep_files=False)
192+ outer.commit('delete file3')
193+ nb_conflicts = outer.merge_from_branch(inner, to_revision='3')
194+ self.assertEqual(4, nb_conflicts)
195+ self.assertTreeLayout(['dir-outer',
196+ 'dir-outer/dir',
197+ 'dir-outer/dir/file1',
198+ # Ideally th conflict helpers should be in
199+ # dir-outer/dir but since we can't easily find
200+ # back the file3 -> outer-dir/dir rename, root
201+ # is good enough -- vila 20100401
202+ 'file3.BASE',
203+ 'file3.OTHER',
204+ 'foo'],
205+ outer)
206+

Subscribers

People subscribed via source and target branches