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

[PATCH 1/4 v6] 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.

update_main_cache_tree learned a new flag, WRITE_TREE_REPAIR.  When
WRITE_TREE_REPAIR is set, portions of the cache-tree which do not
correspond to existing tree objects are invalidated (and portions which
do are marked as valid).  No new tree objects are created.

Signed-off-by: David Turner <dturner@twitter.com>
---
 builtin/checkout.c    |  8 ++++++++
 cache-tree.c          | 12 +++++++++++-
 cache-tree.h          |  1 +
 t/t0090-cache-tree.sh | 19 ++++++++++++++++---
 4 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07cf555..054214f 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, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);
+
 	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..f951d7d 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -239,9 +239,12 @@ static int update_one(struct cache_tree *it,
 	struct strbuf buffer;
 	int missing_ok = flags & WRITE_TREE_MISSING_OK;
 	int dryrun = flags & WRITE_TREE_DRY_RUN;
+	int repair = flags & WRITE_TREE_REPAIR;
 	int to_invalidate = 0;
 	int i;
 
+	assert(!(dryrun && repair));
+
 	*skip_count = 0;
 
 	if (0 <= it->entry_count && has_sha1_file(it->sha1))
@@ -374,7 +377,14 @@ static int update_one(struct cache_tree *it,
 #endif
 	}
 
-	if (dryrun)
+	if (repair) {
+		unsigned char sha1[20];
+		hash_sha1_file(buffer.buf, buffer.len, tree_type, sha1);
+		if (has_sha1_file(sha1))
+			hashcpy(it->sha1, sha1);
+		else
+			to_invalidate = 1;
+	} else if (dryrun)
 		hash_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1);
 	else if (write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1)) {
 		strbuf_release(&buffer);
diff --git a/cache-tree.h b/cache-tree.h
index f1923ad..666d18f 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_REPAIR 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..98fb1ab 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -44,14 +44,14 @@ test_expect_success 'read-tree HEAD establishes cache-tree' '
 
 test_expect_success 'git-add invalidates cache-tree' '
 	test_when_finished "git reset --hard; git read-tree HEAD" &&
-	echo "I changed this file" > foo &&
+	echo "I changed this file" >foo &&
 	git add foo &&
 	test_invalid_cache_tree
 '
 
 test_expect_success 'update-index invalidates cache-tree' '
 	test_when_finished "git reset --hard; git read-tree HEAD" &&
-	echo "I changed this file" > foo &&
+	echo "I changed this file" >foo &&
 	git update-index --add foo &&
 	test_invalid_cache_tree
 '
@@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' '
 	test_shallow_cache_tree
 '
 
-test_expect_failure 'checkout gives cache-tree' '
+test_expect_success 'checkout gives cache-tree' '
+	git tag current &&
 	git checkout HEAD^ &&
 	test_shallow_cache_tree
 '
 
+test_expect_success 'checkout -b gives cache-tree' '
+	git checkout current &&
+	git checkout -b prev HEAD^ &&
+	test_shallow_cache_tree
+'
+
+test_expect_success 'checkout -B gives cache-tree' '
+	git checkout current &&
+	git checkout -B prev HEAD^ &&
+	test_shallow_cache_tree
+'
+
 test_done
-- 
2.0.0.390.gcb682f8

[PATCH 2/4 v6] test-dump-cache-tree: invalid trees are not errors

[David Turner]

Do not treat known-invalid trees as errors even when their subtree_nr is
incorrect.  Because git already knows that these trees are invalid,
an incorrect subtree_nr will not cause problems.

Add a couple of comments.

Signed-off-by: David Turner <dturner@twitter.com>
---
 test-dump-cache-tree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 47eab97..cbbbd8e 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -26,16 +26,16 @@ static int dump_cache_tree(struct cache_tree *it,
 		return 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) {
+			/* claims to be valid but is lying */
 			dump_one(ref, pfx, "#(ref) ");
 			errs = 1;
 		}
-- 
2.0.0.390.gcb682f8

[PATCH 3/4 v6] cache-tree: subdirectory tests

[David Turner]

Add tests to confirm that invalidation of subdirectories neither over-
nor under-invalidates.

Signed-off-by: David Turner <dturner@twitter.com>
---
 t/t0090-cache-tree.sh | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 98fb1ab..3a3342e 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -22,9 +22,10 @@ test_shallow_cache_tree () {
 }
 
 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
+	printf "invalid                                  %s ()\n" "" "$@" >expect &&
+	test-dump-cache-tree | \
+	sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p' >actual &&
+	test_cmp expect actual
 }
 
 test_no_cache_tree () {
@@ -49,6 +50,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 &&
-- 
2.0.0.390.gcb682f8

[PATCH 4/4 v6] cache-tree: Write updated cache-tree after commit

[David Turner]

During the commit process, update the cache-tree. 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.  Make all tests test the entire cache-tree, not just the root
level.

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

diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..99c9054 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,8 +385,12 @@ 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"));
@@ -435,6 +441,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 3a3342e..db7a8d0 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -8,7 +8,7 @@ cache-tree extension.
  . ./test-lib.sh
 
 cmp_cache_tree () {
-	test-dump-cache-tree >actual &&
+	test-dump-cache-tree | sed -e '/#(ref)/d' >actual &&
 	sed "s/$_x40/SHA/" <actual >filtered &&
 	test_cmp "$1" filtered
 }
@@ -16,8 +16,34 @@ cmp_cache_tree () {
 # We don't bother with actually checking the SHA1:
 # test-dump-cache-tree already verifies that all existing data is
 # correct.
-test_shallow_cache_tree () {
-	printf "SHA  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect &&
+generate_expected_cache_tree_rec () {
+	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"|awk '$1 {++c} END {print 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_rec "$dir$subtree" "$dir" || return 1
+	    cd ..
+	done &&
+	dir=$parent
+}
+
+generate_expected_cache_tree () {
+    cwd=$(pwd)
+    generate_expected_cache_tree_rec
+    ret="$?"
+    cd "$cwd"
+    return $ret
+}
+
+test_cache_tree () {
+	generate_expected_cache_tree >expect &&
 	cmp_cache_tree expect
 }
 
@@ -33,14 +59,14 @@ 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
+	test_cache_tree
 '
 
 test_expect_success 'read-tree HEAD establishes cache-tree' '
 	git read-tree HEAD &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'git-add invalidates cache-tree' '
@@ -58,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" &&
@@ -65,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' '
@@ -79,7 +119,7 @@ test_expect_success 'update-index invalidates cache-tree' '
 test_expect_success 'write-tree establishes cache-tree' '
 	test-scrap-cache-tree &&
 	git write-tree &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'test-scrap-cache-tree works' '
@@ -90,37 +130,56 @@ test_expect_success 'test-scrap-cache-tree works' '
 
 test_expect_success 'second commit has cache-tree' '
 	test_commit bar &&
-	test_shallow_cache_tree
+	test_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_cache_tree
 '
 
 test_expect_success 'reset --hard gives cache-tree' '
 	test-scrap-cache-tree &&
 	git reset --hard &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'reset --hard without index gives cache-tree' '
 	rm -f .git/index &&
 	git reset --hard &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'checkout gives cache-tree' '
 	git tag current &&
 	git checkout HEAD^ &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'checkout -b gives cache-tree' '
 	git checkout current &&
 	git checkout -b prev HEAD^ &&
-	test_shallow_cache_tree
+	test_cache_tree
 '
 
 test_expect_success 'checkout -B gives cache-tree' '
 	git checkout current &&
 	git checkout -B prev HEAD^ &&
-	test_shallow_cache_tree
+	test_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_cache_tree
 '
 
 test_done
-- 
2.0.0.390.gcb682f8

Re: [PATCH 3/4 v6] cache-tree: subdirectory tests

[Eric Sunshine]

On Thu, Jul 10, 2014 at 8:31 PM, David Turner <dturner@twopensource.com> wrote:
> Add tests to confirm that invalidation of subdirectories neither over-
> nor under-invalidates.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  t/t0090-cache-tree.sh | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 98fb1ab..3a3342e 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -22,9 +22,10 @@ test_shallow_cache_tree () {
>  }
>
>  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
> +       printf "invalid                                  %s ()\n" "" "$@" >expect &&
> +       test-dump-cache-tree | \

nit: unnecessary backslash

> +       sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p' >actual &&
> +       test_cmp expect actual
>  }
>
>  test_no_cache_tree () {
> @@ -49,6 +50,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 &&
> --
> 2.0.0.390.gcb682f8

Re: [PATCH 3/4 v6] cache-tree: subdirectory tests

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Jul 10, 2014 at 8:31 PM, David Turner <dturner@twopensource.com> wrote:
>> Add tests to confirm that invalidation of subdirectories neither over-
>> nor under-invalidates.
>>
>> Signed-off-by: David Turner <dturner@twitter.com>
>> ---
>>  t/t0090-cache-tree.sh | 26 +++++++++++++++++++++++---
>>  1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
>> index 98fb1ab..3a3342e 100755
>> --- a/t/t0090-cache-tree.sh
>> +++ b/t/t0090-cache-tree.sh
>> @@ -22,9 +22,10 @@ test_shallow_cache_tree () {
>>  }
>>
>>  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
>> +       printf "invalid                                  %s ()\n" "" "$@" >expect &&

Hmm.  This will always expect that the top-level is invalid, even
when $# is 0.  It is OK if you never need to use this to test that a
cache-tree is fully valid, but is it something we want to check?

Existing tests are mostly about "cache-tree is populated fully at a
few strategic, well known and easy places and then it degrades over
time", but after all your series is adding more places to that set
of "a few places", so we may want to make sure that future breakages
to the new code paths that "repair" the cache-tree are caught by
these tests.

>> +       test-dump-cache-tree | \
>
> nit: unnecessary backslash

Good eyes ;-)

>> +       sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p' >actual &&

Is the second one to remove "#(ref)", which appears for a good
"reference" cache tree entry shown for comparison, necessary?  Do
they ever begin with "invalid"?  If they ever begin with "invalid"
that itself may even be a noteworthy breakage to catch, no?

>> +       test_cmp expect actual
>>  }
>>
>>  test_no_cache_tree () {
>> @@ -49,6 +50,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 &&
>> --
>> 2.0.0.390.gcb682f8

Re: [PATCH 3/4 v6] cache-tree: subdirectory tests

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>> +       sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p' >actual &&
>
> Is the second one to remove "#(ref)", which appears for a good
> "reference" cache tree entry shown for comparison, necessary?  Do
> they ever begin with "invalid"?  If they ever begin with "invalid"
> that itself may even be a noteworthy breakage to catch, no?

Answering to myself...

Because test-dump-cache-tree uses DRY_RUN to create only an in-core
copy of tree object, and we notice that the reference cache-tree
created in the tests contains the object name of a tree that does
not yet exist in the object database.  We get "invalid #(ref)" for
such node.

In the ideal world, I think whoever tries to compare two cache-trees
(i.e. test-dump-cache-tree) should *not* care, because we are merely
trying to show what the correct tree object name for the node would
be, but this is only for testing, so the best way forward would be
to:

 - Stop using DRY_RUN in test-dump-cache-tree.c;

 - Stop the code to support DRY_RUN from cache-tree.c (nobody but
   the test uses it); and

 - Drop the "-e '#(ref)/d'" from the above.

I would think.

Re: [PATCH 4/4 v6] cache-tree: Write updated cache-tree after commit

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> @@ -16,8 +16,34 @@ cmp_cache_tree () {
>  # We don't bother with actually checking the SHA1:
>  # test-dump-cache-tree already verifies that all existing data is
>  # correct.

Is this statement now stale and needs to be removed?

> -test_shallow_cache_tree () {
> -	printf "SHA  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect &&
> +generate_expected_cache_tree_rec () {
> +	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"|awk '$1 {++c} END {print 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_rec "$dir$subtree" "$dir" || return 1
> +	    cd ..
> +	done &&
> +	dir=$parent
> +}
> +
> +generate_expected_cache_tree () {
> +    cwd=$(pwd)
> +    generate_expected_cache_tree_rec
> +    ret="$?"
> +    cd "$cwd"
> +    return $ret
> +}

As we always have had trouble between $PWD and $(pwd) on other
platforms, I'd prefer something simpler, like:

	expected_cache_tree () {
		(
                	# subshell to let it wander around freely
	        	generate_expected_cache_tree_rec
		)
	}

Other than these two, the new tests were good from a cursory look.
The change to builtin/commit.c looked good, too.

Thanks.

Re: [PATCH 3/4 v6] cache-tree: subdirectory tests

[David Turner]

On Fri, 2014-07-11 at 08:40 -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >>> +       sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p' >actual &&
> >
> > Is the second one to remove "#(ref)", which appears for a good
> > "reference" cache tree entry shown for comparison, necessary?  Do
> > they ever begin with "invalid"?  If they ever begin with "invalid"
> > that itself may even be a noteworthy breakage to catch, no?
> 
> Answering to myself...
> 
> Because test-dump-cache-tree uses DRY_RUN to create only an in-core
> copy of tree object, and we notice that the reference cache-tree
> created in the tests contains the object name of a tree that does
> not yet exist in the object database.  We get "invalid #(ref)" for
> such node.
> 
> In the ideal world, I think whoever tries to compare two cache-trees
> (i.e. test-dump-cache-tree) should *not* care, because we are merely
> trying to show what the correct tree object name for the node would
> be, but this is only for testing, so the best way forward would be
> to:
> 
>  - Stop using DRY_RUN in test-dump-cache-tree.c;
> 
>  - Stop the code to support DRY_RUN from cache-tree.c (nobody but
>    the test uses it); and
> 
>  - Drop the "-e '#(ref)/d'" from the above.
> 
> I would think.

Do you mean that I should do this in this patch set, or that it's a good
idea for the future?

Also, if we don't use DRY_RUN, won't test-dump-cache-tree add trees to
the actual ODB, which would be odd for a test program?

Re: [PATCH 3/4 v6] cache-tree: subdirectory tests

[David Turner]

On Fri, 2014-07-11 at 08:27 -0700, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Thu, Jul 10, 2014 at 8:31 PM, David Turner <dturner@twopensource.com> wrote:
> >> Add tests to confirm that invalidation of subdirectories neither over-
> >> nor under-invalidates.
> >>
> >> Signed-off-by: David Turner <dturner@twitter.com>
> >> ---
> >>  t/t0090-cache-tree.sh | 26 +++++++++++++++++++++++---
> >>  1 file changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> >> index 98fb1ab..3a3342e 100755
> >> --- a/t/t0090-cache-tree.sh
> >> +++ b/t/t0090-cache-tree.sh
> >> @@ -22,9 +22,10 @@ test_shallow_cache_tree () {
> >>  }
> >>
> >>  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
> >> +       printf "invalid                                  %s ()\n" "" "$@" >expect &&
> 
> Hmm.  This will always expect that the top-level is invalid, even
> when $# is 0.  It is OK if you never need to use this to test that a
> cache-tree is fully valid, but is it something we want to check?

We have test_cache_tree to check that it's fully valid.

> Existing tests are mostly about "cache-tree is populated fully at a
> few strategic, well known and easy places and then it degrades over
> time", but after all your series is adding more places to that set
> of "a few places", so we may want to make sure that future breakages
> to the new code paths that "repair" the cache-tree are caught by
> these tests.

This patchset un-failed "initial commit has cache-tree", and added
"commit in child dir has cache-tree" and "partial commit gives
cache-tree".  I've just added a test for interactive commit; when you
take a look at the next patchset, you can let me know if this seems
sufficient to you.

Re: [PATCH 4/4 v6] cache-tree: Write updated cache-tree after commit

[David Turner]

On Fri, 2014-07-11 at 08:52 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > @@ -16,8 +16,34 @@ cmp_cache_tree () {
> >  # We don't bother with actually checking the SHA1:
> >  # test-dump-cache-tree already verifies that all existing data is
> >  # correct.
> 
> Is this statement now stale and needs to be removed?

I think it is still accurate; we still don't bother to check SHAs and
test-dump-cache-tree still does the comparison.

Re: [PATCH 3/4 v6] cache-tree: subdirectory tests

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> On Fri, 2014-07-11 at 08:40 -0700, Junio C Hamano wrote:
>
>> In the ideal world, I think whoever tries to compare two cache-trees
>> (i.e. test-dump-cache-tree) should *not* care, because we are merely
>> trying to show what the correct tree object name for the node would
>> be, but this is only for testing, so the best way forward would be
>> to:
>> 
>>  - Stop using DRY_RUN in test-dump-cache-tree.c;
>> 
>>  - Stop the code to support DRY_RUN from cache-tree.c (nobody but
>>    the test uses it); and
>> 
>>  - Drop the "-e '#(ref)/d'" from the above.
>> 
>> I would think.
>
> Do you mean that I should do this in this patch set, or that it's a good
> idea for the future?

I have no strong preference either way.  Removing DRY_RUN may
simplify things in the code that gets used in the real life (as
opposed to the code that is only used during the tests), so I do not
mind it if it was done before the series as a preparation step.

> Also, if we don't use DRY_RUN, won't test-dump-cache-tree add trees to
> the actual ODB, which would be odd for a test program?

I do not see it as odd at all; after all, nobody in the real-life
uses dry-run and as you can see its use is broken, or at least is
inconsistent with the rest of the system.