[PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[David Turner]

When git checkout checks out a branch, create or update the
cache-tree so that subsequent operations are faster.

Signed-off-by: David Turner <dturner@twitter.com>
---
 builtin/checkout.c    |  8 ++++++++
 cache-tree.c          |  5 +++--
 t/t0090-cache-tree.sh | 15 ++++++++++++++-
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07cf555..a023a86 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		}
 	}
 
+	if (!active_cache_tree)
+		active_cache_tree = cache_tree();
+
+	if (!cache_tree_fully_valid(active_cache_tree))
+		cache_tree_update(active_cache_tree,
+				  (const struct cache_entry * const *)active_cache,
+				  active_nr, 0);
+
 	if (write_cache(newfd, active_cache, active_nr) ||
 	    commit_locked_index(lock_file))
 		die(_("unable to write new index file"));
diff --git a/cache-tree.c b/cache-tree.c
index 7fa524a..28d2356 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -612,9 +612,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 			cache_tree_find(active_cache_tree, prefix);
 		if (!subtree)
 			return WRITE_TREE_PREFIX_ERROR;
-		hashcpy(sha1, subtree->sha1);
+		if (sha1)
+			hashcpy(sha1, subtree->sha1);
 	}
-	else
+	else if (sha1)
 		hashcpy(sha1, active_cache_tree->sha1);
 
 	if (0 <= newfd)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 6c33e28..7c60675 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' '
 	test_shallow_cache_tree
 '
 
-test_expect_failure 'checkout gives cache-tree' '
+test_expect_success 'checkout gives cache-tree' '
+	git tag current
 	git checkout HEAD^ &&
 	test_shallow_cache_tree
 '
 
+test_expect_success 'checkout -b gives cache-tree' '
+	git checkout current &&
+	git checkout -b prev HEAD^ &&
+	test_shallow_cache_tree
+'
+
+test_expect_success 'checkout -B gives cache-tree' '
+	git checkout current &&
+	git checkout -B prev HEAD^ &&
+	test_shallow_cache_tree
+'
+
 test_done
-- 
2.0.0.390.gcb682f8

[PATCH 2/3] test-dump-cache-tree: Improve output format and exit code

[David Turner]

Make test-dump-cache-tree more useful for testing.  Do not treat known
invalid trees as errors (and do not produce non-zero exit code),
because we can fall back to the non-cache-tree codepath.

Signed-off-by: David Turner <dturner@twitter.com>
---
 t/t0090-cache-tree.sh  | 28 +++++++++++++++++++++++++---
 test-dump-cache-tree.c | 24 ++++++++++++------------
 2 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 7c60675..5383258 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -21,10 +21,13 @@ test_shallow_cache_tree () {
 	cmp_cache_tree expect
 }
 
+# Test that the cache-tree for a given directory is invalid.
+# If no directory is given, check that the root is invalid
 test_invalid_cache_tree () {
-	echo "invalid                                   (0 subtrees)" >expect &&
-	printf "SHA #(ref)  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >>expect &&
-	cmp_cache_tree expect
+	test-dump-cache-tree >actual &&
+	sed -e "s/$_x40/SHA/" -e "s/[0-9]* subtrees//g" <actual >filtered &&
+	expect=$(printf "invalid                                  $1 ()\n") &&
+	fgrep "$expect" filtered
 }
 
 test_no_cache_tree () {
@@ -49,6 +52,25 @@ test_expect_success 'git-add invalidates cache-tree' '
 	test_invalid_cache_tree
 '
 
+test_expect_success 'git-add in subdir invalidates cache-tree' '
+	test_when_finished "git reset --hard; git read-tree HEAD" &&
+	mkdir dirx &&
+	echo "I changed this file" > dirx/foo &&
+	git add dirx/foo &&
+	test_invalid_cache_tree
+'
+
+test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '
+	git tag no-children
+	test_when_finished "git reset --hard no-children; git read-tree HEAD" &&
+	mkdir dir1 dir2 &&
+	test_commit dir1/a &&
+	test_commit dir2/b &&
+	echo "I changed this file" > dir1/a &&
+	git add dir1/a &&
+	test_invalid_cache_tree dir1/
+'
+
 test_expect_success 'update-index invalidates cache-tree' '
 	test_when_finished "git reset --hard; git read-tree HEAD" &&
 	echo "I changed this file" > foo &&
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 47eab97..ad42ca1 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -6,12 +6,12 @@
 static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
 {
 	if (it->entry_count < 0)
-		printf("%-40s %s%s (%d subtrees)\n",
-		       "invalid", x, pfx, it->subtree_nr);
+		printf("%-40s %s (%d subtrees)%s\n",
+		       "invalid", pfx, it->subtree_nr, x);
 	else
-		printf("%s %s%s (%d entries, %d subtrees)\n",
-		       sha1_to_hex(it->sha1), x, pfx,
-		       it->entry_count, it->subtree_nr);
+		printf("%s %s (%d entries, %d subtrees)%s\n",
+		       sha1_to_hex(it->sha1), pfx,
+		       it->entry_count, it->subtree_nr, x);
 }
 
 static int dump_cache_tree(struct cache_tree *it,
@@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,
 		/* missing in either */
 		return 0;
 
-	if (it->entry_count < 0) {
+	if (it->entry_count < 0)
+		/* invalid */
 		dump_one(it, pfx, "");
-		dump_one(ref, pfx, "#(ref) ");
-		if (it->subtree_nr != ref->subtree_nr)
-			errs = 1;
-	}
 	else {
-		dump_one(it, pfx, "");
 		if (hashcmp(it->sha1, ref->sha1) ||
 		    ref->entry_count != it->entry_count ||
 		    ref->subtree_nr != it->subtree_nr) {
-			dump_one(ref, pfx, "#(ref) ");
+			/* claims to be valid but is lying */
+			dump_one(ref, pfx, " #(error)");
 			errs = 1;
+		} else {
+			/* is actually valid */
+			dump_one(it, pfx, "");
 		}
 	}
 
-- 
2.0.0.390.gcb682f8

[PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[David Turner]

During the commit process, the cache-tree is updated. We need to write
this updated cache-tree so that it's ready for subsequent commands.

Add test code which demonstrates that git commit now writes the cache
tree.  Also demonstrate that cache-tree invalidation is correct.

Signed-off-by: David Turner <dturner@twitter.com>
---
 builtin/commit.c      | 15 ++++++-------
 t/t0090-cache-tree.sh | 61 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..dbd4f4b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 
 		discard_cache();
 		read_cache_from(index_lock.filename);
+		if (update_main_cache_tree(WRITE_TREE_SILENT) >= 0)
+			write_cache(fd, active_cache, active_nr);
 
 		commit_style = COMMIT_NORMAL;
 		return index_lock.filename;
@@ -383,14 +385,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	if (!only && !pathspec.nr) {
 		fd = hold_locked_index(&index_lock, 1);
 		refresh_cache_or_die(refresh_flags);
-		if (active_cache_changed) {
-			update_main_cache_tree(WRITE_TREE_SILENT);
-			if (write_cache(fd, active_cache, active_nr) ||
-			    commit_locked_index(&index_lock))
-				die(_("unable to write new_index file"));
-		} else {
-			rollback_lock_file(&index_lock);
-		}
+		update_main_cache_tree(WRITE_TREE_SILENT);
+		if (write_cache(fd, active_cache, active_nr) ||
+		    commit_locked_index(&index_lock))
+			die(_("unable to write new_index file"));
 		commit_style = COMMIT_AS_IS;
 		return get_index_file();
 	}
@@ -435,6 +433,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	fd = hold_locked_index(&index_lock, 1);
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
+	update_main_cache_tree(WRITE_TREE_SILENT);
 	if (write_cache(fd, active_cache, active_nr) ||
 	    close_lock_file(&index_lock))
 		die(_("unable to write new_index file"));
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 5383258..d50acb8 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -16,8 +16,31 @@ cmp_cache_tree () {
 # We don't bother with actually checking the SHA1:
 # test-dump-cache-tree already verifies that all existing data is
 # correct.
+generate_expected_cache_tree () {
+	if [ -n "$1" ]
+	then
+		local dir="$1/"
+	else
+		local dir="$1"
+	fi
+	# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
+	# We want to count only foo because it's the only direct child
+	local subtrees=$(git ls-files|egrep '/' |cut -d / -f 1|uniq) &&
+	local subtree_count=$(echo "$subtrees"|grep -c .) &&
+	local files_count=$(git ls-files|grep -v /|wc -l) &&
+	local entries=$(expr "$subtree_count" + "$files_count") &&
+	printf "SHA $dir (%d entries, %d subtrees)\n" $entries $subtree_count &&
+	local subtree &&
+	for subtree in $subtrees
+	do
+	    cd "$subtree"
+	    generate_expected_cache_tree "$dir$subtree" || return 1
+	    cd ..
+	done
+}
+
 test_shallow_cache_tree () {
-	printf "SHA  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect &&
+	generate_expected_cache_tree > expect &&
 	cmp_cache_tree expect
 }
 
@@ -35,7 +58,7 @@ test_no_cache_tree () {
 	cmp_cache_tree expect
 }
 
-test_expect_failure 'initial commit has cache-tree' '
+test_expect_success 'initial commit has cache-tree' '
 	test_commit foo &&
 	test_shallow_cache_tree
 '
@@ -60,15 +83,28 @@ test_expect_success 'git-add in subdir invalidates cache-tree' '
 	test_invalid_cache_tree
 '
 
+cat >before <<\EOF
+SHA  (3 entries, 2 subtrees)
+SHA dir1/ (1 entries, 0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
+cat >expect <<\EOF
+invalid                                   (2 subtrees)
+invalid                                  dir1/ (0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
 test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '
 	git tag no-children
 	test_when_finished "git reset --hard no-children; git read-tree HEAD" &&
 	mkdir dir1 dir2 &&
 	test_commit dir1/a &&
 	test_commit dir2/b &&
+	cmp_cache_tree before &&
 	echo "I changed this file" > dir1/a &&
 	git add dir1/a &&
-	test_invalid_cache_tree dir1/
+	cmp_cache_tree expect
 '
 
 test_expect_success 'update-index invalidates cache-tree' '
@@ -95,6 +131,14 @@ test_expect_success 'second commit has cache-tree' '
 	test_shallow_cache_tree
 '
 
+test_expect_success 'commit in child dir has cache-tree' '
+	mkdir dir &&
+	>dir/child.t &&
+	git add dir/child.t &&
+	git commit -m dir/child.t
+	test_shallow_cache_tree
+'
+
 test_expect_success 'reset --hard gives cache-tree' '
 	test-scrap-cache-tree &&
 	git reset --hard &&
@@ -125,4 +169,15 @@ test_expect_success 'checkout -B gives cache-tree' '
 	test_shallow_cache_tree
 '
 
+test_expect_success 'partial commit gives cache-tree' '
+	git checkout -b partial no-children &&
+	test_commit one &&
+	test_commit two &&
+	echo "some change" > one.t &&
+	git add one.t &&
+	echo "some other change" > two.t &&
+	git commit two.t -m partial &&
+	test_shallow_cache_tree
+'
+
 test_done
-- 
2.0.0.390.gcb682f8

Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[Torsten Bögershausen]

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 6c33e28..7c60675 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' '
  	test_shallow_cache_tree
  '
  
-test_expect_failure 'checkout gives cache-tree' '
+test_expect_success 'checkout gives cache-tree' '
+	git tag current
  	git checkout HEAD^ &&
  	test_shallow_cache_tree

The && chainis broken here.
Does the test now pass, because "git tag" is added ?
In this case: does it may make sense the keep the old one as it is
an  add a new test case  like this ?

+test_expect_success 'tag and checkout gives cache-tree' '

[]

Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[Torsten Bögershausen]

[]

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 5383258..d50acb8 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -16,8 +16,31 @@ cmp_cache_tree () {
  # We don't bother with actually checking the SHA1:
  # test-dump-cache-tree already verifies that all existing data is
  # correct.
+generate_expected_cache_tree () {
+	if [ -n "$1" ]
+	then
+		local dir="$1/"
+	else
+		local dir="$1"
+	fi

I think the Git test cases prefer "test" over "[]":

	if test -n "$1"
	then
		local dir="$1/"
	else
		local dir="$1"
	fi

And should there be a && after the "fi" ?
And as test -n tests for a non-zero string,
could we write like this (and drop the local ?)?

	if test -n "$1"
	then
		dir="$1/"
	else
		dir=""
	fi

Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code

[Eric Sunshine]

On Mon, Jun 30, 2014 at 8:13 PM, David Turner <dturner@twopensource.com> wrote:
> Make test-dump-cache-tree more useful for testing.  Do not treat known
> invalid trees as errors (and do not produce non-zero exit code),
> because we can fall back to the non-cache-tree codepath.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  t/t0090-cache-tree.sh  | 28 +++++++++++++++++++++++++---
>  test-dump-cache-tree.c | 24 ++++++++++++------------
>  2 files changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 7c60675..5383258 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -21,10 +21,13 @@ test_shallow_cache_tree () {
>         cmp_cache_tree expect
>  }
>
> +# Test that the cache-tree for a given directory is invalid.
> +# If no directory is given, check that the root is invalid
>  test_invalid_cache_tree () {
> -       echo "invalid                                   (0 subtrees)" >expect &&
> -       printf "SHA #(ref)  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >>expect &&
> -       cmp_cache_tree expect
> +       test-dump-cache-tree >actual &&
> +       sed -e "s/$_x40/SHA/" -e "s/[0-9]* subtrees//g" <actual >filtered &&
> +       expect=$(printf "invalid                                  $1 ()\n") &&
> +       fgrep "$expect" filtered
>  }
>
>  test_no_cache_tree () {
> @@ -49,6 +52,25 @@ test_expect_success 'git-add invalidates cache-tree' '
>         test_invalid_cache_tree
>  '
>
> +test_expect_success 'git-add in subdir invalidates cache-tree' '
> +       test_when_finished "git reset --hard; git read-tree HEAD" &&
> +       mkdir dirx &&
> +       echo "I changed this file" > dirx/foo &&

Style: drop whitespace after >

> +       git add dirx/foo &&
> +       test_invalid_cache_tree
> +'
> +
> +test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '
> +       git tag no-children

Broken &&-chain.

> +       test_when_finished "git reset --hard no-children; git read-tree HEAD" &&
> +       mkdir dir1 dir2 &&
> +       test_commit dir1/a &&
> +       test_commit dir2/b &&
> +       echo "I changed this file" > dir1/a &&

Style: drop whitespace after >

> +       git add dir1/a &&
> +       test_invalid_cache_tree dir1/
> +'
> +
>  test_expect_success 'update-index invalidates cache-tree' '
>         test_when_finished "git reset --hard; git read-tree HEAD" &&
>         echo "I changed this file" > foo &&
> diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
> index 47eab97..ad42ca1 100644
> --- a/test-dump-cache-tree.c
> +++ b/test-dump-cache-tree.c
> @@ -6,12 +6,12 @@
>  static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
>  {
>         if (it->entry_count < 0)
> -               printf("%-40s %s%s (%d subtrees)\n",
> -                      "invalid", x, pfx, it->subtree_nr);
> +               printf("%-40s %s (%d subtrees)%s\n",
> +                      "invalid", pfx, it->subtree_nr, x);
>         else
> -               printf("%s %s%s (%d entries, %d subtrees)\n",
> -                      sha1_to_hex(it->sha1), x, pfx,
> -                      it->entry_count, it->subtree_nr);
> +               printf("%s %s (%d entries, %d subtrees)%s\n",
> +                      sha1_to_hex(it->sha1), pfx,
> +                      it->entry_count, it->subtree_nr, x);
>  }
>
>  static int dump_cache_tree(struct cache_tree *it,
> @@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,
>                 /* missing in either */
>                 return 0;
>
> -       if (it->entry_count < 0) {
> +       if (it->entry_count < 0)
> +               /* invalid */
>                 dump_one(it, pfx, "");
> -               dump_one(ref, pfx, "#(ref) ");
> -               if (it->subtree_nr != ref->subtree_nr)
> -                       errs = 1;
> -       }
>         else {
> -               dump_one(it, pfx, "");
>                 if (hashcmp(it->sha1, ref->sha1) ||
>                     ref->entry_count != it->entry_count ||
>                     ref->subtree_nr != it->subtree_nr) {
> -                       dump_one(ref, pfx, "#(ref) ");
> +                       /* claims to be valid but is lying */
> +                       dump_one(ref, pfx, " #(error)");
>                         errs = 1;
> +               } else {
> +                       /* is actually valid */
> +                       dump_one(it, pfx, "");
>                 }
>         }
>
> --
> 2.0.0.390.gcb682f8
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[Johannes Sixt]

Am 7/1/2014 6:26, schrieb Torsten Bögershausen:
> And as test -n tests for a non-zero string,
> could we write like this (and drop the local ?)?
> 
>     if test -n "$1"
>     then
>         dir="$1/"
>     else
>         dir=""
>     fi

These six lines can be written as

	dir="$1${1:+/}" &&

and 'local' must be dropped because is not universally supported, yet.

-- Hannes

Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[David Turner]

On Tue, 2014-07-01 at 06:16 +0200, Torsten Bögershausen wrote:
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 6c33e28..7c60675 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' '
>   	test_shallow_cache_tree
>   '
>   
> -test_expect_failure 'checkout gives cache-tree' '
> +test_expect_success 'checkout gives cache-tree' '
> +	git tag current
>   	git checkout HEAD^ &&
>   	test_shallow_cache_tree
> 
> The && chainis broken here.
> Does the test now pass, because "git tag" is added ?

The tag does not cause the cache-tree to be created, so git tag does not
cause the test to pass.

Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> When git checkout checks out a branch, create or update the
> cache-tree so that subsequent operations are faster.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  builtin/checkout.c    |  8 ++++++++
>  cache-tree.c          |  5 +++--
>  t/t0090-cache-tree.sh | 15 ++++++++++++++-
>  3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 07cf555..a023a86 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  		}
>  	}
>  
> +	if (!active_cache_tree)
> +		active_cache_tree = cache_tree();
> +
> +	if (!cache_tree_fully_valid(active_cache_tree))
> +		cache_tree_update(active_cache_tree,
> +				  (const struct cache_entry * const *)active_cache,
> +				  active_nr, 0);
> +

This looks much better than the previous round, but it still does
allow verify_cache() to throw noises against unmerged entries in the
cache, as WRITE_TREE_SILENT flag is not passed down, no?

	$ git checkout master^0
        $ git am $this_message
        $ make
        $ edit builtin/checkout.c ;# make changes to the above lines
        $ ./git checkout -m master^0
x       builtin/checkout.c: unmerged (972c8a7b28f16f88475268f9a714048c228e69db)
x       builtin/checkout.c: unmerged (f1dc56e55f7b2200412142b10517458ccfda2952)
x       builtin/checkout.c: unmerged (3b9753ba8c19e7dfe6e922f30eb85c83a92a4596)
        M       builtin/checkout.c
        Warning: you are leaving 1 commit behind, not connected to
        any of your branches:

          25fab54 cache-tree: Create/update cache-tree on checkout

        Switched to branch 'master'

Passing WRITE_TREE_SILENT in the flags parameter will get rid of the
conflict notice output from the above.

The user is not interested in writing a brand new tree object at all
in this case, so it feels wrong to actually let the call chain go
down to update_one() and create new tree objects.

	Side note.  And passing WRITE_TREE_DRY_RUN is not a good
	solution either, because a later write_cache_as_tree() will
	not create the necessary tree object once you stuff a tree
	object name in the cache-tree.

What we want in this code path is a way to repair a sub cache_tree
if it can be repaired without creating a new tree object and
otherwise leave that part invalid.  The existing cache-tree
framework is not prepared to do that kind of thing.  It wants to
start from the bottom and percolate things up, computing levels
nearer to the top-level only after it fully created the trees for
deeper levels, because it is meant to be used only when we really
want to write out trees.  We may want to reuse update_one() but

I am not convinced that doing an equivalent of write-tree when you
switch branches is the right approach in the first place.  You will
eventually write it out as a tree, and having a relatively undamaged
cache-tree will help you when you do so, but spending the cycles
necessary to compute a fully populated cache-tree, only to let it
degrade over time until you are ready to write it out as a tree,
somehow sounds like asking for a duplicated work upfront.

By the way, you seem to have touched write_cache_as_tree() in the
same patch, but I am not sure what makes the change necessary.  I do
not see a new caller to it that passes a NULL to its sha1 parameter.

> diff --git a/cache-tree.c b/cache-tree.c
> index 7fa524a..28d2356 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -612,9 +612,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
>  			cache_tree_find(active_cache_tree, prefix);
>  		if (!subtree)
>  			return WRITE_TREE_PREFIX_ERROR;
> -		hashcpy(sha1, subtree->sha1);
> +		if (sha1)
> +			hashcpy(sha1, subtree->sha1);
>  	}
> -	else
> +	else if (sha1)
>  		hashcpy(sha1, active_cache_tree->sha1);
>  
>  	if (0 <= newfd)
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 6c33e28..7c60675 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' '
>  	test_shallow_cache_tree
>  '
>  
> -test_expect_failure 'checkout gives cache-tree' '
> +test_expect_success 'checkout gives cache-tree' '
> +	git tag current
>  	git checkout HEAD^ &&
>  	test_shallow_cache_tree
>  '
>  
> +test_expect_success 'checkout -b gives cache-tree' '
> +	git checkout current &&
> +	git checkout -b prev HEAD^ &&
> +	test_shallow_cache_tree
> +'
> +
> +test_expect_success 'checkout -B gives cache-tree' '
> +	git checkout current &&
> +	git checkout -B prev HEAD^ &&
> +	test_shallow_cache_tree
> +'
> +
>  test_done

Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> On Tue, 2014-07-01 at 06:16 +0200, Torsten Bögershausen wrote:
>> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
>> index 6c33e28..7c60675 100755
>> --- a/t/t0090-cache-tree.sh
>> +++ b/t/t0090-cache-tree.sh
>> @@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' '
>>   	test_shallow_cache_tree
>>   '
>>   
>> -test_expect_failure 'checkout gives cache-tree' '
>> +test_expect_success 'checkout gives cache-tree' '
>> +	git tag current
>>   	git checkout HEAD^ &&
>>   	test_shallow_cache_tree
>> 
>> The && chainis broken here.
>> Does the test now pass, because "git tag" is added ?
>
> The tag does not cause the cache-tree to be created, so git tag does not
> cause the test to pass.

That does not explain why it is a good idea to declare success of
this test if this new "git tag current" fails here for whatever
reason (e.g. somebody updated "git tag" for a reason that is
completely unrelated to cache-tree and made it segfault without
creating the "current" tag).

Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[David Turner]

On Tue, 2014-07-01 at 14:03 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > On Tue, 2014-07-01 at 06:16 +0200, Torsten Bögershausen wrote:
> >> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> >> index 6c33e28..7c60675 100755
> >> --- a/t/t0090-cache-tree.sh
> >> +++ b/t/t0090-cache-tree.sh
> >> @@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' '
> >>   	test_shallow_cache_tree
> >>   '
> >>   
> >> -test_expect_failure 'checkout gives cache-tree' '
> >> +test_expect_success 'checkout gives cache-tree' '
> >> +	git tag current
> >>   	git checkout HEAD^ &&
> >>   	test_shallow_cache_tree
> >> 
> >> The && chainis broken here.
> >> Does the test now pass, because "git tag" is added ?
> >
> > The tag does not cause the cache-tree to be created, so git tag does not
> > cause the test to pass.
> 
> That does not explain why it is a good idea to declare success of
> this test if this new "git tag current" fails here for whatever
> reason (e.g. somebody updated "git tag" for a reason that is
> completely unrelated to cache-tree and made it segfault without
> creating the "current" tag).

Indeed; that's why the latest version includes &&.

Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[David Turner]

On Tue, 2014-07-01 at 13:15 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > When git checkout checks out a branch, create or update the
> > cache-tree so that subsequent operations are faster.
> >
> > Signed-off-by: David Turner <dturner@twitter.com>
> > ---
> >  builtin/checkout.c    |  8 ++++++++
> >  cache-tree.c          |  5 +++--
> >  t/t0090-cache-tree.sh | 15 ++++++++++++++-
> >  3 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index 07cf555..a023a86 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts *opts,
> >  		}
> >  	}
> >  
> > +	if (!active_cache_tree)
> > +		active_cache_tree = cache_tree();
> > +
> > +	if (!cache_tree_fully_valid(active_cache_tree))
> > +		cache_tree_update(active_cache_tree,
> > +				  (const struct cache_entry * const *)active_cache,
> > +				  active_nr, 0);
> > +
> 
> This looks much better than the previous round, but it still does
> allow verify_cache() to throw noises against unmerged entries in the
> cache, as WRITE_TREE_SILENT flag is not passed down, no?
> 
> 	$ git checkout master^0
>         $ git am $this_message
>         $ make
>         $ edit builtin/checkout.c ;# make changes to the above lines
>         $ ./git checkout -m master^0
> x       builtin/checkout.c: unmerged (972c8a7b28f16f88475268f9a714048c228e69db)
> x       builtin/checkout.c: unmerged (f1dc56e55f7b2200412142b10517458ccfda2952)
> x       builtin/checkout.c: unmerged (3b9753ba8c19e7dfe6e922f30eb85c83a92a4596)
>         M       builtin/checkout.c
>         Warning: you are leaving 1 commit behind, not connected to
>         any of your branches:
> 
>           25fab54 cache-tree: Create/update cache-tree on checkout
> 
>         Switched to branch 'master'
> 
> Passing WRITE_TREE_SILENT in the flags parameter will get rid of the
> conflict notice output from the above.
> 
> The user is not interested in writing a brand new tree object at all
> in this case, so it feels wrong to actually let the call chain go
> down to update_one() and create new tree objects.
> 
> 	Side note.  And passing WRITE_TREE_DRY_RUN is not a good
> 	solution either, because a later write_cache_as_tree() will
> 	not create the necessary tree object once you stuff a tree
> 	object name in the cache-tree.
> 
> What we want in this code path is a way to repair a sub cache_tree
> if it can be repaired without creating a new tree object and
> otherwise leave that part invalid.  The existing cache-tree
> framework is not prepared to do that kind of thing.  It wants to
> start from the bottom and percolate things up, computing levels
> nearer to the top-level only after it fully created the trees for
> deeper levels, because it is meant to be used only when we really
> want to write out trees.  We may want to reuse update_one() but
> 
> I am not convinced that doing an equivalent of write-tree when you
> switch branches is the right approach in the first place.  You will
> eventually write it out as a tree, and having a relatively undamaged
> cache-tree will help you when you do so, but spending the cycles
> necessary to compute a fully populated cache-tree, only to let it
> degrade over time until you are ready to write it out as a tree,
> somehow sounds like asking for a duplicated work upfront.

As I understand it, the cache-tree extension was originally designed to
speed up writing the tree later.  However, as Karsten Blees's work (and
my own tests) have shown, it also speeds up git status.  I use git
status a lot while working, and I've talked to a few others who do the
same.  So I think it's worth spending extra time when switching branches
to have a good working experience within that branch.

In the new version of the patchset (which I'll post shortly), I've added
an option WRITE_TREE_REPAIR, which does all of the work to compute a new
tree object, but only adds it to the cache-tree if it already exists
on-disk.  This is a little wasteful for the reason that you note.  So if
you would like, I could add a config option to skip it.  But I think it
is a good default.

Does this seem OK to you, assuming the implementation is good? 

Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

>> I am not convinced that doing an equivalent of write-tree when you
>> switch branches is the right approach in the first place.  You will
>> eventually write it out as a tree, and having a relatively undamaged
>> cache-tree will help you when you do so, but spending the cycles
>> necessary to compute a fully populated cache-tree, only to let it
>> degrade over time until you are ready to write it out as a tree,
>> somehow sounds like asking for a duplicated work upfront.
>
> As I understand it, the cache-tree extension was originally designed to
> speed up writing the tree later.  However, as Karsten Blees's work (and
> my own tests) have shown, it also speeds up git status.  I use git
> status a lot while working, and I've talked to a few others who do the
> same.  So I think it's worth spending extra time when switching branches
> to have a good working experience within that branch.

You are reading the history of the subsystem correctly.  Since
b65982b6 (Optimize "diff-index --cached" using cache-tree,
2009-05-20), having an undamaged cache-tree does help with "git
status" as well.

> In the new version of the patchset (which I'll post shortly), I've added
> an option WRITE_TREE_REPAIR, which does all of the work to compute a new
> tree object, but only adds it to the cache-tree if it already exists
> on-disk.  This is a little wasteful for the reason that you note.  So if
> you would like, I could add a config option to skip it.  But I think it
> is a good default.

OK, sounds good.

Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[David Turner]

On Tue, 2014-07-01 at 15:45 -0700, Junio C Hamano wrote:
> I didn't notice it when I gave the previous review comment but while
> reviewing this round, we already do the cache-tree population for
> "commit -a" in this function, which suggests me that it is the right
> place to do these changes.  Modulo minor niggles, I like this
> iteration much better than the previous one.
> 
> Thanks for working on this.

Thanks for all your comments.  I'll send out a v4 when I can, but I
probably won't have more time to work on this until next week. 

Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> During the commit process, the cache-tree is updated. We need to write
> this updated cache-tree so that it's ready for subsequent commands.
>
> Add test code which demonstrates that git commit now writes the cache
> tree.  Also demonstrate that cache-tree invalidation is correct.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  builtin/commit.c      | 15 ++++++------
>  t/t0090-cache-tree.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 67 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 9cfef6c..dbd4f4b 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
>  
>  		discard_cache();
>  		read_cache_from(index_lock.filename);
> +		if (update_main_cache_tree(WRITE_TREE_SILENT) >= 0)
> +			write_cache(fd, active_cache, active_nr);

OK, interactive-add may leave the cache-tree not quite populated;
we are going to write out a tree from the cache so we need to update
the in-core cache tree anyway, so calling update-main-cache-tree
here would not hurt (it will speed up the write-cache-as-tree we
will eventually call).

We might want to see if we are really changing anything, though.
What happens if the interactive-add gave us an index with fully
valid cache-tree?  Is that rare enough not to matter (not a
rhetorical question)?

> @@ -383,14 +385,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
>  	if (!only && !pathspec.nr) {
>  		fd = hold_locked_index(&index_lock, 1);
>  		refresh_cache_or_die(refresh_flags);
> -		if (active_cache_changed) {
> -			update_main_cache_tree(WRITE_TREE_SILENT);
> -			if (write_cache(fd, active_cache, active_nr) ||
> -			    commit_locked_index(&index_lock))
> -				die(_("unable to write new_index file"));
> -		} else {
> -			rollback_lock_file(&index_lock);
> -		}
> +		update_main_cache_tree(WRITE_TREE_SILENT);
> +		if (write_cache(fd, active_cache, active_nr) ||
> +		    commit_locked_index(&index_lock))
> +			die(_("unable to write new_index file"));


How about doing this part like the following instead, so that we can
avoid the overhead of uselessly rewriting the index file when we do
not have to?

diff --git a/builtin/commit.c b/builtin/commit.c
index 5e2221c..7d730a5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -383,8 +383,11 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	if (!only && !pathspec.nr) {
 		fd = hold_locked_index(&index_lock, 1);
 		refresh_cache_or_die(refresh_flags);
-		if (active_cache_changed) {
+		if (active_cache_changed || !cache_tree_fully_valid(active_cache_tree)) {
 			update_main_cache_tree(WRITE_TREE_SILENT);
+			active_cache_changed = 1;
+		}
+		if (active_cache_changed) {
 			if (write_cache(fd, active_cache, active_nr) ||
 			    commit_locked_index(&index_lock))
 				die(_("unable to write new_index file"));

It makes me wonder if we should teach update_main_cache_tree() to
somehow smudge active_cache_changed bit as necessary.  Then we do
not have to make the call to update-main-cache-tree conditional.

> @@ -435,6 +433,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
>  	fd = hold_locked_index(&index_lock, 1);
>  	add_remove_files(&partial);
>  	refresh_cache(REFRESH_QUIET);
> +	update_main_cache_tree(WRITE_TREE_SILENT);
>  	if (write_cache(fd, active_cache, active_nr) ||
>  	    close_lock_file(&index_lock))
>  		die(_("unable to write new_index file"));

This is the index that will be used after we create the commit
(which will be created from a temporary index that will be discarded
immediately after we create the commit).  As we _know_ we are
changing something in this code path by calling add_remote_files(),
it is fine to call update-main-cache-tree here unconditionally.

I didn't notice it when I gave the previous review comment but while
reviewing this round, we already do the cache-tree population for
"commit -a" in this function, which suggests me that it is the right
place to do these changes.  Modulo minor niggles, I like this
iteration much better than the previous one.

Thanks for working on this.

[PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[David Turner]

During the commit process, the cache-tree is updated. We need to write
this updated cache-tree so that it's ready for subsequent commands.

Add test code which demonstrates that git commit now writes the cache
tree.  Also demonstrate that cache-tree invalidation is correct.

Signed-off-by: David Turner <dturner@twitter.com>
---
 builtin/commit.c      | 15 ++++++------
 t/t0090-cache-tree.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..dbd4f4b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 
 		discard_cache();
 		read_cache_from(index_lock.filename);
+		if (update_main_cache_tree(WRITE_TREE_SILENT) >= 0)
+			write_cache(fd, active_cache, active_nr);
 
 		commit_style = COMMIT_NORMAL;
 		return index_lock.filename;
@@ -383,14 +385,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	if (!only && !pathspec.nr) {
 		fd = hold_locked_index(&index_lock, 1);
 		refresh_cache_or_die(refresh_flags);
-		if (active_cache_changed) {
-			update_main_cache_tree(WRITE_TREE_SILENT);
-			if (write_cache(fd, active_cache, active_nr) ||
-			    commit_locked_index(&index_lock))
-				die(_("unable to write new_index file"));
-		} else {
-			rollback_lock_file(&index_lock);
-		}
+		update_main_cache_tree(WRITE_TREE_SILENT);
+		if (write_cache(fd, active_cache, active_nr) ||
+		    commit_locked_index(&index_lock))
+			die(_("unable to write new_index file"));
 		commit_style = COMMIT_AS_IS;
 		return get_index_file();
 	}
@@ -435,6 +433,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	fd = hold_locked_index(&index_lock, 1);
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
+	update_main_cache_tree(WRITE_TREE_SILENT);
 	if (write_cache(fd, active_cache, active_nr) ||
 	    close_lock_file(&index_lock))
 		die(_("unable to write new_index file"));
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 8437c5f..625157e 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -13,11 +13,35 @@ cmp_cache_tree () {
 	test_cmp "$1" filtered
 }
 
+grep_nonmatch_ok () {
+    grep $@
+    test "$?" = "2" && return 1
+    return 0
+}
+
 # We don't bother with actually checking the SHA1:
 # test-dump-cache-tree already verifies that all existing data is
 # correct.
+generate_expected_cache_tree () {
+	dir="$1${1:+/}" &&
+	parent="$2" &&
+	# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
+	# We want to count only foo because it's the only direct child
+	subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&
+	subtree_count=$(echo "$subtrees"|grep_nonmatch_ok -c .) &&
+	entries=$(git ls-files|wc -l) &&
+	printf "SHA $dir (%d entries, %d subtrees)\n" $entries $subtree_count &&
+	for subtree in $subtrees
+	do
+	    cd "$subtree"
+	    generate_expected_cache_tree "$dir$subtree" $dir || return 1
+	    cd ..
+	done &&
+	dir=$parent
+}
+
 test_shallow_cache_tree () {
-	printf "SHA  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect &&
+	generate_expected_cache_tree >expect &&
 	cmp_cache_tree expect
 }
 
@@ -35,7 +59,7 @@ test_no_cache_tree () {
 	cmp_cache_tree expect
 }
 
-test_expect_failure 'initial commit has cache-tree' '
+test_expect_success 'initial commit has cache-tree' '
 	test_commit foo &&
 	test_shallow_cache_tree
 '
@@ -60,6 +84,18 @@ test_expect_success 'git-add in subdir invalidates cache-tree' '
 	test_invalid_cache_tree
 '
 
+cat >before <<\EOF
+SHA  (3 entries, 2 subtrees)
+SHA dir1/ (1 entries, 0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
+cat >expect <<\EOF
+invalid                                   (2 subtrees)
+invalid                                  dir1/ (0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
 test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '
 	git tag no-children &&
 	test_when_finished "git reset --hard no-children; git read-tree HEAD" &&
@@ -67,8 +103,10 @@ test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '
 	test_commit dir1/a &&
 	test_commit dir2/b &&
 	echo "I changed this file" >dir1/a &&
+	cmp_cache_tree before &&
+	echo "I changed this file" >dir1/a &&
 	git add dir1/a &&
-	test_invalid_cache_tree dir1/
+	cmp_cache_tree expect
 '
 
 test_expect_success 'update-index invalidates cache-tree' '
@@ -95,6 +133,14 @@ test_expect_success 'second commit has cache-tree' '
 	test_shallow_cache_tree
 '
 
+test_expect_success 'commit in child dir has cache-tree' '
+	mkdir dir &&
+	>dir/child.t &&
+	git add dir/child.t &&
+	git commit -m dir/child.t &&
+	test_shallow_cache_tree
+'
+
 test_expect_success 'reset --hard gives cache-tree' '
 	test-scrap-cache-tree &&
 	git reset --hard &&
@@ -125,4 +171,15 @@ test_expect_success 'checkout -B gives cache-tree' '
 	test_shallow_cache_tree
 '
 
+test_expect_success 'partial commit gives cache-tree' '
+	git checkout -b partial no-children &&
+	test_commit one &&
+	test_commit two &&
+	echo "some change" >one.t &&
+	git add one.t &&
+	echo "some other change" >two.t &&
+	git commit two.t -m partial &&
+	test_shallow_cache_tree
+'
+
 test_done
-- 
2.0.0.390.gcb682f8

Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> During the commit process, the cache-tree is updated. We need to write
> this updated cache-tree so that it's ready for subsequent commands.
>
> Add test code which demonstrates that git commit now writes the cache
> tree.  Also demonstrate that  cache-tree invalidation is correct.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  builtin/commit.c      |  6 ++++++
>  t/t0090-cache-tree.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 9cfef6c..6814e87 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -607,6 +607,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	const char *hook_arg2 = NULL;
>  	int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
>  	int old_display_comment_prefix;
> +	static struct lock_file index_lock;
> +	int index_fd;
>  
>  	/* This checks and barfs if author is badly specified */
>  	determine_author_info(author_ident);
> @@ -872,6 +874,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		error(_("Error building trees"));
>  		return 0;
>  	}
> +	/* After updating the cache-tree, rewrite the index */
> +	index_fd = hold_locked_index(&index_lock, 0);
> +	if (index_fd >= 0 && write_index(&the_index, index_fd) >= 0)
> +		commit_locked_index(&index_lock);

Is this run unconditionally even when we are making a partial commit
out of a temporary index file (which will be discarded after we
create this commit)?

I have a feeling that a better place to populate the cache-tree may
be prepare-index which knows the distinction between various modes
of commit, but I haven't looked at the code path for a while...

[PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[David Turner]

During the commit process, the cache-tree is updated. We need to write
this updated cache-tree so that it's ready for subsequent commands.

Add test code which demonstrates that git commit now writes the cache
tree.  Also demonstrate that  cache-tree invalidation is correct.

Signed-off-by: David Turner <dturner@twitter.com>
---
 builtin/commit.c      |  6 ++++++
 t/t0090-cache-tree.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..6814e87 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -607,6 +607,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	const char *hook_arg2 = NULL;
 	int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
 	int old_display_comment_prefix;
+	static struct lock_file index_lock;
+	int index_fd;
 
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
@@ -872,6 +874,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		error(_("Error building trees"));
 		return 0;
 	}
+	/* After updating the cache-tree, rewrite the index */
+	index_fd = hold_locked_index(&index_lock, 0);
+	if (index_fd >= 0 && write_index(&the_index, index_fd) >= 0)
+		commit_locked_index(&index_lock);
 
 	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
 			    git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 5383258..ef012b9 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -16,8 +16,31 @@ cmp_cache_tree () {
 # We don't bother with actually checking the SHA1:
 # test-dump-cache-tree already verifies that all existing data is
 # correct.
+generate_expected_cache_tree () {
+	if [ -n "$1" ]
+	then
+		local dir="$1/"
+	else
+		local dir="$1"
+	fi
+	# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
+	# We want to count only foo because it's the only direct child
+	local subtrees=$(git ls-files|egrep '/' |cut -d / -f 1|uniq) &&
+	local subtree_count=$(echo "$subtrees"|grep -c .) &&
+	local files_count=$(git ls-files|grep -v /|wc -l) &&
+	local entries=$(expr "$subtree_count" + "$files_count") &&
+	printf "SHA $dir (%d entries, %d subtrees)\n" $entries $subtree_count &&
+	local subtree &&
+	for subtree in $subtrees
+	do
+	    cd "$subtree"
+	    generate_expected_cache_tree "$dir$subtree" || return 1
+	    cd ..
+	done
+}
+
 test_shallow_cache_tree () {
-	printf "SHA  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect &&
+	generate_expected_cache_tree > expect &&
 	cmp_cache_tree expect
 }
 
@@ -35,7 +58,7 @@ test_no_cache_tree () {
 	cmp_cache_tree expect
 }
 
-test_expect_failure 'initial commit has cache-tree' '
+test_expect_success 'initial commit has cache-tree' '
 	test_commit foo &&
 	test_shallow_cache_tree
 '
@@ -60,15 +83,28 @@ test_expect_success 'git-add in subdir invalidates cache-tree' '
 	test_invalid_cache_tree
 '
 
+cat >before <<\EOF
+SHA  (3 entries, 2 subtrees)
+SHA dir1/ (1 entries, 0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
+cat >expect <<\EOF
+invalid                                   (2 subtrees)
+invalid                                  dir1/ (0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
 test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '
 	git tag no-children
 	test_when_finished "git reset --hard no-children; git read-tree HEAD" &&
 	mkdir dir1 dir2 &&
 	test_commit dir1/a &&
 	test_commit dir2/b &&
+	cmp_cache_tree before &&
 	echo "I changed this file" > dir1/a &&
 	git add dir1/a &&
-	test_invalid_cache_tree dir1/
+	cmp_cache_tree expect
 '
 
 test_expect_success 'update-index invalidates cache-tree' '
@@ -95,6 +131,14 @@ test_expect_success 'second commit has cache-tree' '
 	test_shallow_cache_tree
 '
 
+test_expect_success 'commit in child dir has cache-tree' '
+	mkdir dir &&
+	>dir/child.t &&
+	git add dir/child.t &&
+	git commit -m dir/child.t
+	test_shallow_cache_tree
+'
+
 test_expect_success 'reset --hard gives cache-tree' '
 	test-scrap-cache-tree &&
 	git reset --hard &&
-- 
2.0.0.390.gcb682f8