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

On 12/10, Elijah Newren wrote:

On Mon, Dec 10, 2018 at 8:09 AM Duy Nguyen [off-list ref] wrote:

quoted

On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer [off-list ref] wrote:

quoted

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 <redacted>
---

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.
:-)

quoted

quoted

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.