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

From: Jeff King <peff@peff.net>
Date: Feb 20, 2012, 20:35 UTC
Message-ID: <20120220203540.GA5966@sigill.intra.peff.net>
In-Reply-To: <87d3991gyg.fsf@thomas.inf.ethz.ch>

On Mon, Feb 20, 2012 at 07:45:59PM +0100, Thomas Rast wrote:

Show 13 quoted lines

> > +	o->result.cache_tree = o->src_index->cache_tree;
> >  	o->src_index = NULL;
> >  	ret = check_updates(o) ? (-2) : 0;
> >  	if (o->dst_index)
> 
> Brilliant.  I know I'm stealing Junio's punchline, but please make it so
> :-)
> 
> Browsing around in history, it seems that this was silently broken by
> 34110cd (Make 'unpack_trees()' have a separate source and destination
> index, 2008-03-06), which introduced the distinction between source and
> destination index.  Before that they were the same, so the cache tree
> would have been updated correctly.

OK, good. When you write a one-liner that makes a huge change in performance, it is usually a good idea to think to yourself "no, it couldn't be this easy, could it?".

But after more discussion from people more clueful than I (this is the first time I've even looked at cache-tree code), I'm feeling like this is the right direction, at least, if not exactly the right patch. And seeing that it is in fact a regression in 34110cd, and that the existing cache-tree invalidations predate that makes me feel better. At one point, at least, they were complete and we were depending on them to be accurate. Things may have changed since then, of course, but I at least know that they were sufficient in 34110cd^.

Show 14 quoted lines

> +# NEEDSWORK: only one of these two can succeed.  The second is there
> +# because it would be the better result.
> +test_expect_success 'checkout HEAD^ correctly invalidates cache-tree' '
> +	git checkout HEAD^ &&
> +	test_invalid_cache_tree
> +'
> +
> +test_expect_failure 'checkout HEAD^ gives full cache-tree' '
> +	git checkout master &&
> +	git read-tree HEAD &&
>  	git checkout HEAD^ &&
> -	test_shallow_cache_tree
> +	test_cache_tree
>  '

I think you can construct two tests that will both work in the "ideal" case. In the first one, you move to a tree that updates "foo", and therefore the root cache-tree is invalidated.

In the second, you update "subdir1/foo" in the index, then move to a commit that differs in "subdir1/bar" and "subdir2/bar". You should see that subdir2 has the cache-tree of the destination commit, but that subdir1 is invalidated (and therefore the root is also invalidated). That will fail with my patch, of course, as it would invalidate subdir2, also; so it would just be an expect_failure for somebody in the future.

In general, t0090 could benefit from using a larger tree. For example, the add test does "git add foo" and checks that the root cache-tree was invalidated. But it should _also_ check that the cache-tree for a subdirectory is _not_ invalidated (and it isn't; git-add does the right thing).

I'll see if I can work up some fancier tests, too.

-Peff