Merge lp:~lifeless/bzr/bug-403322 into lp:bzr/2.0

[Robert Collins]

[Robert Collins (lifeless)]

This avoids an ordering bug in dirstate updates by not using the less tested, O(tree) performing set_state_from_inventory method (which leaves us with one caller of it, and that internal to dirstate).

[Andrew Bennetts (spiv)]

The NEWS entry is a bit too abstract. It basically says "make the internals slightly nicer", but it's in the Bug Fixes section. I assume from your merge proposal that this actually fixes a bug, possibly a user-observable one, so the NEWS entry ought to say something about that. i.e. start the entry with something like "Fix IndexError on moving added file", then you can go into details.

32 + # reconstruct a clean inventory from the dirstate,
33 + self._inventory = None
34 + # generate a delta,
35 + delta = inv._make_delta(self.inventory)

That looks a bit odd. _make_delta is always called with None? Oh, I see "_inventory" != "inventory". Ugh. That's confusing (I spent too many minutes puzzling over this!), but not the fault of this patch I guess, and your comment (eventually) helped.

In general using _make_delta and apply_inventory_delta sounds good! So tweak that NEWS entry and you're good.

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Review: Approve
> The NEWS entry is a bit too abstract. It basically says "make the internals slightly nicer", but it's in the Bug Fixes section. I assume from your merge proposal that this actually fixes a bug, possibly a user-observable one, so the NEWS entry ought to say something about that. i.e. start the entry with something like "Fix IndexError on moving added file", then you can go into details.
>
> 32 + # reconstruct a clean inventory from the dirstate,
> 33 + self._inventory = None
> 34 + # generate a delta,
> 35 + delta = inv._make_delta(self.inventory)
>
> That looks a bit odd. _make_delta is always called with None? Oh, I see "_inventory" != "inventory". Ugh. That's confusing (I spent too many minutes puzzling over this!), but not the fault of this patch I guess, and your comment (eventually) helped.
>
> In general using _make_delta and apply_inventory_delta sounds good! So tweak that NEWS entry and you're good.
>

I think updating the comment to say:

# Setting self._inventory = None forces the dirstate to regenerate the
# working inventory. We do this because 'inv' may be a copy of
# self._inventory, or may be mutated, etc.
self._inventory = None
delta = inv._make_delta(self.inventory)

This doesn't seem like the best-performing way to go, but I will say the
delta code paths are probably more thoroughly tested at this point.

Have you tested to make sure that bug #403322 is really fixed? (bzr mv
of a newly added directory doesn't fail?)

It might be nice to have a test case added for that edge case.

def test_rename_one_after_add(self):
  tree = self.make_branch_and_tree('.')
  self.build_tree(['a/'])
  tree.add(['a'])
  tree.commit('a')
  self.build_tree(['a/a/'])
  tree.rename_one('a/a', 'a/d')
  tree.rename_one('a/d', 'a/b') # bug #403322

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrDX+sACgkQJdeBCYSNAANHiwCgiljZLNPxpLPwMT2S0SGnWJ+e
+DQAoNBO0TtlWCt2ujHzJX8JYbLPXy+3
=m9t2
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

On Wed, 2009-09-30 at 13:42 +0000, John A Meinel wrote:
>
>
> It might be nice to have a test case added for that edge case.

So, if I was going to add one I'd be strongly tempted to add a test at
the broken layer, but the fix is to stop using that layer.

-Rob

[Robert Collins (lifeless)]

Sadly, this WT level change breaks TestCommitBuilder, though I'm
*totally* at a loss to see how. I'm tracking it down nowish.

If its trivial I'll just land, otherwise we'll be needing more reviews.

-Rob

[Robert Collins (lifeless)]

On Thu, 2009-10-01 at 00:00 +0000, Robert Collins wrote:
> Sadly, this WT level change breaks TestCommitBuilder, though I'm
> *totally* at a loss to see how. I'm tracking it down nowish.
>
> If its trivial I'll just land, otherwise we'll be needing more reviews.

It was trivial.

-Rob

=== modified file 'NEWS'
--- NEWS	2009-09-30 00:02:11 +0000
+++ NEWS	2009-09-30 22:22:10 +0000
@@ -22,6 +22,14 @@
   ``keyboard-interactive`` auth but not ``password`` auth when using
   Paramiko.   (Andrew Bennetts, #433846)
 
+* Occasional IndexError on renamed files have been fixed. Operations that
+  set a full inventory in the working tree will now go via the
+  apply_inventory_delta code path which is simpler and easier to
+  understand than dirstates set_state_from_inventory method. This may
+  have a small performance impact on operations built on _write_inventory,
+  but such operations are already doing full tree scans, so no radical
+  performance change should be observed. (Robert Collins, #403322)
+
 Improvements
 ************
 

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2009-08-25 04:43:21 +0000
+++ bzrlib/workingtree_4.py	2009-09-30 22:22:10 +0000
@@ -1267,9 +1267,17 @@
         if self._dirty:
             raise AssertionError("attempting to write an inventory when the "
                 "dirstate is dirty will lose pending changes")
-        self.current_dirstate().set_state_from_inventory(inv)
-        self._make_dirty(reset_inventory=False)
-        if self._inventory is not None:
+        had_inventory = self._inventory is not None
+        # Setting self._inventory = None forces the dirstate to regenerate the
+        # working inventory. We do this because self.inventory may be inv, or
+        # may have been modified, and either case would prevent a clean delta
+        # being created.
+        self._inventory = None
+        # generate a delta,
+        delta = inv._make_delta(self.inventory)
+        # and apply it.
+        self.apply_inventory_delta(delta)
+        if had_inventory:
             self._inventory = inv
         self.flush()