[RFC] Inventory delta support for CommitBuilder

[Ian Clatworthy]

Aaron Bentley wrote:
> Ian Clatworthy wrote:
>> Attached are some changes I'd like some feedback on before I proceed with:
> 
>> * writing more supporting tests
>> * making associated changes to commit.py.
> 
> 
>> 1. The logic in apply_inventory_delta() on MutableTree should largely
>>    move down to Inventory.apply_delta().
> 
> We want to stop exposing the concept of inventory as part of the public
> API.  Tree.apply_inventory_delta is meant to assist in this transition.
>  So this move would be against our larger goals.

That's exactly the sort of thing I was fishing for. Thanks.

>>    BTW, as well as being called
>>    inside builder.finish_inventory(), I want Inventory.apply_delta()
>>    so that certain changes detected during commit processing (e.g.
>>    SHAs & sizes of new files) can be passed back to the working tree
>>    as an atomic transaction.
> 
> In particular, we want to reimplement apply_inventory_delta on WT4, so
> that it doesn't require an inventory read.  So using
> Inventory.apply_delta for the WT would be bad-- it would ensure we
> continue to use Inventory on dirstate trees, instead of updating the
> dirstate tree efficiently.

OK, I'll come back to that bridge later. It's the implicit flush() and
write_inventory() inside apply_inventory_delta that causes the problems
(performance) that (partially) led me to wanting to operate on the
Inventory "in memory" & just set a dirty flag. A custom
apply_inventory_delta() on dirstate sounds a better solution.

> Step 2 seems to go against the purpose of dirstate trees, and also seems
> to go against the idea that inventories will be provided as some kind of
> delta in packs.

Did you really mean step 2 here (the CommitBuilder.update_entry_contents
proposal) or step 1 (above)? Apologies if I'm missing something obvious
here but I don't see the connection of update_entry_contents() to
dirstate. Isn't dirstate just a working tree format or does it imply
more than that?

Trying to address your bigger picture point about removing inventory
from APIs as a worthy goal, the new arg (called changes) to
finish_inventory was taken from the arg to apply_inventory_delta: a list
of tuples including the new InventoryEntry. It therefore may not be easy
in the short term to fully hide InventoryEntrys. Or maybe it is? If you
think it would be better, I could flatten the InventoryEntry object
passed to update_entry_content() to multiple parameters. That would
imply that update_entry_contents builds the changes delta internally
inside the CommitBuilder and uses it implicitly in finish_inventory.

FWIW, there are some UI implementation issues around "pointless commits"
and verbose reporting that make my think that tracking
deletions/modifications/additions separately is a good idea. The new
Inventory.make_delta_from_edits() method can turn those into the changes
expected by apply_inventory_delta. I'm mentioning that because that
tracking of those 3 lists would move into the CommitBuilder if I
flattened IEs. Not a problem against flattening I hope - just something
I wanted to mention in case it triggers some further insight you had.

Ian C.