Re: [PATCH 4/8] read-cache: add invalidate parameter to remove_marked_cache_entries

[Thomas Gummerer <t.gummerer@gmail.com>]

On 12/10, Elijah Newren wrote:
> On Mon, Dec 10, 2018 at 8:09 AM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > >
> > > When marking cache entries for removal, and later removing them all at
> > > once using 'remove_marked_cache_entries()', cache entries currently
> > > have to be invalidated manually in the cache tree and in the untracked
> > > cache.
> > >
> > > Add an invalidate flag to the function.  With the flag set, the
> > > function will take care of invalidating the path in the cache tree and
> > > in the untracked cache.
> > >
> > > This will be useful in a subsequent commit.
> > >
> > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > ---
> > >
> > > For the two current callsites, unpack-trees seems to do this
> > > invalidation itself internally.
> >
> > I'm still a bit scared of this invalidation business in unpack-trees.
> > The thing is, we handle two separate index_state there, src_index and
> > result and invalidation has to be done on the right one (because index
> > extensions are on src_index until the very end of unpack-trees;
> > invalidating on 'result' would be no-op and wrong).
> > remove_marked_cache_entries() seems to be called on 'result' while
> > invalidate_ce_path() is on src_index, hm....
> 
> Is Thomas avoiding problems here simply because merge is the only
> caller of unpack_trees with src_index != dst_index?  Or does src_index
> == dst_index for checkout not actually help?

I'm trying to avoid problems in this patch by keeping status quo, and
not changing the cache-tree invalidation in any way.  'git checkout --
<pathspec>' doesn't use unpack-trees, so I don't think I have to worry
about src_index vs. dst_index.

In what I was saying above I was merely trying to explain why we don't
need invalidate the cache-tree in the 'remove_marked_cache_entries()'
function.

> If that does help with the checkout case, then allow me to find a
> different way to muddy the waters...  I think I might want to make use
> of this function in the merge machinery at some point, so I either
> need to figure out how to convince you to verify if all this cache
> tree invalidation stuff is sane, or somehow figure out all the
> cache_tree stuff stuff myself so I can figure out what is right here.
> :-)
> 
> > > I don't quite understand why we don't
> > > need it in split-index mode though.  I assume it's because the cache
> > > tree in the main index would already have been invalidated?  I didn't
> > > have much time to dig, but couldn't produce any failures with it
> > > either, so I assume not invalidating paths is the right thing to do
> > > here.
> >
> > Yeah I think it's because cache-tree and untracked cache are already
> > properly invalidated. This merge base thingy is done when we load the
> > index files up, not when we write them down. The "front" index may
> > record that a few paths in the base index are no longer valid and need
> > to be deleted. But untracked cache and cache-tree both should have
> > recorded that same info when these paths are marked for delete at
> > index write time.

Thanks, that makes sense to me.