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

-----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-----