Jeff King [off-list ref] writes:
Interestingly, on my git.git repo, I had an empty cache. Running "git
read-tree HEAD" filled it (according to test-dump-cache-tree). It seems
that running "git checkout" empties the cache. So perhaps git could do
better about keeping the cache valid over time.
At least in the early days unpack-trees built the result by manually
adding an entry without calling the add_index_entry() all over the place,
which meant that it was futile to pretend that there is even a slight
chance that complex beast would correctly invalidate cached tree
information at all the necessary places. I recall that I added a code to
nuke the cache tree at the very beginning of "merging" codepaths to avoid
any bogus cache tree result to be stored in the resulting index.
These days, we have src_index and dst_index, and dst_index IIRC can start
as empty in which case "start from kept information and selectively
invalidate" would not work at all. When src_index and dst_index are the
same, however, you should be able to keep the cached tree valid, at least
in theory.
On Mon, Feb 20, 2012 at 11:56:13AM -0800, Junio C Hamano wrote:
These days, we have src_index and dst_index, and dst_index IIRC can start
as empty in which case "start from kept information and selectively
invalidate" would not work at all. When src_index and dst_index are the
same, however, you should be able to keep the cached tree valid, at least
in theory.
Yeah, I was worried that the cache invalidations sprinkled throughout
unpack-trees.c would not be sufficient (and because we are invalidating,
a missing invalidation would give us bogus cache info, which is Very
Bad).
So I think the one-liner I posted before is not sufficient in the
general case, because it definitely doesn't consider where the
destination is starting from. It should at least be more like:
if (src_index == dst_index) {
/* We would ordinarily want to do a deep copy here, but since
* we know that we will be overwriting src_index in the long
* run, it's OK to just take ownership of its cache_tree. */
o->result.cache_tree = o->src_index->cache_tree;
o->src_index->cache_tree = NULL;
}
[... do the usual tree traversal here, except invalidate entries in
o->result.call_tree instead of o->src_index. That makes it a
no-op when src_index != dst_index (because we have no cache tree
defined in result, then), and otherwise we are invalidating what
will go into the result...]
[then as before, we copy the result to dst_index; except now the
result may have src_index's cache_tree plus any invalidations]
o->result = *o->dst_index;
And fortunately that does exactly what we want in all cases, because we
always either read from and write to the_index, or we write to NULL (in
which case we will not bother with a cache_tree for the result, and it
is fixing a minor bug that we might be invalidating src_index's tree in the
first place).
I'm still slightly worried that we are missing some invalidation
somewhere deep in unpack_tree's callbacks (especially because they _are_
callbacks, and invalidating the cache_tree properly is now a promise
that the callbacks have to make).
-Peff