{"thread":{"id":"37034","subject":"[PATCH 1/3] cache-tree: Create/update cache-tree on checkout","startedAt":"2014-07-01T19:14:33Z","lastAt":"2014-07-01T22:58:04Z","messageCount":7,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"245251","messageId":"1404242075-7068-1-git-send-email-dturner@twitter.com","threadId":"37034","inReplyTo":null,"subject":"[PATCH 1/3] cache-tree: Create/update cache-tree on checkout","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-01T19:14:33Z","receivedAt":"2014-07-01T19:14:33Z","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 | 19 ++++++++++++++++---\n 3 files changed, 27 insertions(+), 5 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..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":"245253","messageId":"1404242075-7068-2-git-send-email-dturner@twitter.com","threadId":"37034","inReplyTo":"1404242075-7068-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-01T19:14:34Z","receivedAt":"2014-07-01T19:14:34Z","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 98fb1ab..8437c5f 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":"245252","messageId":"1404242075-7068-3-git-send-email-dturner@twitter.com","threadId":"37034","inReplyTo":"1404242075-7068-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-01T19:14:35Z","receivedAt":"2014-07-01T19:14:35Z","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 | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 67 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 8437c5f..625157e 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -13,11 +13,35 @@ cmp_cache_tree () {\n \ttest_cmp \"$1\" filtered\n }\n \n+grep_nonmatch_ok () {\n+    grep $@\n+    test \"$?\" = \"2\" && return 1\n+    return 0\n+}\n+\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+\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\"|grep_nonmatch_ok -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 \"$dir$subtree\" $dir || return 1\n+\t    cd ..\n+\tdone &&\n+\tdir=$parent\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 +59,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,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@@ -67,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@@ -95,6 +133,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 +171,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":"245257","messageId":"xmqqzjgsvlb3.fsf@gitster.dls.corp.google.com","threadId":"37034","inReplyTo":"1404242075-7068-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-01T21:08:48Z","receivedAt":"2014-07-01T21:08:48Z","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\nCould you number your patches e.g. [PATCH v4 1/3] and/or summarize\nbelow the three-dash line what got updated since the last round?\nThe readers can guess without when one is sending a reroll every\nother day or less frequently, but with rerolls more often than that,\nit gets unwieldy to check which points raised during the review have\nbeen addressed.\n\nThanks.\n\n>  builtin/checkout.c    |  8 ++++++++\n>  cache-tree.c          |  5 +++--\n>  t/t0090-cache-tree.sh | 19 ++++++++++++++++---\n>  3 files changed, 27 insertions(+), 5 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>  \tif (write_cache(newfd, active_cache, active_nr) ||\n>  \t    commit_locked_index(lock_file))\n>  \t\tdie(_(\"unable to write new index file\"));\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..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"},{"id":"245259","messageId":"xmqqpphovjqk.fsf@gitster.dls.corp.google.com","threadId":"37034","inReplyTo":"1404242075-7068-2-git-send-email-dturner@twitter.com","subject":"Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-01T21:42:43Z","receivedAt":"2014-07-01T21:42:43Z","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> 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\nUnder-explained.  \"more useful\" is subjective so I won't complain\nabout it being not explained enough, but I cannot quite parse and\nunderstand the second sentence.\n\nIt is not \"we treat known invalid trees as errors\".  I think what\nyou meant is \"we insist that a cache-tree entry, even if it is an\ninvalidated one, must record the correct number of subtrees and\nsignal errors otherwise, which is wrong\".\n\nI honestly cannot guess what you meant to say by \"because we can\n...\".\n\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>  \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\nI am guessing this is related to the \"more useful\", but I cannot\noffhand tell what this output shuffling is about.  It would be\nbetter to illustrate in the log message to support the \"more useful\"\nclaim by showing how improved/readable the output got with this\nchange.\n\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\nUnfortunately this is not quite what we would want; this \"#(ref)\"\noutput is so that we can see what tree object we should be referring\nto, while debugging, if this entry were not invalidated; losing that\ndoes not \"Improve output\"---it loses information from the output.\n\n> -\t\tif (it->subtree_nr != ref->subtree_nr)\n> -\t\t\terrs = 1;\n\nI am guessing that this is the change you did not explain quite\nenough with \"do not treat ... as errors\".  This code expects that\neven an invalidated cache-tree entry knows how many subtrees it has,\nwhich is unreasonable.  I do not recall offhand if we used to have\nsome code to ensure that such an invariant holds or not, but when\ninvalidating a directory (say \"t/\") by adding a new subdirectory and\na new file in it (e.g. \"t/dir/file\") to the index, we do not do\nanything other than to invalidate \"t/\" and \"t/dir/\", and I do not\nthink the codepath recounts the number of subdirectories to adjust\nsubtree_nr in any way to match the reality, so removal of this check\nis the right thing to do.\n\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"},{"id":"245261","messageId":"xmqqlhscvgts.fsf@gitster.dls.corp.google.com","threadId":"37034","inReplyTo":"1404242075-7068-3-git-send-email-dturner@twitter.com","subject":"Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-01T22:45:35Z","receivedAt":"2014-07-01T22:45:35Z","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> During the commit process, the cache-tree is updated. We need to write\n> this updated cache-tree so that it's ready for subsequent commands.\n>\n> Add test code which demonstrates that git commit now writes the cache\n> tree.  Also demonstrate that cache-tree invalidation is correct.\n>\n> Signed-off-by: David Turner <dturner@twitter.com>\n> ---\n>  builtin/commit.c      | 15 ++++++------\n>  t/t0090-cache-tree.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---\n>  2 files changed, 67 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 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\nOK, interactive-add may leave the cache-tree not quite populated;\nwe are going to write out a tree from the cache so we need to update\nthe in-core cache tree anyway, so calling update-main-cache-tree\nhere would not hurt (it will speed up the write-cache-as-tree we\nwill eventually call).\n\nWe might want to see if we are really changing anything, though.\nWhat happens if the interactive-add gave us an index with fully\nvalid cache-tree?  Is that rare enough not to matter (not a\nrhetorical question)?\n\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\n\nHow about doing this part like the following instead, so that we can\navoid the overhead of uselessly rewriting the index file when we do\nnot have to?\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5e2221c..7d730a5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -383,8 +383,11 @@ 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 || !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\nIt makes me wonder if we should teach update_main_cache_tree() to\nsomehow smudge active_cache_changed bit as necessary.  Then we do\nnot have to make the call to update-main-cache-tree conditional.\n\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\"));\n\nThis is the index that will be used after we create the commit\n(which will be created from a temporary index that will be discarded\nimmediately after we create the commit).  As we _know_ we are\nchanging something in this code path by calling add_remote_files(),\nit is fine to call update-main-cache-tree here unconditionally.\n\nI didn't notice it when I gave the previous review comment but while\nreviewing this round, we already do the cache-tree population for\n\"commit -a\" in this function, which suggests me that it is the right\nplace to do these changes.  Modulo minor niggles, I like this\niteration much better than the previous one.\n\nThanks for working on this.\n"},{"id":"245268","messageId":"1404255484.3109.7.camel@stross","threadId":"37034","inReplyTo":"xmqqlhscvgts.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-01T22:58:04Z","receivedAt":"2014-07-01T22:58:04Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-07-01 at 15:45 -0700, Junio C Hamano wrote:\n> I didn't notice it when I gave the previous review comment but while\n> reviewing this round, we already do the cache-tree population for\n> \"commit -a\" in this function, which suggests me that it is the right\n> place to do these changes.  Modulo minor niggles, I like this\n> iteration much better than the previous one.\n> \n> Thanks for working on this.\n\nThanks for all your comments.  I'll send out a v4 when I can, but I\nprobably won't have more time to work on this until next week. \n"}]}