Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[David Turner <dturner@twopensource.com>]

On Tue, 2014-07-01 at 13:15 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > When git checkout checks out a branch, create or update the
> > cache-tree so that subsequent operations are faster.
> >
> > Signed-off-by: David Turner <dturner@twitter.com>
> > ---
> >  builtin/checkout.c    |  8 ++++++++
> >  cache-tree.c          |  5 +++--
> >  t/t0090-cache-tree.sh | 15 ++++++++++++++-
> >  3 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index 07cf555..a023a86 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts *opts,
> >  		}
> >  	}
> >  
> > +	if (!active_cache_tree)
> > +		active_cache_tree = cache_tree();
> > +
> > +	if (!cache_tree_fully_valid(active_cache_tree))
> > +		cache_tree_update(active_cache_tree,
> > +				  (const struct cache_entry * const *)active_cache,
> > +				  active_nr, 0);
> > +
> 
> This looks much better than the previous round, but it still does
> allow verify_cache() to throw noises against unmerged entries in the
> cache, as WRITE_TREE_SILENT flag is not passed down, no?
> 
> 	$ git checkout master^0
>         $ git am $this_message
>         $ make
>         $ edit builtin/checkout.c ;# make changes to the above lines
>         $ ./git checkout -m master^0
> x       builtin/checkout.c: unmerged (972c8a7b28f16f88475268f9a714048c228e69db)
> x       builtin/checkout.c: unmerged (f1dc56e55f7b2200412142b10517458ccfda2952)
> x       builtin/checkout.c: unmerged (3b9753ba8c19e7dfe6e922f30eb85c83a92a4596)
>         M       builtin/checkout.c
>         Warning: you are leaving 1 commit behind, not connected to
>         any of your branches:
> 
>           25fab54 cache-tree: Create/update cache-tree on checkout
> 
>         Switched to branch 'master'
> 
> Passing WRITE_TREE_SILENT in the flags parameter will get rid of the
> conflict notice output from the above.
> 
> The user is not interested in writing a brand new tree object at all
> in this case, so it feels wrong to actually let the call chain go
> down to update_one() and create new tree objects.
> 
> 	Side note.  And passing WRITE_TREE_DRY_RUN is not a good
> 	solution either, because a later write_cache_as_tree() will
> 	not create the necessary tree object once you stuff a tree
> 	object name in the cache-tree.
> 
> What we want in this code path is a way to repair a sub cache_tree
> if it can be repaired without creating a new tree object and
> otherwise leave that part invalid.  The existing cache-tree
> framework is not prepared to do that kind of thing.  It wants to
> start from the bottom and percolate things up, computing levels
> nearer to the top-level only after it fully created the trees for
> deeper levels, because it is meant to be used only when we really
> want to write out trees.  We may want to reuse update_one() but
> 
> I am not convinced that doing an equivalent of write-tree when you
> switch branches is the right approach in the first place.  You will
> eventually write it out as a tree, and having a relatively undamaged
> cache-tree will help you when you do so, but spending the cycles
> necessary to compute a fully populated cache-tree, only to let it
> degrade over time until you are ready to write it out as a tree,
> somehow sounds like asking for a duplicated work upfront.

As I understand it, the cache-tree extension was originally designed to
speed up writing the tree later.  However, as Karsten Blees's work (and
my own tests) have shown, it also speeds up git status.  I use git
status a lot while working, and I've talked to a few others who do the
same.  So I think it's worth spending extra time when switching branches
to have a good working experience within that branch.

In the new version of the patchset (which I'll post shortly), I've added
an option WRITE_TREE_REPAIR, which does all of the work to compute a new
tree object, but only adds it to the cache-tree if it already exists
on-disk.  This is a little wasteful for the reason that you note.  So if
you would like, I could add a config option to skip it.  But I think it
is a good default.

Does this seem OK to you, assuming the implementation is good?