Code review comment for lp:~vila/bzr/375898-fix-root-more
Revision history for this message
| Vincent Ladeuil (vila) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
35a32c5
(Get the code!)
>>>>> "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-
So, if I put back your patch to _merge_names, 6 tests are failing with
CantMoveRoot including:
^^^^[log from bzrlib.
-------
Traceback (most recent call last):
File "/home/
nb_conflicts = outer.merge_
File "/home/
result = unbound(self, *args, **kwargs)
File "/home/
conflicts = merger.do_merge()
File "/home/
self.
File "/home/
merge.
File "/home/
self.
File "/home/
lambda t, c: conflict_pass(t, c, self.other_tree))
File "/home/
new_
File "/home/
lambda t, c: conflict_pass(t, c, self.other_tree))
File "/home/
tt.
File "/home/
TreeTransfo
File "/home/
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 to move the root (nor adding
it), the *content* of the root has been dealt with during a *previous*
merge. We are only touching files that *were* in the other_root.
Hence, I concluded that this patch was not in the right direction and
removed it.
Now (as you hinted below and I agree with that) there is still a problem
somewhere and my gut feeling is that the switch-the-winners code is a
bug and that the existing code is working around this bug so that two
wrongs make a right (or several).
I even think that switch-the-winners is right as long as a single root
is involved... and I won't be surprised if fixing merge_names allow us
to also get rid of fix_root() and fixup_new_roots(). I suspect
adjust_name() should be fixed to special case the roots and that may be
enough but I don't want to dive into this without better tests.
<snip/>
jam> ^- The comment here is certainly wrong now. Perhaps something like:
jam> # other_root doesn't have a physical representation. We still need to
jam> # move any references to the actual root of the tree.
Fixed.
jam> + if not present:
jam> + try:
jam> + # If the other root is not in the current tree, we
jam> don't
jam> + # reparent file and dirs that are expected in the final
jam> + # tree. These paths where preventively introduced in
jam> + # _merge_names for possible conflicts. We may want
jam> to find
jam> + # a better way -- vila 20100401
jam> + self.tt.
jam> + self.tt.
jam> + continue
jam> + except (errors.
jam> + pass
jam> ^- This section is a bit ugly.
I went to fully inline reparent_children into fix_root(), I think this
version is now clearer:
if self.other_
# the other tree's root is a non-root in the current tree (as when
# a previously unrelated branch is merged into another)
return
try:
except NoSuchFile:
# other_root doesn't have a physical representation. We still need
# to move any references to the actual root of the tree.
# 'other_
# calling adjust_path for children which *want* to be present with a
# correct place to go.
for thing, child in self.other_
if not other_root_
# Move the item into the root
if other_root_
jam> Certainly it seems to be necessary, but it certainly feels like
jam> 'accumulating cruft'.
Well, the 'try: final_kind() expect NoSuchFile: blah' idiom is used in
many places, and has puzzled me long enough, we may want to embed that
exception catching in final_kind and just let him return None.
From there we could just do:
if not other_root_
if self.tt.
# The item exist in the final tree and has a defined place
# to go already.
# move it into the root
Is that the cruft you were referring to ?
jam> The paragraph is a bit hard to read. How about:
Right.
jam> rename "present" to "parent_is_present"
jam> Change the block to:
Addressed by the inlining above.
jam> However, in working through this logic, I'm pretty sure it is
jam> wrong. Specifically, consider the case where "final_name"
jam> raises an exception.
Bah, nice catch. While restarting the branch from scratch I missed that,
thanks.
<snip/>
jam> I'm hesitant, but okay with landing the patch. You've pretty
jam> clearly demonstrated that it fixes the problem. The hesitancy
jam> is that it "doesn't make sense" to me. Which is fairly serious,
I share the feeling but for different reasons. That makes it even more
serious, but I think the correct way to fix that is to add more tests
for transform and push down fix_root() and fixup_new_roots() which
sounds even more like 'accumulating cruft' to me. Hopefully we'll be
able to also address the switch-the-winners in _merge_names doing so.
jam> but we've already spent far too long on this. I don't see it
jam> getting much better without some more serious surgery on the
jam> internals of merge and transform...
There we fully agree on both points.
I'll merge with the fixes mentioned above, yell if you disagree ;-)