Code review comment for lp:~vila/bzr/375898-fix-root-more

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 clearly
demonstrated that it fixes the problem. The hesitancy is that it
"doesn't make sense" to me. Which is fairly serious, but we've already
spent far too long on this. I don't see it getting much better without
some more serious surgery on the internals of merge and transform...

 review: needs_fixing
 merge: approve

(tweak)
John
=:->