{"thread":{"id":"37031","subject":"[PATCH 2/3] test-dump-cache-tree: Improve output format and exit code","startedAt":"2014-07-01T00:13:15Z","lastAt":"2014-07-07T16:58:14Z","messageCount":13,"participants":["David Turner","Torsten Bögershausen","Eric Sunshine","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"245219","messageId":"1404173597-24713-1-git-send-email-dturner@twitter.com","threadId":"37031","inReplyTo":null,"subject":"[PATCH 1/3] cache-tree: Create/update cache-tree on checkout","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-01T00:13:15Z","receivedAt":"2014-07-01T00:13:15Z","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\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n builtin/checkout.c    |  8 ++++++++\n cache-tree.c          |  5 +++--\n t/t0090-cache-tree.sh | 15 ++++++++++++++-\n 3 files changed, 25 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 07cf555..a023a86 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, 0);\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..28d2356 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -612,9 +612,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \t\t\tcache_tree_find(active_cache_tree, prefix);\n \t\tif (!subtree)\n \t\t\treturn WRITE_TREE_PREFIX_ERROR;\n-\t\thashcpy(sha1, subtree->sha1);\n+\t\tif (sha1)\n+\t\t\thashcpy(sha1, subtree->sha1);\n \t}\n-\telse\n+\telse if (sha1)\n \t\thashcpy(sha1, active_cache_tree->sha1);\n \n \tif (0 <= newfd)\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 6c33e28..7c60675 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\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":"245217","messageId":"1404173597-24713-2-git-send-email-dturner@twitter.com","threadId":"37031","inReplyTo":"1404173597-24713-1-git-send-email-dturner@twitter.com","subject":"[PATCH 2/3] test-dump-cache-tree: Improve output format and exit code","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-01T00:13:16Z","receivedAt":"2014-07-01T00:13:16Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Make test-dump-cache-tree more useful for testing.  Do not treat known\ninvalid trees as errors (and do not produce non-zero exit code),\nbecause we can fall back to the non-cache-tree codepath.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n t/t0090-cache-tree.sh  | 28 +++++++++++++++++++++++++---\n test-dump-cache-tree.c | 24 ++++++++++++------------\n 2 files changed, 37 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 7c60675..5383258 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -21,10 +21,13 @@ test_shallow_cache_tree () {\n \tcmp_cache_tree expect\n }\n \n+# Test that the cache-tree for a given directory is invalid.\n+# If no directory is given, check that the root is invalid\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+\ttest-dump-cache-tree >actual &&\n+\tsed -e \"s/$_x40/SHA/\" -e \"s/[0-9]* subtrees//g\" <actual >filtered &&\n+\texpect=$(printf \"invalid                                  $1 ()\\n\") &&\n+\tfgrep \"$expect\" filtered\n }\n \n test_no_cache_tree () {\n@@ -49,6 +52,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 &&\ndiff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c\nindex 47eab97..ad42ca1 100644\n--- a/test-dump-cache-tree.c\n+++ b/test-dump-cache-tree.c\n@@ -6,12 +6,12 @@\n static void dump_one(struct cache_tree *it, const char *pfx, const char *x)\n {\n \tif (it->entry_count < 0)\n-\t\tprintf(\"%-40s %s%s (%d subtrees)\\n\",\n-\t\t       \"invalid\", x, pfx, it->subtree_nr);\n+\t\tprintf(\"%-40s %s (%d subtrees)%s\\n\",\n+\t\t       \"invalid\", pfx, it->subtree_nr, x);\n \telse\n-\t\tprintf(\"%s %s%s (%d entries, %d subtrees)\\n\",\n-\t\t       sha1_to_hex(it->sha1), x, pfx,\n-\t\t       it->entry_count, it->subtree_nr);\n+\t\tprintf(\"%s %s (%d entries, %d subtrees)%s\\n\",\n+\t\t       sha1_to_hex(it->sha1), pfx,\n+\t\t       it->entry_count, it->subtree_nr, x);\n }\n \n static int dump_cache_tree(struct cache_tree *it,\n@@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,\n \t\t/* missing in either */\n \t\treturn 0;\n \n-\tif (it->entry_count < 0) {\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\tdump_one(ref, pfx, \"#(ref) \");\n+\t\t\t/* claims to be valid but is lying */\n+\t\t\tdump_one(ref, pfx, \" #(error)\");\n \t\t\terrs = 1;\n+\t\t} else {\n+\t\t\t/* is actually valid */\n+\t\t\tdump_one(it, pfx, \"\");\n \t\t}\n \t}\n \n-- \n2.0.0.390.gcb682f8\n"},{"id":"245218","messageId":"1404173597-24713-3-git-send-email-dturner@twitter.com","threadId":"37031","inReplyTo":"1404173597-24713-1-git-send-email-dturner@twitter.com","subject":"[PATCH 3/3] cache-tree: Write index with updated cache-tree after commit","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-01T00:13:17Z","receivedAt":"2014-07-01T00:13:17Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"During the commit process, the cache-tree is updated. We need to write\nthis updated cache-tree so that it's ready for subsequent commands.\n\nAdd test code which demonstrates that git commit now writes the cache\ntree.  Also demonstrate that cache-tree invalidation is correct.\n\nSigned-off-by: David Turner <dturner@twitter.com>\n---\n builtin/commit.c      | 15 ++++++-------\n t/t0090-cache-tree.sh | 61 ++++++++++++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 65 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 9cfef6c..dbd4f4b 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,14 +385,10 @@ 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\t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\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-\t\t} else {\n-\t\t\trollback_lock_file(&index_lock);\n-\t\t}\n+\t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n+\t\tif (write_cache(fd, active_cache, active_nr) ||\n+\t\t    commit_locked_index(&index_lock))\n+\t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_AS_IS;\n \t\treturn get_index_file();\n \t}\n@@ -435,6 +433,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 5383258..d50acb8 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -16,8 +16,31 @@ 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+generate_expected_cache_tree () {\n+\tif [ -n \"$1\" ]\n+\tthen\n+\t\tlocal dir=\"$1/\"\n+\telse\n+\t\tlocal dir=\"$1\"\n+\tfi\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+\tlocal subtrees=$(git ls-files|egrep '/' |cut -d / -f 1|uniq) &&\n+\tlocal subtree_count=$(echo \"$subtrees\"|grep -c .) &&\n+\tlocal files_count=$(git ls-files|grep -v /|wc -l) &&\n+\tlocal entries=$(expr \"$subtree_count\" + \"$files_count\") &&\n+\tprintf \"SHA $dir (%d entries, %d subtrees)\\n\" $entries $subtree_count &&\n+\tlocal subtree &&\n+\tfor subtree in $subtrees\n+\tdo\n+\t    cd \"$subtree\"\n+\t    generate_expected_cache_tree \"$dir$subtree\" || return 1\n+\t    cd ..\n+\tdone\n+}\n+\n test_shallow_cache_tree () {\n-\tprintf \"SHA  (%d entries, 0 subtrees)\\n\" $(git ls-files|wc -l) >expect &&\n+\tgenerate_expected_cache_tree > expect &&\n \tcmp_cache_tree expect\n }\n \n@@ -35,7 +58,7 @@ 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 '\n@@ -60,15 +83,28 @@ 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 \tmkdir dir1 dir2 &&\n \ttest_commit dir1/a &&\n \ttest_commit dir2/b &&\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@@ -95,6 +131,14 @@ test_expect_success 'second commit has cache-tree' '\n \ttest_shallow_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_shallow_cache_tree\n+'\n+\n test_expect_success 'reset --hard gives cache-tree' '\n \ttest-scrap-cache-tree &&\n \tgit reset --hard &&\n@@ -125,4 +169,15 @@ test_expect_success 'checkout -B gives cache-tree' '\n \ttest_shallow_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_shallow_cache_tree\n+'\n+\n test_done\n-- \n2.0.0.390.gcb682f8\n"},{"id":"245221","messageId":"53B23605.40705@web.de","threadId":"37031","inReplyTo":"1404173597-24713-1-git-send-email-dturner@twitter.com","subject":"Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-07-01T04:16:05Z","receivedAt":"2014-07-01T04:16:05Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 6c33e28..7c60675 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\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\nThe && chainis broken here.\nDoes the test now pass, because \"git tag\" is added ?\nIn this case: does it may make sense the keep the old one as it is\nan  add a new test case  like this ?\n\n+test_expect_success 'tag and checkout gives cache-tree' '\n\n[]\n"},{"id":"245222","messageId":"53B2386B.4060802@web.de","threadId":"37031","inReplyTo":"1404173597-24713-3-git-send-email-dturner@twitter.com","subject":"Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-07-01T04:26:19Z","receivedAt":"2014-07-01T04:26:19Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"[]\n\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 5383258..d50acb8 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -16,8 +16,31 @@ 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+generate_expected_cache_tree () {\n+\tif [ -n \"$1\" ]\n+\tthen\n+\t\tlocal dir=\"$1/\"\n+\telse\n+\t\tlocal dir=\"$1\"\n+\tfi\n\nI think the Git test cases prefer \"test\" over \"[]\":\n\n\tif test -n \"$1\"\n\tthen\n\t\tlocal dir=\"$1/\"\n\telse\n\t\tlocal dir=\"$1\"\n\tfi\n\nAnd should there be a && after the \"fi\" ?\nAnd as test -n tests for a non-zero string,\ncould we write like this (and drop the local ?)?\n\n\tif test -n \"$1\"\n\tthen\n\t\tdir=\"$1/\"\n\telse\n\t\tdir=\"\"\n\tfi\n"},{"id":"245224","messageId":"CAPig+cSKiB3b-gufaOx4=rWBjWWyGiqx-M85+2UAGq-BUkkLHA@mail.gmail.com","threadId":"37031","inReplyTo":"1404173597-24713-2-git-send-email-dturner@twitter.com","subject":"Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-07-01T04:43:09Z","receivedAt":"2014-07-01T04:43:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 30, 2014 at 8:13 PM, David Turner <dturner@twopensource.com> wrote:\n> Make test-dump-cache-tree more useful for testing.  Do not treat known\n> invalid trees as errors (and do not produce non-zero exit code),\n> because we can fall back to the non-cache-tree codepath.\n>\n> Signed-off-by: David Turner <dturner@twitter.com>\n> ---\n>  t/t0090-cache-tree.sh  | 28 +++++++++++++++++++++++++---\n>  test-dump-cache-tree.c | 24 ++++++++++++------------\n>  2 files changed, 37 insertions(+), 15 deletions(-)\n>\n> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n> index 7c60675..5383258 100755\n> --- a/t/t0090-cache-tree.sh\n> +++ b/t/t0090-cache-tree.sh\n> @@ -21,10 +21,13 @@ test_shallow_cache_tree () {\n>         cmp_cache_tree expect\n>  }\n>\n> +# Test that the cache-tree for a given directory is invalid.\n> +# If no directory is given, check that the root is invalid\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> +       test-dump-cache-tree >actual &&\n> +       sed -e \"s/$_x40/SHA/\" -e \"s/[0-9]* subtrees//g\" <actual >filtered &&\n> +       expect=$(printf \"invalid                                  $1 ()\\n\") &&\n> +       fgrep \"$expect\" filtered\n>  }\n>\n>  test_no_cache_tree () {\n> @@ -49,6 +52,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\nStyle: drop whitespace after >\n\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\nBroken &&-chain.\n\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\nStyle: drop whitespace after >\n\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> diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c\n> index 47eab97..ad42ca1 100644\n> --- a/test-dump-cache-tree.c\n> +++ b/test-dump-cache-tree.c\n> @@ -6,12 +6,12 @@\n>  static void dump_one(struct cache_tree *it, const char *pfx, const char *x)\n>  {\n>         if (it->entry_count < 0)\n> -               printf(\"%-40s %s%s (%d subtrees)\\n\",\n> -                      \"invalid\", x, pfx, it->subtree_nr);\n> +               printf(\"%-40s %s (%d subtrees)%s\\n\",\n> +                      \"invalid\", pfx, it->subtree_nr, x);\n>         else\n> -               printf(\"%s %s%s (%d entries, %d subtrees)\\n\",\n> -                      sha1_to_hex(it->sha1), x, pfx,\n> -                      it->entry_count, it->subtree_nr);\n> +               printf(\"%s %s (%d entries, %d subtrees)%s\\n\",\n> +                      sha1_to_hex(it->sha1), pfx,\n> +                      it->entry_count, it->subtree_nr, x);\n>  }\n>\n>  static int dump_cache_tree(struct cache_tree *it,\n> @@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,\n>                 /* missing in either */\n>                 return 0;\n>\n> -       if (it->entry_count < 0) {\n> +       if (it->entry_count < 0)\n> +               /* invalid */\n>                 dump_one(it, pfx, \"\");\n> -               dump_one(ref, pfx, \"#(ref) \");\n> -               if (it->subtree_nr != ref->subtree_nr)\n> -                       errs = 1;\n> -       }\n>         else {\n> -               dump_one(it, pfx, \"\");\n>                 if (hashcmp(it->sha1, ref->sha1) ||\n>                     ref->entry_count != it->entry_count ||\n>                     ref->subtree_nr != it->subtree_nr) {\n> -                       dump_one(ref, pfx, \"#(ref) \");\n> +                       /* claims to be valid but is lying */\n> +                       dump_one(ref, pfx, \" #(error)\");\n>                         errs = 1;\n> +               } else {\n> +                       /* is actually valid */\n> +                       dump_one(it, pfx, \"\");\n>                 }\n>         }\n>\n> --\n> 2.0.0.390.gcb682f8\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"245225","messageId":"53B24BF5.3020007@viscovery.net","threadId":"37031","inReplyTo":"53B2386B.4060802@web.de","subject":"Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-07-01T05:49:41Z","receivedAt":"2014-07-01T05:49:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 7/1/2014 6:26, schrieb Torsten Bögershausen:\n> And as test -n tests for a non-zero string,\n> could we write like this (and drop the local ?)?\n> \n>     if test -n \"$1\"\n>     then\n>         dir=\"$1/\"\n>     else\n>         dir=\"\"\n>     fi\n\nThese six lines can be written as\n\n\tdir=\"$1${1:+/}\" &&\n\nand 'local' must be dropped because is not universally supported, yet.\n\n-- Hannes\n"},{"id":"245250","messageId":"1404242126.6112.7.camel@stross","threadId":"37031","inReplyTo":"53B23605.40705@web.de","subject":"Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-01T19:15:26Z","receivedAt":"2014-07-01T19:15:26Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-07-01 at 06:16 +0200, Torsten Bögershausen wrote:\n> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n> index 6c33e28..7c60675 100755\n> --- a/t/t0090-cache-tree.sh\n> +++ b/t/t0090-cache-tree.sh\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> The && chainis broken here.\n> Does the test now pass, because \"git tag\" is added ?\n\nThe tag does not cause the cache-tree to be created, so git tag does not\ncause the test to pass.\n"},{"id":"245255","messageId":"xmqq8uocx2c5.fsf@gitster.dls.corp.google.com","threadId":"37031","inReplyTo":"1404173597-24713-1-git-send-email-dturner@twitter.com","subject":"Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-01T20:15:38Z","receivedAt":"2014-07-01T20:15:38Z","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> When git checkout checks out a branch, create or update the\n> cache-tree so that subsequent operations are faster.\n>\n> Signed-off-by: David Turner <dturner@twitter.com>\n> ---\n>  builtin/checkout.c    |  8 ++++++++\n>  cache-tree.c          |  5 +++--\n>  t/t0090-cache-tree.sh | 15 ++++++++++++++-\n>  3 files changed, 25 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 07cf555..a023a86 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, 0);\n> +\n\nThis looks much better than the previous round, but it still does\nallow verify_cache() to throw noises against unmerged entries in the\ncache, as WRITE_TREE_SILENT flag is not passed down, no?\n\n\t$ git checkout master^0\n        $ git am $this_message\n        $ make\n        $ edit builtin/checkout.c ;# make changes to the above lines\n        $ ./git checkout -m master^0\nx       builtin/checkout.c: unmerged (972c8a7b28f16f88475268f9a714048c228e69db)\nx       builtin/checkout.c: unmerged (f1dc56e55f7b2200412142b10517458ccfda2952)\nx       builtin/checkout.c: unmerged (3b9753ba8c19e7dfe6e922f30eb85c83a92a4596)\n        M       builtin/checkout.c\n        Warning: you are leaving 1 commit behind, not connected to\n        any of your branches:\n\n          25fab54 cache-tree: Create/update cache-tree on checkout\n\n        Switched to branch 'master'\n\nPassing WRITE_TREE_SILENT in the flags parameter will get rid of the\nconflict notice output from the above.\n\nThe user is not interested in writing a brand new tree object at all\nin this case, so it feels wrong to actually let the call chain go\ndown to update_one() and create new tree objects.\n\n\tSide note.  And passing WRITE_TREE_DRY_RUN is not a good\n\tsolution either, because a later write_cache_as_tree() will\n\tnot create the necessary tree object once you stuff a tree\n\tobject name in the cache-tree.\n\nWhat we want in this code path is a way to repair a sub cache_tree\nif it can be repaired without creating a new tree object and\notherwise leave that part invalid.  The existing cache-tree\nframework is not prepared to do that kind of thing.  It wants to\nstart from the bottom and percolate things up, computing levels\nnearer to the top-level only after it fully created the trees for\ndeeper levels, because it is meant to be used only when we really\nwant to write out trees.  We may want to reuse update_one() but\n\nI am not convinced that doing an equivalent of write-tree when you\nswitch branches is the right approach in the first place.  You will\neventually write it out as a tree, and having a relatively undamaged\ncache-tree will help you when you do so, but spending the cycles\nnecessary to compute a fully populated cache-tree, only to let it\ndegrade over time until you are ready to write it out as a tree,\nsomehow sounds like asking for a duplicated work upfront.\n\nBy the way, you seem to have touched write_cache_as_tree() in the\nsame patch, but I am not sure what makes the change necessary.  I do\nnot see a new caller to it that passes a NULL to its sha1 parameter.\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 7fa524a..28d2356 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -612,9 +612,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n>  \t\t\tcache_tree_find(active_cache_tree, prefix);\n>  \t\tif (!subtree)\n>  \t\t\treturn WRITE_TREE_PREFIX_ERROR;\n> -\t\thashcpy(sha1, subtree->sha1);\n> +\t\tif (sha1)\n> +\t\t\thashcpy(sha1, subtree->sha1);\n>  \t}\n> -\telse\n> +\telse if (sha1)\n>  \t\thashcpy(sha1, active_cache_tree->sha1);\n>  \n>  \tif (0 <= newfd)\n> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n> index 6c33e28..7c60675 100755\n> --- a/t/t0090-cache-tree.sh\n> +++ b/t/t0090-cache-tree.sh\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"},{"id":"245256","messageId":"xmqq4mz0x04l.fsf@gitster.dls.corp.google.com","threadId":"37031","inReplyTo":"1404242126.6112.7.camel@stross","subject":"Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-01T21:03:22Z","receivedAt":"2014-07-01T21:03:22Z","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 Tue, 2014-07-01 at 06:16 +0200, Torsten Bögershausen wrote:\n>> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n>> index 6c33e28..7c60675 100755\n>> --- a/t/t0090-cache-tree.sh\n>> +++ b/t/t0090-cache-tree.sh\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>> The && chainis broken here.\n>> Does the test now pass, because \"git tag\" is added ?\n>\n> The tag does not cause the cache-tree to be created, so git tag does not\n> cause the test to pass.\n\nThat does not explain why it is a good idea to declare success of\nthis test if this new \"git tag current\" fails here for whatever\nreason (e.g. somebody updated \"git tag\" for a reason that is\ncompletely unrelated to cache-tree and made it segfault without\ncreating the \"current\" tag).\n"},{"id":"245260","messageId":"1404252587.3109.1.camel@stross","threadId":"37031","inReplyTo":"xmqq4mz0x04l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-01T22:09:47Z","receivedAt":"2014-07-01T22:09:47Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-07-01 at 14:03 -0700, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n> > On Tue, 2014-07-01 at 06:16 +0200, Torsten Bögershausen wrote:\n> >> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n> >> index 6c33e28..7c60675 100755\n> >> --- a/t/t0090-cache-tree.sh\n> >> +++ b/t/t0090-cache-tree.sh\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> >> The && chainis broken here.\n> >> Does the test now pass, because \"git tag\" is added ?\n> >\n> > The tag does not cause the cache-tree to be created, so git tag does not\n> > cause the test to pass.\n> \n> That does not explain why it is a good idea to declare success of\n> this test if this new \"git tag current\" fails here for whatever\n> reason (e.g. somebody updated \"git tag\" for a reason that is\n> completely unrelated to cache-tree and made it segfault without\n> creating the \"current\" tag).\n\nIndeed; that's why the latest version includes &&.\n"},{"id":"245431","messageId":"1404619467.3109.38.camel@stross","threadId":"37031","inReplyTo":"xmqq8uocx2c5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-06T04:04:27Z","receivedAt":"2014-07-06T04:04:27Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-07-01 at 13:15 -0700, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n> > When git checkout checks out a branch, create or update the\n> > cache-tree so that subsequent operations are faster.\n> >\n> > Signed-off-by: David Turner <dturner@twitter.com>\n> > ---\n> >  builtin/checkout.c    |  8 ++++++++\n> >  cache-tree.c          |  5 +++--\n> >  t/t0090-cache-tree.sh | 15 ++++++++++++++-\n> >  3 files changed, 25 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/builtin/checkout.c b/builtin/checkout.c\n> > index 07cf555..a023a86 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, 0);\n> > +\n> \n> This looks much better than the previous round, but it still does\n> allow verify_cache() to throw noises against unmerged entries in the\n> cache, as WRITE_TREE_SILENT flag is not passed down, no?\n> \n> \t$ git checkout master^0\n>         $ git am $this_message\n>         $ make\n>         $ edit builtin/checkout.c ;# make changes to the above lines\n>         $ ./git checkout -m master^0\n> x       builtin/checkout.c: unmerged (972c8a7b28f16f88475268f9a714048c228e69db)\n> x       builtin/checkout.c: unmerged (f1dc56e55f7b2200412142b10517458ccfda2952)\n> x       builtin/checkout.c: unmerged (3b9753ba8c19e7dfe6e922f30eb85c83a92a4596)\n>         M       builtin/checkout.c\n>         Warning: you are leaving 1 commit behind, not connected to\n>         any of your branches:\n> \n>           25fab54 cache-tree: Create/update cache-tree on checkout\n> \n>         Switched to branch 'master'\n> \n> Passing WRITE_TREE_SILENT in the flags parameter will get rid of the\n> conflict notice output from the above.\n> \n> The user is not interested in writing a brand new tree object at all\n> in this case, so it feels wrong to actually let the call chain go\n> down to update_one() and create new tree objects.\n> \n> \tSide note.  And passing WRITE_TREE_DRY_RUN is not a good\n> \tsolution either, because a later write_cache_as_tree() will\n> \tnot create the necessary tree object once you stuff a tree\n> \tobject name in the cache-tree.\n> \n> What we want in this code path is a way to repair a sub cache_tree\n> if it can be repaired without creating a new tree object and\n> otherwise leave that part invalid.  The existing cache-tree\n> framework is not prepared to do that kind of thing.  It wants to\n> start from the bottom and percolate things up, computing levels\n> nearer to the top-level only after it fully created the trees for\n> deeper levels, because it is meant to be used only when we really\n> want to write out trees.  We may want to reuse update_one() but\n> \n> I am not convinced that doing an equivalent of write-tree when you\n> switch branches is the right approach in the first place.  You will\n> eventually write it out as a tree, and having a relatively undamaged\n> cache-tree will help you when you do so, but spending the cycles\n> necessary to compute a fully populated cache-tree, only to let it\n> degrade over time until you are ready to write it out as a tree,\n> somehow sounds like asking for a duplicated work upfront.\n\nAs I understand it, the cache-tree extension was originally designed to\nspeed up writing the tree later.  However, as Karsten Blees's work (and\nmy own tests) have shown, it also speeds up git status.  I use git\nstatus a lot while working, and I've talked to a few others who do the\nsame.  So I think it's worth spending extra time when switching branches\nto have a good working experience within that branch.\n\nIn the new version of the patchset (which I'll post shortly), I've added\nan option WRITE_TREE_REPAIR, which does all of the work to compute a new\ntree object, but only adds it to the cache-tree if it already exists\non-disk.  This is a little wasteful for the reason that you note.  So if\nyou would like, I could add a config option to skip it.  But I think it\nis a good default.\n\nDoes this seem OK to you, assuming the implementation is good? \n"},{"id":"245473","messageId":"xmqq4mytf6mx.fsf@gitster.dls.corp.google.com","threadId":"37031","inReplyTo":"1404619467.3109.38.camel@stross","subject":"Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-07T16:58:14Z","receivedAt":"2014-07-07T16:58:14Z","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>> I am not convinced that doing an equivalent of write-tree when you\n>> switch branches is the right approach in the first place.  You will\n>> eventually write it out as a tree, and having a relatively undamaged\n>> cache-tree will help you when you do so, but spending the cycles\n>> necessary to compute a fully populated cache-tree, only to let it\n>> degrade over time until you are ready to write it out as a tree,\n>> somehow sounds like asking for a duplicated work upfront.\n>\n> As I understand it, the cache-tree extension was originally designed to\n> speed up writing the tree later.  However, as Karsten Blees's work (and\n> my own tests) have shown, it also speeds up git status.  I use git\n> status a lot while working, and I've talked to a few others who do the\n> same.  So I think it's worth spending extra time when switching branches\n> to have a good working experience within that branch.\n\nYou are reading the history of the subsystem correctly.  Since\nb65982b6 (Optimize \"diff-index --cached\" using cache-tree,\n2009-05-20), having an undamaged cache-tree does help with \"git\nstatus\" as well.\n\n> In the new version of the patchset (which I'll post shortly), I've added\n> an option WRITE_TREE_REPAIR, which does all of the work to compute a new\n> tree object, but only adds it to the cache-tree if it already exists\n> on-disk.  This is a little wasteful for the reason that you note.  So if\n> you would like, I could add a config option to skip it.  But I think it\n> is a good default.\n\nOK, sounds good.\n"}]}