dirstate file gets damaged by commit() in a merge when there are 2 deletes in a directory.

[Matthew Fuller]

update_delta_from_parent in WorkingTree4 fails to correctly update during a commit when the number of deleted items in any directory is 2.

[Martin Pool (mbp)]

Thanks for the reproduction recipe, that's very useful.

I thought there was already a bug like this reported...

[John A Meinel (jameinel)]

I can confirm that it happens with bzr.dev 2975, but it doesn't happen with bzr 0.91.0.final
Nor does it happen with bzr 0.92 (2949).

I'll try to track down the problem

[John A Meinel (jameinel)]

This was introduced by Robert's 'update_basis_from_delta' patch. At least it succeeds with bzr.dev 2966 but fails with:
 2967 Canonical.com Patch Queue Manager 2007-11-05 [merge]
      (robertc) Implement WorkingTree4.update_basis_by_delta giving a 30% performance improvement for commit. (Robert Collins)

So I'm guessing the commit after the file has been moved and the other file has been deleted is getting a delta that it isn't handling properly.

[Robert Collins (lifeless)]

On Fri, 2007-11-09 at 13:29 +0000, John A Meinel wrote:
>
> This was introduced by Robert's 'update_basis_from_delta' patch.

Ok, we should add tests to the update_basis_from_delta interface tests
to trigger this then I think.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

[John A Meinel (jameinel)]

working on it in the associated branch

[John A Meinel (jameinel)]

It seems the problem is that 'update_basis_by_delta' is deleting the 'b/' directory, but leaving the 'b/b' file.

At the moment, I only see 'update_basis_by_delta' tests testing a single parent.

I wonder if that isn't the problem.

It is also certainly an interaction between the 'WT.unversion()' followed by 'WT.update_basis_by_delta'.

To this point, I haven't tracked down the specific cause, but I figured I would give an update on progress.

[Robert Collins (lifeless)]

On Tue, 2007-11-13 at 23:41 +0000, John A Meinel wrote:
> It seems the problem is that 'update_basis_by_delta' is deleting the
> 'b/' directory, but leaving the 'b/b' file.
>
> At the moment, I only see 'update_basis_by_delta' tests testing a single
> parent.

There are two relevant tests here - test_removes - tests for the b/b
case. And test_content_from _second_parent_is_dropped which tests the
two-parent case, but certainly not exhaustively. Note that the
implementation starts by doing self._discard_merge_parents. Possibly
that is borked; but it should be possible to eliminate that promptly
(and it looks fine to me).

> I wonder if that isn't the problem.
>
> It is also certainly an interaction between the 'WT.unversion()'
> followed by 'WT.update_basis_by_delta'.

Interesting. I don't know that the interface tests test this
interaction; though I would have thought they do.

> To this point, I haven't tracked down the specific cause, but I figured
> I would give an update on progress.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

[John A Meinel (jameinel)]

I've tracked this down to a simple logic bug in _discard_merge_parents. It iterates through, and for blocks with all removed children, it removes the whole thing in one pass, rather than removing the children one by one. But the check for 'all removed children' was incorrect.

The actual fix is almost trivial:
=== modified file 'bzrlib/dirstate.py'
--- bzrlib/dirstate.py 2007-11-05 19:40:28 +0000
+++ bzrlib/dirstate.py 2007-11-14 20:10:39 +0000
@@ -914,7 +914,7 @@
                     if (entry[1][0][0], entry[1][1][0]) in dead_patterns:
                         deleted_positions.append(pos)
                 if deleted_positions:
- if len(deleted_positions) == len(block):
+ if len(deleted_positions) == len(block[1]):
                         del block[1][:]
                     else:
                         for pos in reversed(deleted_positions):

The reason it is only triggered under the given conditions, is that you must have exactly 2 deleted items. And then it accidentally deletes the whole block.

[John A Meinel (jameinel)]

This bug hasn't been present in a release, but only existed between 0.92 and 1.0rc1 in bzr.dev.

Merged as bzr.dev 2998