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

13 messages from 2014-07-01 to 2014-07-07. Participants: David Turner, Torsten Bögershausen, Eric Sunshine, Johannes Sixt, Junio C Hamano.
Thread: https://gitlist.dev/t/37031

## David Turner, 2014-07-01 00:13

Subject: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout
Message-ID: <1404173597-24713-1-git-send-email-dturner@twitter.com>
URL: https://gitlist.dev/e/1404173597-24713-1-git-send-email-dturner%40twitter.com

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

```

## David Turner, 2014-07-01 00:13

Subject: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code
Message-ID: <1404173597-24713-2-git-send-email-dturner@twitter.com>
URL: https://gitlist.dev/e/1404173597-24713-2-git-send-email-dturner%40twitter.com
In-Reply-To: <1404173597-24713-1-git-send-email-dturner@twitter.com>

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

```

## David Turner, 2014-07-01 00:13

Subject: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit
Message-ID: <1404173597-24713-3-git-send-email-dturner@twitter.com>
URL: https://gitlist.dev/e/1404173597-24713-3-git-send-email-dturner%40twitter.com
In-Reply-To: <1404173597-24713-1-git-send-email-dturner@twitter.com>

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

```

## Torsten Bögershausen, 2014-07-01 04:16

Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout
Message-ID: <53B23605.40705@web.de>
URL: https://gitlist.dev/e/53B23605.40705%40web.de
In-Reply-To: <1404173597-24713-1-git-send-email-dturner@twitter.com>

```

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

[]

```

## Torsten Bögershausen, 2014-07-01 04:26

Subject: Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit
Message-ID: <53B2386B.4060802@web.de>
URL: https://gitlist.dev/e/53B2386B.4060802%40web.de
In-Reply-To: <1404173597-24713-3-git-send-email-dturner@twitter.com>

```
[]

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

```

## Eric Sunshine, 2014-07-01 04:43

Subject: Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code
Message-ID: <CAPig+cSKiB3b-gufaOx4=rWBjWWyGiqx-M85+2UAGq-BUkkLHA@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcSKiB3b-gufaOx4%3DrWBjWWyGiqx-M85%2B2UAGq-BUkkLHA%40mail.gmail.com
In-Reply-To: <1404173597-24713-2-git-send-email-dturner@twitter.com>

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

```

## Johannes Sixt, 2014-07-01 05:49

Subject: Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit
Message-ID: <53B24BF5.3020007@viscovery.net>
URL: https://gitlist.dev/e/53B24BF5.3020007%40viscovery.net
In-Reply-To: <53B2386B.4060802@web.de>

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

```

## David Turner, 2014-07-01 19:15

Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout
Message-ID: <1404242126.6112.7.camel@stross>
URL: https://gitlist.dev/e/1404242126.6112.7.camel%40stross
In-Reply-To: <53B23605.40705@web.de>

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

```

## Junio C Hamano, 2014-07-01 20:15

Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout
Message-ID: <xmqq8uocx2c5.fsf@gitster.dls.corp.google.com>
URL: https://gitlist.dev/e/xmqq8uocx2c5.fsf%40gitster.dls.corp.google.com
In-Reply-To: <1404173597-24713-1-git-send-email-dturner@twitter.com>

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

```

## Junio C Hamano, 2014-07-01 21:03

Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout
Message-ID: <xmqq4mz0x04l.fsf@gitster.dls.corp.google.com>
URL: https://gitlist.dev/e/xmqq4mz0x04l.fsf%40gitster.dls.corp.google.com
In-Reply-To: <1404242126.6112.7.camel@stross>

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

```

## David Turner, 2014-07-01 22:09

Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout
Message-ID: <1404252587.3109.1.camel@stross>
URL: https://gitlist.dev/e/1404252587.3109.1.camel%40stross
In-Reply-To: <xmqq4mz0x04l.fsf@gitster.dls.corp.google.com>

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

```

## David Turner, 2014-07-06 04:04

Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout
Message-ID: <1404619467.3109.38.camel@stross>
URL: https://gitlist.dev/e/1404619467.3109.38.camel%40stross
In-Reply-To: <xmqq8uocx2c5.fsf@gitster.dls.corp.google.com>

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

```

## Junio C Hamano, 2014-07-07 16:58

Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout
Message-ID: <xmqq4mytf6mx.fsf@gitster.dls.corp.google.com>
URL: https://gitlist.dev/e/xmqq4mytf6mx.fsf%40gitster.dls.corp.google.com
In-Reply-To: <1404619467.3109.38.camel@stross>

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

```