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

On Thu, 2009-07-16 at 21:43 +0000, John A Meinel wrote:
> 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?

Because:

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

old_iterator - the iterator over the shallow copy will see the entry
tuples, and thus needs to see the 'r' record, to skip over it. If it
doesn't see the 'r' record, it changes it to 'a' in the row, which
corrupts multi column dirstates. Auditing the full logic again may make
it cleaner, but I'm really on a diversion here from fast-commit, and I
have limited time budget :(.

> 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

Done. And I've tweaked 'aliased iterator' to mention current_old
specifically.

> 2) Fix 'utf' => 'utf-8'

Done.

> 3) Make sure (at least manually) that things are correct when there is
> a merge tree around.

It is correct :).

The loop finds all the rows that are used by the merge trees; column 0
is set to 'r' in them all, and in the special case of there being - ah,
found a remaining conceptual bug. Will tweak, and I think it will make
it more obvious to you.

-Rob