Re: git status: small difference between stating whole repository and small subdirectory

From: Jeff King <peff@peff.net>
Date: Feb 20, 2012, 20:09 UTC
Message-ID: <20120220200902.GA5314@sigill.intra.peff.net>
In-Reply-To: <7vr4xpl1nm.fsf@alter.siamese.dyndns.org>

On Mon, Feb 20, 2012 at 11:56:13AM -0800, Junio C Hamano wrote:

Show 5 quoted lines

> 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