threads / patch / 37034

patch, 3 partscache-tree: Create/update cache-tree on checkout

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

## tl;dr

7 messages between Jul 1, 2014 and Jul 1, 2014. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

David Turner· Jul 1, 2014, 19:14 UTC · lore

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

Show changes to 3 files +27 −5

builtin/checkout.c, cache-tree.c, t/t0090-cache-tree.sh

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

David Turner· Jul 1, 2014, 19:14 UTC · re: David Turner · lore

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

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

Show changes to 2 files +37 −15

t/t0090-cache-tree.sh, test-dump-cache-tree.c

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

Junio C Hamano· Jul 1, 2014, 21:42 UTC · re: David Turner · lore

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

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

Show 19 quoted lines

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

Show 12 quoted lines

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

Show 15 quoted lines

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

David Turner· Jul 1, 2014, 19:14 UTC · re: David Turner · lore

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

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

Show changes to 2 files +67 −11

builtin/commit.c, t/t0090-cache-tree.sh

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

Junio C Hamano· Jul 1, 2014, 22:45 UTC · re: David Turner · lore

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

David Turner <dturner@twopensource.com> writes:

Show 22 quoted lines

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

Show 16 quoted lines

> @@ -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?

Show changes to builtin/commit.c +4 −1

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.

David Turner· Jul 1, 2014, 22:58 UTC · re: Junio C Hamano · lore

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

On Tue, 2014-07-01 at 15:45 -0700, Junio C Hamano wrote:

Show 7 quoted lines

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

Junio C Hamano· Jul 1, 2014, 21:08 UTC · re: David Turner · lore

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

David Turner <dturner@twopensource.com> writes:

Show 5 quoted lines

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

Show 86 quoted lines

>  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