Merge lp:~vila/bzr/375898-fix-root-more into lp:bzr/2.0
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 |
Related bugs: |
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 :)
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: final_kind( trans_id) final_name( trans_id) NoFinalPath, errors.NoSuchFile), e:
+ 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.
+ self.tt.
+ continue
+ except (errors.
+ 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: tt.final_ kind(trans_ id): tt.final_ name(trans_ id): NoFinalPath) , e: adjust_ path(self. tt.final_ name(trans_ id), target, trans_id)
# '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.
self.
except (errors.NoSuchFile, errors.
pass # The file wants to be present, but doesn't have a location
else:
continue
self.tt.
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: tt.final_ name(trans_ id) # raises exception adjust_ path(self. tt.final_ name(trans_ id), target, trans_id)
try:
self.
except ... :
pass:
self.tt.
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 final_name( trans_id) ' from the inner try/except.
remove the 'self.tt.
I'm hesitant, but okay with landing the patch. You've pretty clearl...