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

Jeff King [off-list ref] writes:

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.

It makes "git checkout" with no changes just work (since we preserve the
cache tree, and it doesn't need updated). It makes something like "git
checkout HEAD^" work, keeping most of the cache-tree intact, but
invalidating trees containing paths that were modified.

Great.  Here's a test you could use.  It's a bit noisy because the
shallow in test_shallow_cache_tree no longer made any sense, but I think
it tests what we want to see.

diff --git i/t/t0090-cache-tree.sh w/t/t0090-cache-tree.sh
index 6c33e28..5706305 100755
--- i/t/t0090-cache-tree.sh
+++ w/t/t0090-cache-tree.sh

@@ -16,14 +16,16 @@ cmp_cache_tree () {
 # We don't bother with actually checking the SHA1:
 # test-dump-cache-tree already verifies that all existing data is
 # correct.
-test_shallow_cache_tree () {
-	printf "SHA  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect &&
+test_cache_tree () {
+	printf "SHA  (%d entries, 1 subtrees)\n" $(git ls-files|wc -l) >expect &&
+	printf "SHA sub/ (%d entries, 0 subtrees)\n" $(git ls-files sub|wc -l) >>expect &&
 	cmp_cache_tree expect
 }
 
 test_invalid_cache_tree () {
-	echo "invalid                                   (0 subtrees)" >expect &&
-	printf "SHA #(ref)  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >>expect &&
+	echo "invalid                                   (1 subtrees)" >expect &&
+	printf "SHA #(ref)  (%d entries, 1 subtrees)\n" $(git ls-files|wc -l) >>expect &&
+	printf "SHA sub/ (%d entries, 0 subtrees)\n" $(git ls-files sub|wc -l) >>expect &&
 	cmp_cache_tree expect
 }
 

@@ -33,13 +35,16 @@ test_no_cache_tree () {
 }
 
 test_expect_failure 'initial commit has cache-tree' '
+	mkdir sub &&
+	echo bar > sub/bar &&
+	git add sub/bar &&
 	test_commit foo &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'read-tree HEAD establishes cache-tree' '
 	git read-tree HEAD &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'git-add invalidates cache-tree' '

@@ -59,7 +64,7 @@ test_expect_success 'update-index invalidates cache-tree' '
 test_expect_success 'write-tree establishes cache-tree' '
 	test-scrap-cache-tree &&
 	git write-tree &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'test-scrap-cache-tree works' '

@@ -70,24 +75,39 @@ test_expect_success 'test-scrap-cache-tree works' '
 
 test_expect_success 'second commit has cache-tree' '
 	test_commit bar &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'reset --hard gives cache-tree' '
 	test-scrap-cache-tree &&
 	git reset --hard &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'reset --hard without index gives cache-tree' '
 	rm -f .git/index &&
 	git reset --hard &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
-test_expect_failure 'checkout gives cache-tree' '
+test_expect_success 'checkout HEAD leaves cache-tree intact' '
+	git read-tree HEAD &&
+	git checkout HEAD &&
+	test_cache_tree
+'
+
+# 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
 '
 
 test_done

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

quoted

+	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^.

+# 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