On 12/10, Elijah Newren wrote: > On Mon, Dec 10, 2018 at 8:09 AM Duy Nguyen <pclouds@xxxxxxxxx> wrote: > > > > On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@xxxxxxxxx> 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@xxxxxxxxx> > > > --- > > > > > > 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.
