[brisbane-core/MERGE] inventory fix & refactor

[John Arbash Meinel]

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

ian.clatworthy at internode.on.net wrote:
> The attached patch fixes CHKInventory._get_mutable_inventory()
> and refactors CHKInventory_create_by_apply_delta() so that
> an apply_delta() method can be added real soon now.
> 
> Altogether, there are currently 15 public methods on Inventory
> that CHKInventory doesn't support. Adding apply_delta() will
> reduce that number by one and make it easy to add others
> that I need (e.g. add, __del__, rename) for fastimport to work
> on formats using CHKInventories. Yes, I know that fastimport
> will need tweaking to be efficent on new formats but I want
> it working first.
> 
> As I'm partly offline currently and can't commit to the branch,
> can I please ask the reviewer to merge this?
> 
> Thanks,
> Ian C.

Comment:

I don't think we *want* to support mutating a CHKInventory in memory. I
would rather get rid of Inventory apis that we don't want to support,
than mutate CHKInventory into conforming with the existing interface.

I should also mention that you introduced "tabs" instead of whitespace.
I'm guessing your environment has dramatically changed recently (your
editor seems different, and your emails are being formatted *very*
differently).

So the tabs thing is sufficient for BB:resubmit, but more critical is
deciding whether we we want to allow mutating a CHKInventory in-place.

To give some background-

(CHK)Inventory._get_mutable_inventory was introduced because we have a
MemoryTree which is effectively a working tree without disk. We need to
be able to add/remove/rename/etc files in that object.
However, for committed data (like RevisionTrees) that is never necessary.

We gain a lot by not allowing arbitrary mutations on CHKInventory. We
know what is 'dirty', we can cache various InventoryEntries because we
know people are not allowed to mutate them (at least, not mutate them
and expect it to be automatically noticed.)

In the end, I think it is worthwhile to have a "FixedInventory"
interface, which only has "create_by_apply_delta" which creates another
"FixedInventory", and a "MutableInventory" interface, along with some
ways to go between them. (MutableInventory.save() could create/return a
FixedInventory.)

I may be one of the only/few that feel this way, so I'm happy to start a
discussion on it.

John
=:->


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

iEYEARECAAYFAkmuj2EACgkQJdeBCYSNAAMUWQCeNloKSPI1kR1Nirz1K+0YzQ+X
k5AAnitc0TF7vy0HQaoaI4KUW/iJAh5U
=ncx5
-----END PGP SIGNATURE-----