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

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

[David Turner]

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

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

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

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

[David Turner]

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

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

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

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

[Eric Sunshine]

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

Style: drop whitespace after >

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

Broken &&-chain.

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

Style: drop whitespace after >

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

[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

[Torsten Bögershausen]

[]

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

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

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

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

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

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

[Johannes Sixt]

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

These six lines can be written as

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

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

-- Hannes

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

[Torsten Bögershausen]

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

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

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

[]

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

[David Turner]

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

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

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

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

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

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

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

[David Turner]

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

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

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

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

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

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

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

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

        Switched to branch 'master'

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

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

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

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

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

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

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

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

[David Turner]

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

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

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

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

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

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

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

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

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

OK, sounds good.

[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

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

[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    |  4 ++++
 cache-tree.c          | 22 ++++++++++++----------
 cache-tree.h          |  1 +
 t/t0090-cache-tree.sh | 15 ++++++++++++++-
 4 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07cf555..df791e8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -553,6 +553,10 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		}
 	}
 
+	if (write_cache_as_tree(NULL, WRITE_TREE_DO_NOT_WRITE, "")) {
+		warn("Unable to write cache_tree");
+	}
+
 	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..fb203c6 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -568,17 +568,18 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat
 
 int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 {
-	int entries, was_valid, newfd;
+	int entries, was_valid, newfd = -1;
 	struct lock_file *lock_file;
 
-	/*
-	 * We can't free this memory, it becomes part of a linked list
-	 * parsed atexit()
-	 */
-	lock_file = xcalloc(1, sizeof(struct lock_file));
-
-	newfd = hold_locked_index(lock_file, 1);
+	if (!(flags & WRITE_TREE_DO_NOT_WRITE)) {
+		/*
+		 * We can't free this memory, it becomes part of a linked list
+		 * parsed atexit()
+		 */
+		lock_file = xcalloc(1, sizeof(struct lock_file));
 
+		newfd = hold_locked_index(lock_file, 1);
+	}
 	entries = read_cache();
 	if (entries < 0)
 		return WRITE_TREE_UNREADABLE_INDEX;
@@ -612,9 +613,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/cache-tree.h b/cache-tree.h
index f1923ad..daf6640 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -39,6 +39,7 @@ int update_main_cache_tree(int);
 #define WRITE_TREE_IGNORE_CACHE_TREE 2
 #define WRITE_TREE_DRY_RUN 4
 #define WRITE_TREE_SILENT 8
+#define WRITE_TREE_DO_NOT_WRITE 16
 
 /* error return codes */
 #define WRITE_TREE_UNREADABLE_INDEX (-1)
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

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

[Duy Nguyen]

On Sat, Jun 28, 2014 at 7:20 AM, David Turner <dturner@twopensource.com> wrote:
> 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    |  4 ++++
>  cache-tree.c          | 22 ++++++++++++----------
>  cache-tree.h          |  1 +
>  t/t0090-cache-tree.sh | 15 ++++++++++++++-
>  4 files changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 07cf555..df791e8 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -553,6 +553,10 @@ static int merge_working_tree(const struct checkout_opts *opts,
>                 }
>         }
>
> +       if (write_cache_as_tree(NULL, WRITE_TREE_DO_NOT_WRITE, "")) {
> +               warn("Unable to write cache_tree");
> +       }
> +

I wonder if we should do this in !opts->force code path only. In the
opts->force code path we could use prime_cache_tree() (like
read-tree), which is supposedly faster (but may need some tests to be
sure). prime_cache_tree() could be made a bit faster by doing it
during tree traversal in unpack_trees() so we don't have to unpack any
tree objects twice.
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> I wonder if we should do this in !opts->force code path only. In the
> opts->force code path we could use prime_cache_tree() (like
> read-tree), which is supposedly faster...

Nobody sane should be constantly running "checkout -f", so even if
priming from existing tree objects were faster, it would be adding
complexity to the code to optimize for a wrong case.

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

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> When git checkout checks out a branch, create or update the
> cache-tree so that subsequent operations are faster.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  builtin/checkout.c    |  4 ++++
>  cache-tree.c          | 22 ++++++++++++----------
>  cache-tree.h          |  1 +
>  t/t0090-cache-tree.sh | 15 ++++++++++++++-
>  4 files changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 07cf555..df791e8 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -553,6 +553,10 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  		}
>  	}
>  
> +	if (write_cache_as_tree(NULL, WRITE_TREE_DO_NOT_WRITE, "")) {
> +		warn("Unable to write cache_tree");
> +	}
> +

This is not even warn-worthy event, isn't it?  "checkout -m other"
would attempt a three-way merge to replay your local changes
relative to your current HEAD in the context of "other" branch and
will leave conflicts in the index and in the working tree, and it is
perfectly a normal thing that you cannot write the index in such a
state as a tree object.  Perhaps you should check if the index is
unmerged before even attempting to compute the cache tree.

Also it looks very strange that write-cache-as-tree, whose *PRIMARY*
function is to write a tree, receives a "DO NOT WRITE" option here.
I understand that you want a new interface into the cache-tree
subsystem that only updates the cache-tree but it somehow smells
like a sloppy/lazy refactoring that is not done quite right.