[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 | 19 ++++++++++++++++---
 3 files changed, 27 insertions(+), 5 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..98fb1ab 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -44,14 +44,14 @@ test_expect_success 'read-tree HEAD establishes cache-tree' '
 
 test_expect_success 'git-add invalidates cache-tree' '
 	test_when_finished "git reset --hard; git read-tree HEAD" &&
-	echo "I changed this file" > foo &&
+	echo "I changed this file" >foo &&
 	git add foo &&
 	test_invalid_cache_tree
 '
 
 test_expect_success 'update-index invalidates cache-tree' '
 	test_when_finished "git reset --hard; git read-tree HEAD" &&
-	echo "I changed this file" > foo &&
+	echo "I changed this file" >foo &&
 	git update-index --add foo &&
 	test_invalid_cache_tree
 '
@@ -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 98fb1ab..8437c5f 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 | 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 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>
> ---

Could you number your patches e.g. [PATCH v4 1/3] and/or summarize
below the three-dash line what got updated since the last round?
The readers can guess without when one is sending a reroll every
other day or less frequently, but with rerolls more often than that,
it gets unwieldy to check which points raised during the review have
been addressed.

Thanks.

>  builtin/checkout.c    |  8 ++++++++
>  cache-tree.c          |  5 +++--
>  t/t0090-cache-tree.sh | 19 ++++++++++++++++---
>  3 files changed, 27 insertions(+), 5 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..98fb1ab 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -44,14 +44,14 @@ test_expect_success 'read-tree HEAD establishes cache-tree' '
>  
>  test_expect_success 'git-add invalidates cache-tree' '
>  	test_when_finished "git reset --hard; git read-tree HEAD" &&
> -	echo "I changed this file" > foo &&
> +	echo "I changed this file" >foo &&
>  	git add foo &&
>  	test_invalid_cache_tree
>  '
>  
>  test_expect_success 'update-index invalidates cache-tree' '
>  	test_when_finished "git reset --hard; git read-tree HEAD" &&
> -	echo "I changed this file" > foo &&
> +	echo "I changed this file" >foo &&
>  	git update-index --add foo &&
>  	test_invalid_cache_tree
>  '
> @@ -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 2/3] test-dump-cache-tree: Improve output format and exit code

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

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

Under-explained.  "more useful" is subjective so I won't complain
about it being not explained enough, but I cannot quite parse and
understand the second sentence.

It is not "we treat known invalid trees as errors".  I think what
you meant is "we insist that a cache-tree entry, even if it is an
invalidated one, must record the correct number of subtrees and
signal errors otherwise, which is wrong".

I honestly cannot guess what you meant to say by "because we can
...".

> 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);

I am guessing this is related to the "more useful", but I cannot
offhand tell what this output shuffling is about.  It would be
better to illustrate in the log message to support the "more useful"
claim by showing how improved/readable the output got with this
change.

>  }
>  
>  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) ");

Unfortunately this is not quite what we would want; this "#(ref)"
output is so that we can see what tree object we should be referring
to, while debugging, if this entry were not invalidated; losing that
does not "Improve output"---it loses information from the output.

> -		if (it->subtree_nr != ref->subtree_nr)
> -			errs = 1;

I am guessing that this is the change you did not explain quite
enough with "do not treat ... as errors".  This code expects that
even an invalidated cache-tree entry knows how many subtrees it has,
which is unreasonable.  I do not recall offhand if we used to have
some code to ensure that such an invariant holds or not, but when
invalidating a directory (say "t/") by adding a new subdirectory and
a new file in it (e.g. "t/dir/file") to the index, we do not do
anything other than to invalidate "t/" and "t/dir/", and I do not
think the codepath recounts the number of subdirectories to adjust
subtree_nr in any way to match the reality, so removal of this check
is the right thing to do.

> -	}
>  	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, "");
>  		}
>  	}

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.

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

[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 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

[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 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