{"thread":{"id":"37106","subject":"[PATCH 1/4 v6] cache-tree: Create/update cache-tree on checkout","startedAt":"2014-07-11T00:31:23Z","lastAt":"2014-07-13T16:42:23Z","messageCount":12,"participants":["David Turner","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":4},"messages":[{"id":"245746","messageId":"1405038686-1138-1-git-send-email-dturner@twitter.com","threadId":"37106","inReplyTo":null,"subject":"[PATCH 1/4 v6] cache-tree: Create/update cache-tree on checkout","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-11T00:31:23Z","receivedAt":"2014-07-11T00:31:23Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"When git checkout checks out a branch, create or update the\ncache-tree so that subsequent operations are faster.\n\nupdate_main_cache_tree learned a new flag, WRITE_TREE_REPAIR.  When\nWRITE_TREE_REPAIR is set, portions of the cache-tree which do not\ncorrespond to existing tree objects are invalidated (and portions which\ndo are marked as valid).  No new tree objects are created.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n builtin/checkout.c    |  8 ++++++++\n cache-tree.c          | 12 +++++++++++-\n cache-tree.h          |  1 +\n t/t0090-cache-tree.sh | 19 ++++++++++++++++---\n 4 files changed, 36 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 07cf555..054214f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t}\n \t}\n \n+\tif (!active_cache_tree)\n+\t\tactive_cache_tree = cache_tree();\n+\n+\tif (!cache_tree_fully_valid(active_cache_tree))\n+\t\tcache_tree_update(active_cache_tree,\n+\t\t\t\t  (const struct cache_entry * const *)active_cache,\n+\t\t\t\t  active_nr, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\n \tif (write_cache(newfd, active_cache, active_nr) ||\n \t    commit_locked_index(lock_file))\n \t\tdie(_(\"unable to write new index file\"));\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 7fa524a..f951d7d 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -239,9 +239,12 @@ static int update_one(struct cache_tree *it,\n \tstruct strbuf buffer;\n \tint missing_ok = flags & WRITE_TREE_MISSING_OK;\n \tint dryrun = flags & WRITE_TREE_DRY_RUN;\n+\tint repair = flags & WRITE_TREE_REPAIR;\n \tint to_invalidate = 0;\n \tint i;\n \n+\tassert(!(dryrun && repair));\n+\n \t*skip_count = 0;\n \n \tif (0 <= it->entry_count && has_sha1_file(it->sha1))\n@@ -374,7 +377,14 @@ static int update_one(struct cache_tree *it,\n #endif\n \t}\n \n-\tif (dryrun)\n+\tif (repair) {\n+\t\tunsigned char sha1[20];\n+\t\thash_sha1_file(buffer.buf, buffer.len, tree_type, sha1);\n+\t\tif (has_sha1_file(sha1))\n+\t\t\thashcpy(it->sha1, sha1);\n+\t\telse\n+\t\t\tto_invalidate = 1;\n+\t} else if (dryrun)\n \t\thash_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1);\n \telse if (write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1)) {\n \t\tstrbuf_release(&buffer);\ndiff --git a/cache-tree.h b/cache-tree.h\nindex f1923ad..666d18f 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -39,6 +39,7 @@ int update_main_cache_tree(int);\n #define WRITE_TREE_IGNORE_CACHE_TREE 2\n #define WRITE_TREE_DRY_RUN 4\n #define WRITE_TREE_SILENT 8\n+#define WRITE_TREE_REPAIR 16\n \n /* error return codes */\n #define WRITE_TREE_UNREADABLE_INDEX (-1)\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 6c33e28..98fb1ab 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -44,14 +44,14 @@ test_expect_success 'read-tree HEAD establishes cache-tree' '\n \n test_expect_success 'git-add invalidates cache-tree' '\n \ttest_when_finished \"git reset --hard; git read-tree HEAD\" &&\n-\techo \"I changed this file\" > foo &&\n+\techo \"I changed this file\" >foo &&\n \tgit add foo &&\n \ttest_invalid_cache_tree\n '\n \n test_expect_success 'update-index invalidates cache-tree' '\n \ttest_when_finished \"git reset --hard; git read-tree HEAD\" &&\n-\techo \"I changed this file\" > foo &&\n+\techo \"I changed this file\" >foo &&\n \tgit update-index --add foo &&\n \ttest_invalid_cache_tree\n '\n@@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' '\n \ttest_shallow_cache_tree\n '\n \n-test_expect_failure 'checkout gives cache-tree' '\n+test_expect_success 'checkout gives cache-tree' '\n+\tgit tag current &&\n \tgit checkout HEAD^ &&\n \ttest_shallow_cache_tree\n '\n \n+test_expect_success 'checkout -b gives cache-tree' '\n+\tgit checkout current &&\n+\tgit checkout -b prev HEAD^ &&\n+\ttest_shallow_cache_tree\n+'\n+\n+test_expect_success 'checkout -B gives cache-tree' '\n+\tgit checkout current &&\n+\tgit checkout -B prev HEAD^ &&\n+\ttest_shallow_cache_tree\n+'\n+\n test_done\n-- \n2.0.0.390.gcb682f8\n"},{"id":"245748","messageId":"1405038686-1138-2-git-send-email-dturner@twitter.com","threadId":"37106","inReplyTo":"1405038686-1138-1-git-send-email-dturner@twitter.com","subject":"[PATCH 2/4 v6] test-dump-cache-tree: invalid trees are not errors","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-11T00:31:24Z","receivedAt":"2014-07-11T00:31:24Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Do not treat known-invalid trees as errors even when their subtree_nr is\nincorrect.  Because git already knows that these trees are invalid,\nan incorrect subtree_nr will not cause problems.\n\nAdd a couple of comments.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n test-dump-cache-tree.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c\nindex 47eab97..cbbbd8e 100644\n--- a/test-dump-cache-tree.c\n+++ b/test-dump-cache-tree.c\n@@ -26,16 +26,16 @@ static int dump_cache_tree(struct cache_tree *it,\n \t\treturn 0;\n \n \tif (it->entry_count < 0) {\n+\t\t/* invalid */\n \t\tdump_one(it, pfx, \"\");\n \t\tdump_one(ref, pfx, \"#(ref) \");\n-\t\tif (it->subtree_nr != ref->subtree_nr)\n-\t\t\terrs = 1;\n \t}\n \telse {\n \t\tdump_one(it, pfx, \"\");\n \t\tif (hashcmp(it->sha1, ref->sha1) ||\n \t\t    ref->entry_count != it->entry_count ||\n \t\t    ref->subtree_nr != it->subtree_nr) {\n+\t\t\t/* claims to be valid but is lying */\n \t\t\tdump_one(ref, pfx, \"#(ref) \");\n \t\t\terrs = 1;\n \t\t}\n-- \n2.0.0.390.gcb682f8\n"},{"id":"245747","messageId":"1405038686-1138-3-git-send-email-dturner@twitter.com","threadId":"37106","inReplyTo":"1405038686-1138-1-git-send-email-dturner@twitter.com","subject":"[PATCH 3/4 v6] cache-tree: subdirectory tests","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-11T00:31:25Z","receivedAt":"2014-07-11T00:31:25Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Add tests to confirm that invalidation of subdirectories neither over-\nnor under-invalidates.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n t/t0090-cache-tree.sh | 26 +++++++++++++++++++++++---\n 1 file changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 98fb1ab..3a3342e 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -22,9 +22,10 @@ test_shallow_cache_tree () {\n }\n \n test_invalid_cache_tree () {\n-\techo \"invalid                                   (0 subtrees)\" >expect &&\n-\tprintf \"SHA #(ref)  (%d entries, 0 subtrees)\\n\" $(git ls-files|wc -l) >>expect &&\n-\tcmp_cache_tree expect\n+\tprintf \"invalid                                  %s ()\\n\" \"\" \"$@\" >expect &&\n+\ttest-dump-cache-tree | \\\n+\tsed -n -e \"s/[0-9]* subtrees//\" -e '/#(ref)/d' -e '/^invalid /p' >actual &&\n+\ttest_cmp expect actual\n }\n \n test_no_cache_tree () {\n@@ -49,6 +50,25 @@ test_expect_success 'git-add invalidates cache-tree' '\n \ttest_invalid_cache_tree\n '\n \n+test_expect_success 'git-add in subdir invalidates cache-tree' '\n+\ttest_when_finished \"git reset --hard; git read-tree HEAD\" &&\n+\tmkdir dirx &&\n+\techo \"I changed this file\" >dirx/foo &&\n+\tgit add dirx/foo &&\n+\ttest_invalid_cache_tree\n+'\n+\n+test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '\n+\tgit tag no-children &&\n+\ttest_when_finished \"git reset --hard no-children; git read-tree HEAD\" &&\n+\tmkdir dir1 dir2 &&\n+\ttest_commit dir1/a &&\n+\ttest_commit dir2/b &&\n+\techo \"I changed this file\" >dir1/a &&\n+\tgit add dir1/a &&\n+\ttest_invalid_cache_tree dir1/\n+'\n+\n test_expect_success 'update-index invalidates cache-tree' '\n \ttest_when_finished \"git reset --hard; git read-tree HEAD\" &&\n \techo \"I changed this file\" >foo &&\n-- \n2.0.0.390.gcb682f8\n"},{"id":"245749","messageId":"1405038686-1138-4-git-send-email-dturner@twitter.com","threadId":"37106","inReplyTo":"1405038686-1138-1-git-send-email-dturner@twitter.com","subject":"[PATCH 4/4 v6] cache-tree: Write updated cache-tree after commit","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-11T00:31:26Z","receivedAt":"2014-07-11T00:31:26Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"During the commit process, update the cache-tree. Write this updated\ncache-tree so that it's ready for subsequent commands.\n\nAdd test code which demonstrates that git commit now writes the cache\ntree.  Make all tests test the entire cache-tree, not just the root\nlevel.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n builtin/commit.c      |  9 +++++-\n t/t0090-cache-tree.sh | 87 ++++++++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 81 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 9cfef6c..99c9054 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \n \t\tdiscard_cache();\n \t\tread_cache_from(index_lock.filename);\n+\t\tif (update_main_cache_tree(WRITE_TREE_SILENT) >= 0)\n+\t\t\twrite_cache(fd, active_cache, active_nr);\n \n \t\tcommit_style = COMMIT_NORMAL;\n \t\treturn index_lock.filename;\n@@ -383,8 +385,12 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \tif (!only && !pathspec.nr) {\n \t\tfd = hold_locked_index(&index_lock, 1);\n \t\trefresh_cache_or_die(refresh_flags);\n-\t\tif (active_cache_changed) {\n+\t\tif (active_cache_changed\n+\t\t    || !cache_tree_fully_valid(active_cache_tree)) {\n \t\t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n+\t\t\tactive_cache_changed = 1;\n+\t\t}\n+\t\tif (active_cache_changed) {\n \t\t\tif (write_cache(fd, active_cache, active_nr) ||\n \t\t\t    commit_locked_index(&index_lock))\n \t\t\t\tdie(_(\"unable to write new_index file\"));\n@@ -435,6 +441,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \tfd = hold_locked_index(&index_lock, 1);\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n+\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n \tif (write_cache(fd, active_cache, active_nr) ||\n \t    close_lock_file(&index_lock))\n \t\tdie(_(\"unable to write new_index file\"));\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 3a3342e..db7a8d0 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -8,7 +8,7 @@ cache-tree extension.\n  . ./test-lib.sh\n \n cmp_cache_tree () {\n-\ttest-dump-cache-tree >actual &&\n+\ttest-dump-cache-tree | sed -e '/#(ref)/d' >actual &&\n \tsed \"s/$_x40/SHA/\" <actual >filtered &&\n \ttest_cmp \"$1\" filtered\n }\n@@ -16,8 +16,34 @@ cmp_cache_tree () {\n # We don't bother with actually checking the SHA1:\n # test-dump-cache-tree already verifies that all existing data is\n # correct.\n-test_shallow_cache_tree () {\n-\tprintf \"SHA  (%d entries, 0 subtrees)\\n\" $(git ls-files|wc -l) >expect &&\n+generate_expected_cache_tree_rec () {\n+\tdir=\"$1${1:+/}\" &&\n+\tparent=\"$2\" &&\n+\t# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux\n+\t# We want to count only foo because it's the only direct child\n+\tsubtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&\n+\tsubtree_count=$(echo \"$subtrees\"|awk '$1 {++c} END {print c}') &&\n+\tentries=$(git ls-files|wc -l) &&\n+\tprintf \"SHA $dir (%d entries, %d subtrees)\\n\" \"$entries\" \"$subtree_count\" &&\n+\tfor subtree in $subtrees\n+\tdo\n+\t    cd \"$subtree\"\n+\t    generate_expected_cache_tree_rec \"$dir$subtree\" \"$dir\" || return 1\n+\t    cd ..\n+\tdone &&\n+\tdir=$parent\n+}\n+\n+generate_expected_cache_tree () {\n+    cwd=$(pwd)\n+    generate_expected_cache_tree_rec\n+    ret=\"$?\"\n+    cd \"$cwd\"\n+    return $ret\n+}\n+\n+test_cache_tree () {\n+\tgenerate_expected_cache_tree >expect &&\n \tcmp_cache_tree expect\n }\n \n@@ -33,14 +59,14 @@ test_no_cache_tree () {\n \tcmp_cache_tree expect\n }\n \n-test_expect_failure 'initial commit has cache-tree' '\n+test_expect_success 'initial commit has cache-tree' '\n \ttest_commit foo &&\n-\ttest_shallow_cache_tree\n+\ttest_cache_tree\n '\n \n test_expect_success 'read-tree HEAD establishes cache-tree' '\n \tgit read-tree HEAD &&\n-\ttest_shallow_cache_tree\n+\ttest_cache_tree\n '\n \n test_expect_success 'git-add invalidates cache-tree' '\n@@ -58,6 +84,18 @@ test_expect_success 'git-add in subdir invalidates cache-tree' '\n \ttest_invalid_cache_tree\n '\n \n+cat >before <<\\EOF\n+SHA  (3 entries, 2 subtrees)\n+SHA dir1/ (1 entries, 0 subtrees)\n+SHA dir2/ (1 entries, 0 subtrees)\n+EOF\n+\n+cat >expect <<\\EOF\n+invalid                                   (2 subtrees)\n+invalid                                  dir1/ (0 subtrees)\n+SHA dir2/ (1 entries, 0 subtrees)\n+EOF\n+\n test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '\n \tgit tag no-children &&\n \ttest_when_finished \"git reset --hard no-children; git read-tree HEAD\" &&\n@@ -65,8 +103,10 @@ test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '\n \ttest_commit dir1/a &&\n \ttest_commit dir2/b &&\n \techo \"I changed this file\" >dir1/a &&\n+\tcmp_cache_tree before &&\n+\techo \"I changed this file\" >dir1/a &&\n \tgit add dir1/a &&\n-\ttest_invalid_cache_tree dir1/\n+\tcmp_cache_tree expect\n '\n \n test_expect_success 'update-index invalidates cache-tree' '\n@@ -79,7 +119,7 @@ test_expect_success 'update-index invalidates cache-tree' '\n test_expect_success 'write-tree establishes cache-tree' '\n \ttest-scrap-cache-tree &&\n \tgit write-tree &&\n-\ttest_shallow_cache_tree\n+\ttest_cache_tree\n '\n \n test_expect_success 'test-scrap-cache-tree works' '\n@@ -90,37 +130,56 @@ test_expect_success 'test-scrap-cache-tree works' '\n \n test_expect_success 'second commit has cache-tree' '\n \ttest_commit bar &&\n-\ttest_shallow_cache_tree\n+\ttest_cache_tree\n+'\n+\n+test_expect_success 'commit in child dir has cache-tree' '\n+\tmkdir dir &&\n+\t>dir/child.t &&\n+\tgit add dir/child.t &&\n+\tgit commit -m dir/child.t &&\n+\ttest_cache_tree\n '\n \n test_expect_success 'reset --hard gives cache-tree' '\n \ttest-scrap-cache-tree &&\n \tgit reset --hard &&\n-\ttest_shallow_cache_tree\n+\ttest_cache_tree\n '\n \n test_expect_success 'reset --hard without index gives cache-tree' '\n \trm -f .git/index &&\n \tgit reset --hard &&\n-\ttest_shallow_cache_tree\n+\ttest_cache_tree\n '\n \n test_expect_success 'checkout gives cache-tree' '\n \tgit tag current &&\n \tgit checkout HEAD^ &&\n-\ttest_shallow_cache_tree\n+\ttest_cache_tree\n '\n \n test_expect_success 'checkout -b gives cache-tree' '\n \tgit checkout current &&\n \tgit checkout -b prev HEAD^ &&\n-\ttest_shallow_cache_tree\n+\ttest_cache_tree\n '\n \n test_expect_success 'checkout -B gives cache-tree' '\n \tgit checkout current &&\n \tgit checkout -B prev HEAD^ &&\n-\ttest_shallow_cache_tree\n+\ttest_cache_tree\n+'\n+\n+test_expect_success 'partial commit gives cache-tree' '\n+\tgit checkout -b partial no-children &&\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\techo \"some change\" >one.t &&\n+\tgit add one.t &&\n+\techo \"some other change\" >two.t &&\n+\tgit commit two.t -m partial &&\n+\ttest_cache_tree\n '\n \n test_done\n-- \n2.0.0.390.gcb682f8\n"},{"id":"245755","messageId":"CAPig+cQVjy5eBtGLsX3uaTEsHyvyjhqCMFaLDn9Upueis-z1eQ@mail.gmail.com","threadId":"37106","inReplyTo":"1405038686-1138-3-git-send-email-dturner@twitter.com","subject":"Re: [PATCH 3/4 v6] cache-tree: subdirectory tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-07-11T06:03:44Z","receivedAt":"2014-07-11T06:03:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 10, 2014 at 8:31 PM, David Turner <dturner@twopensource.com> wrote:\n> Add tests to confirm that invalidation of subdirectories neither over-\n> nor under-invalidates.\n>\n> Signed-off-by: David Turner <dturner@twitter.com>\n> ---\n>  t/t0090-cache-tree.sh | 26 +++++++++++++++++++++++---\n>  1 file changed, 23 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n> index 98fb1ab..3a3342e 100755\n> --- a/t/t0090-cache-tree.sh\n> +++ b/t/t0090-cache-tree.sh\n> @@ -22,9 +22,10 @@ test_shallow_cache_tree () {\n>  }\n>\n>  test_invalid_cache_tree () {\n> -       echo \"invalid                                   (0 subtrees)\" >expect &&\n> -       printf \"SHA #(ref)  (%d entries, 0 subtrees)\\n\" $(git ls-files|wc -l) >>expect &&\n> -       cmp_cache_tree expect\n> +       printf \"invalid                                  %s ()\\n\" \"\" \"$@\" >expect &&\n> +       test-dump-cache-tree | \\\n\nnit: unnecessary backslash\n\n> +       sed -n -e \"s/[0-9]* subtrees//\" -e '/#(ref)/d' -e '/^invalid /p' >actual &&\n> +       test_cmp expect actual\n>  }\n>\n>  test_no_cache_tree () {\n> @@ -49,6 +50,25 @@ test_expect_success 'git-add invalidates cache-tree' '\n>         test_invalid_cache_tree\n>  '\n>\n> +test_expect_success 'git-add in subdir invalidates cache-tree' '\n> +       test_when_finished \"git reset --hard; git read-tree HEAD\" &&\n> +       mkdir dirx &&\n> +       echo \"I changed this file\" >dirx/foo &&\n> +       git add dirx/foo &&\n> +       test_invalid_cache_tree\n> +'\n> +\n> +test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '\n> +       git tag no-children &&\n> +       test_when_finished \"git reset --hard no-children; git read-tree HEAD\" &&\n> +       mkdir dir1 dir2 &&\n> +       test_commit dir1/a &&\n> +       test_commit dir2/b &&\n> +       echo \"I changed this file\" >dir1/a &&\n> +       git add dir1/a &&\n> +       test_invalid_cache_tree dir1/\n> +'\n> +\n>  test_expect_success 'update-index invalidates cache-tree' '\n>         test_when_finished \"git reset --hard; git read-tree HEAD\" &&\n>         echo \"I changed this file\" >foo &&\n> --\n> 2.0.0.390.gcb682f8\n"},{"id":"245785","messageId":"xmqqbnsv6hmc.fsf@gitster.dls.corp.google.com","threadId":"37106","inReplyTo":"CAPig+cQVjy5eBtGLsX3uaTEsHyvyjhqCMFaLDn9Upueis-z1eQ@mail.gmail.com","subject":"Re: [PATCH 3/4 v6] cache-tree: subdirectory tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T15:27:07Z","receivedAt":"2014-07-11T15:27:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Thu, Jul 10, 2014 at 8:31 PM, David Turner <dturner@twopensource.com> wrote:\n>> Add tests to confirm that invalidation of subdirectories neither over-\n>> nor under-invalidates.\n>>\n>> Signed-off-by: David Turner <dturner@twitter.com>\n>> ---\n>>  t/t0090-cache-tree.sh | 26 +++++++++++++++++++++++---\n>>  1 file changed, 23 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n>> index 98fb1ab..3a3342e 100755\n>> --- a/t/t0090-cache-tree.sh\n>> +++ b/t/t0090-cache-tree.sh\n>> @@ -22,9 +22,10 @@ test_shallow_cache_tree () {\n>>  }\n>>\n>>  test_invalid_cache_tree () {\n>> -       echo \"invalid                                   (0 subtrees)\" >expect &&\n>> -       printf \"SHA #(ref)  (%d entries, 0 subtrees)\\n\" $(git ls-files|wc -l) >>expect &&\n>> -       cmp_cache_tree expect\n>> +       printf \"invalid                                  %s ()\\n\" \"\" \"$@\" >expect &&\n\nHmm.  This will always expect that the top-level is invalid, even\nwhen $# is 0.  It is OK if you never need to use this to test that a\ncache-tree is fully valid, but is it something we want to check?\n\nExisting tests are mostly about \"cache-tree is populated fully at a\nfew strategic, well known and easy places and then it degrades over\ntime\", but after all your series is adding more places to that set\nof \"a few places\", so we may want to make sure that future breakages\nto the new code paths that \"repair\" the cache-tree are caught by\nthese tests.\n\n>> +       test-dump-cache-tree | \\\n>\n> nit: unnecessary backslash\n\nGood eyes ;-)\n\n>> +       sed -n -e \"s/[0-9]* subtrees//\" -e '/#(ref)/d' -e '/^invalid /p' >actual &&\n\nIs the second one to remove \"#(ref)\", which appears for a good\n\"reference\" cache tree entry shown for comparison, necessary?  Do\nthey ever begin with \"invalid\"?  If they ever begin with \"invalid\"\nthat itself may even be a noteworthy breakage to catch, no?\n\n>> +       test_cmp expect actual\n>>  }\n>>\n>>  test_no_cache_tree () {\n>> @@ -49,6 +50,25 @@ test_expect_success 'git-add invalidates cache-tree' '\n>>         test_invalid_cache_tree\n>>  '\n>>\n>> +test_expect_success 'git-add in subdir invalidates cache-tree' '\n>> +       test_when_finished \"git reset --hard; git read-tree HEAD\" &&\n>> +       mkdir dirx &&\n>> +       echo \"I changed this file\" >dirx/foo &&\n>> +       git add dirx/foo &&\n>> +       test_invalid_cache_tree\n>> +'\n>> +\n>> +test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' '\n>> +       git tag no-children &&\n>> +       test_when_finished \"git reset --hard no-children; git read-tree HEAD\" &&\n>> +       mkdir dir1 dir2 &&\n>> +       test_commit dir1/a &&\n>> +       test_commit dir2/b &&\n>> +       echo \"I changed this file\" >dir1/a &&\n>> +       git add dir1/a &&\n>> +       test_invalid_cache_tree dir1/\n>> +'\n>> +\n>>  test_expect_success 'update-index invalidates cache-tree' '\n>>         test_when_finished \"git reset --hard; git read-tree HEAD\" &&\n>>         echo \"I changed this file\" >foo &&\n>> --\n>> 2.0.0.390.gcb682f8\n"},{"id":"245786","messageId":"xmqq7g3j6gzr.fsf@gitster.dls.corp.google.com","threadId":"37106","inReplyTo":"xmqqbnsv6hmc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/4 v6] cache-tree: subdirectory tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T15:40:40Z","receivedAt":"2014-07-11T15:40:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>> +       sed -n -e \"s/[0-9]* subtrees//\" -e '/#(ref)/d' -e '/^invalid /p' >actual &&\n>\n> Is the second one to remove \"#(ref)\", which appears for a good\n> \"reference\" cache tree entry shown for comparison, necessary?  Do\n> they ever begin with \"invalid\"?  If they ever begin with \"invalid\"\n> that itself may even be a noteworthy breakage to catch, no?\n\nAnswering to myself...\n\nBecause test-dump-cache-tree uses DRY_RUN to create only an in-core\ncopy of tree object, and we notice that the reference cache-tree\ncreated in the tests contains the object name of a tree that does\nnot yet exist in the object database.  We get \"invalid #(ref)\" for\nsuch node.\n\nIn the ideal world, I think whoever tries to compare two cache-trees\n(i.e. test-dump-cache-tree) should *not* care, because we are merely\ntrying to show what the correct tree object name for the node would\nbe, but this is only for testing, so the best way forward would be\nto:\n\n - Stop using DRY_RUN in test-dump-cache-tree.c;\n\n - Stop the code to support DRY_RUN from cache-tree.c (nobody but\n   the test uses it); and\n\n - Drop the \"-e '#(ref)/d'\" from the above.\n\nI would think.\n"},{"id":"245787","messageId":"xmqq38e76gfi.fsf@gitster.dls.corp.google.com","threadId":"37106","inReplyTo":"1405038686-1138-4-git-send-email-dturner@twitter.com","subject":"Re: [PATCH 4/4 v6] cache-tree: Write updated cache-tree after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T15:52:49Z","receivedAt":"2014-07-11T15:52:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> @@ -16,8 +16,34 @@ cmp_cache_tree () {\n>  # We don't bother with actually checking the SHA1:\n>  # test-dump-cache-tree already verifies that all existing data is\n>  # correct.\n\nIs this statement now stale and needs to be removed?\n\n> -test_shallow_cache_tree () {\n> -\tprintf \"SHA  (%d entries, 0 subtrees)\\n\" $(git ls-files|wc -l) >expect &&\n> +generate_expected_cache_tree_rec () {\n> +\tdir=\"$1${1:+/}\" &&\n> +\tparent=\"$2\" &&\n> +\t# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux\n> +\t# We want to count only foo because it's the only direct child\n> +\tsubtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&\n> +\tsubtree_count=$(echo \"$subtrees\"|awk '$1 {++c} END {print c}') &&\n> +\tentries=$(git ls-files|wc -l) &&\n> +\tprintf \"SHA $dir (%d entries, %d subtrees)\\n\" \"$entries\" \"$subtree_count\" &&\n> +\tfor subtree in $subtrees\n> +\tdo\n> +\t    cd \"$subtree\"\n> +\t    generate_expected_cache_tree_rec \"$dir$subtree\" \"$dir\" || return 1\n> +\t    cd ..\n> +\tdone &&\n> +\tdir=$parent\n> +}\n> +\n> +generate_expected_cache_tree () {\n> +    cwd=$(pwd)\n> +    generate_expected_cache_tree_rec\n> +    ret=\"$?\"\n> +    cd \"$cwd\"\n> +    return $ret\n> +}\n\nAs we always have had trouble between $PWD and $(pwd) on other\nplatforms, I'd prefer something simpler, like:\n\n\texpected_cache_tree () {\n\t\t(\n                \t# subshell to let it wander around freely\n\t        \tgenerate_expected_cache_tree_rec\n\t\t)\n\t}\n\nOther than these two, the new tests were good from a cursory look.\nThe change to builtin/commit.c looked good, too.\n\nThanks.\n"},{"id":"245844","messageId":"1405118791.3775.26.camel@stross","threadId":"37106","inReplyTo":"xmqq7g3j6gzr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/4 v6] cache-tree: subdirectory tests","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-11T22:46:31Z","receivedAt":"2014-07-11T22:46:31Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2014-07-11 at 08:40 -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >>> +       sed -n -e \"s/[0-9]* subtrees//\" -e '/#(ref)/d' -e '/^invalid /p' >actual &&\n> >\n> > Is the second one to remove \"#(ref)\", which appears for a good\n> > \"reference\" cache tree entry shown for comparison, necessary?  Do\n> > they ever begin with \"invalid\"?  If they ever begin with \"invalid\"\n> > that itself may even be a noteworthy breakage to catch, no?\n> \n> Answering to myself...\n> \n> Because test-dump-cache-tree uses DRY_RUN to create only an in-core\n> copy of tree object, and we notice that the reference cache-tree\n> created in the tests contains the object name of a tree that does\n> not yet exist in the object database.  We get \"invalid #(ref)\" for\n> such node.\n> \n> In the ideal world, I think whoever tries to compare two cache-trees\n> (i.e. test-dump-cache-tree) should *not* care, because we are merely\n> trying to show what the correct tree object name for the node would\n> be, but this is only for testing, so the best way forward would be\n> to:\n> \n>  - Stop using DRY_RUN in test-dump-cache-tree.c;\n> \n>  - Stop the code to support DRY_RUN from cache-tree.c (nobody but\n>    the test uses it); and\n> \n>  - Drop the \"-e '#(ref)/d'\" from the above.\n> \n> I would think.\n\nDo you mean that I should do this in this patch set, or that it's a good\nidea for the future?\n\nAlso, if we don't use DRY_RUN, won't test-dump-cache-tree add trees to\nthe actual ODB, which would be odd for a test program?\n"},{"id":"245845","messageId":"1405118796.3775.27.camel@stross","threadId":"37106","inReplyTo":"xmqqbnsv6hmc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/4 v6] cache-tree: subdirectory tests","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-11T22:46:36Z","receivedAt":"2014-07-11T22:46:36Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2014-07-11 at 08:27 -0700, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n> > On Thu, Jul 10, 2014 at 8:31 PM, David Turner <dturner@twopensource.com> wrote:\n> >> Add tests to confirm that invalidation of subdirectories neither over-\n> >> nor under-invalidates.\n> >>\n> >> Signed-off-by: David Turner <dturner@twitter.com>\n> >> ---\n> >>  t/t0090-cache-tree.sh | 26 +++++++++++++++++++++++---\n> >>  1 file changed, 23 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n> >> index 98fb1ab..3a3342e 100755\n> >> --- a/t/t0090-cache-tree.sh\n> >> +++ b/t/t0090-cache-tree.sh\n> >> @@ -22,9 +22,10 @@ test_shallow_cache_tree () {\n> >>  }\n> >>\n> >>  test_invalid_cache_tree () {\n> >> -       echo \"invalid                                   (0 subtrees)\" >expect &&\n> >> -       printf \"SHA #(ref)  (%d entries, 0 subtrees)\\n\" $(git ls-files|wc -l) >>expect &&\n> >> -       cmp_cache_tree expect\n> >> +       printf \"invalid                                  %s ()\\n\" \"\" \"$@\" >expect &&\n> \n> Hmm.  This will always expect that the top-level is invalid, even\n> when $# is 0.  It is OK if you never need to use this to test that a\n> cache-tree is fully valid, but is it something we want to check?\n\nWe have test_cache_tree to check that it's fully valid.\n\n> Existing tests are mostly about \"cache-tree is populated fully at a\n> few strategic, well known and easy places and then it degrades over\n> time\", but after all your series is adding more places to that set\n> of \"a few places\", so we may want to make sure that future breakages\n> to the new code paths that \"repair\" the cache-tree are caught by\n> these tests.\n\nThis patchset un-failed \"initial commit has cache-tree\", and added\n\"commit in child dir has cache-tree\" and \"partial commit gives\ncache-tree\".  I've just added a test for interactive commit; when you\ntake a look at the next patchset, you can let me know if this seems\nsufficient to you.\n"},{"id":"245855","messageId":"1405121841.3775.31.camel@stross","threadId":"37106","inReplyTo":"xmqq38e76gfi.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/4 v6] cache-tree: Write updated cache-tree after commit","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-11T23:37:21Z","receivedAt":"2014-07-11T23:37:21Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2014-07-11 at 08:52 -0700, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n> > @@ -16,8 +16,34 @@ cmp_cache_tree () {\n> >  # We don't bother with actually checking the SHA1:\n> >  # test-dump-cache-tree already verifies that all existing data is\n> >  # correct.\n> \n> Is this statement now stale and needs to be removed?\n\nI think it is still accurate; we still don't bother to check SHAs and\ntest-dump-cache-tree still does the comparison.\n"},{"id":"245949","messageId":"xmqqsim51a8g.fsf@gitster.dls.corp.google.com","threadId":"37106","inReplyTo":"1405118791.3775.26.camel@stross","subject":"Re: [PATCH 3/4 v6] cache-tree: subdirectory tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-13T16:42:23Z","receivedAt":"2014-07-13T16:42:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> On Fri, 2014-07-11 at 08:40 -0700, Junio C Hamano wrote:\n>\n>> In the ideal world, I think whoever tries to compare two cache-trees\n>> (i.e. test-dump-cache-tree) should *not* care, because we are merely\n>> trying to show what the correct tree object name for the node would\n>> be, but this is only for testing, so the best way forward would be\n>> to:\n>> \n>>  - Stop using DRY_RUN in test-dump-cache-tree.c;\n>> \n>>  - Stop the code to support DRY_RUN from cache-tree.c (nobody but\n>>    the test uses it); and\n>> \n>>  - Drop the \"-e '#(ref)/d'\" from the above.\n>> \n>> I would think.\n>\n> Do you mean that I should do this in this patch set, or that it's a good\n> idea for the future?\n\nI have no strong preference either way.  Removing DRY_RUN may\nsimplify things in the code that gets used in the real life (as\nopposed to the code that is only used during the tests), so I do not\nmind it if it was done before the series as a preparation step.\n\n> Also, if we don't use DRY_RUN, won't test-dump-cache-tree add trees to\n> the actual ODB, which would be odd for a test program?\n\nI do not see it as odd at all; after all, nobody in the real-life\nuses dry-run and as you can see its use is broken, or at least is\ninconsistent with the rest of the system.\n"}]}