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

Revision history for this message
Andrew Bennetts (spiv) wrote :

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

« Back to merge proposal