Code review comment for lp:~lifeless/bzr/bug-395556

As near as I can tell, you don't use deepcopy anymore so you don't need to import it
+from copy import deepcopy

and you need to revert the comment change:
41 - # list - which is a shallow copy.
42 + # list using a deep copy so that forward changes don't make the logic
43 + # more complex. Using a shallow copy results in all entries being seen
44 + # but the state of the entries being wrong, and that leads to stale
45 + # entries being left behind.

62 + if tracing:
63 + trace.mutter("Truncating from old '%s/%s'.",
64 + current_old[0][0].decode('utf'),
65 + current_old[0][1].decode('utf8'))

I don't know what encoding "utf" is, but I'd be really surprised if that works.

same here:
93 + if tracing:
94 + trace.mutter("Deleting from old '%s/%s'.",
95 + current_old[0][0].decode('utf'),
96 + current_old[0][1].decode('utf8'))

Perhaps a smoke test that you can set tracing on and have it not abort, though to really test that requires exercising all the code paths, which seems a bit much.

115 - raise AssertionError('could not find block for %s' % (other_key,))
116 + raise AssertionError('could not find block for %s' % (
117 + other_key,))
118 other_entry_index, present = self._find_entry_index(other_key,
119 self._dirblocks[other_block_index][1])
120 if not present:
121 - raise AssertionError('could not find entry for %s' % (other_key,))
122 + raise AssertionError('could not find entry for %s' % (
123 + other_key,))
124 if path_utf8 is None:
125 raise AssertionError('no path')
126 - self._dirblocks[other_block_index][1][other_entry_index][1][0] = \
127 + other_block = self._dirblocks[other_block_index][1]

move the "other_block = self._dirblocks[other_block_index][1]" up higher, and re-use it in the "other_entry_index, present = XXX" code.
...

126 - self._dirblocks[other_block_index][1][other_entry_index][1][0] = \
127 + other_block = self._dirblocks[other_block_index][1]
128 + # Turn this other location into a reference to the new
129 + # location. This also updates the aliased iterator that
130 + # set_state_from_inventory uses so that the old entry, if
131 + # not already examined, is skipped over.
132 + other_block[other_entry_index][1][0] = \
133 ('r', path_utf8, 0, False, '')
134 + if len(other_block[other_entry_index][1]) == 1:
135 + # We only have one tree in use, so an 'r' record is not
136 + # needed: remove it.
137 + other_block.pop(other_entry_index)

^- Why do you bother setting it, if you are just going to pop it out again right after?

The code comment here also is saying "updates the aliased iterator" but I don't see any such thing.

It at least looks like the code you've written is only tested for when there is a single tree, and not when there are merged trees. I could be wrong, but it at least feels like we need to do something more.

So at least, I would like you to:

1) Fix up the comments that are no longer correct
2) Fix 'utf' => 'utf-8'
3) Make sure (at least manually) that things are correct when there is a merge tree around.