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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html