{"thread":{"id":"37121","subject":"[PATCH v8 1/4] cache-tree: Create/update cache-tree on checkout","startedAt":"2014-07-12T04:44:33Z","lastAt":"2014-09-03T12:02:58Z","messageCount":28,"participants":["David Turner","Duy Nguyen","Junio C Hamano","Ramsay Jones","John Keeping","Eric Sunshine"],"isPatch":true,"patchVersion":8,"patchTotal":4},"messages":[{"id":"245879","messageId":"1405140276-32162-1-git-send-email-dturner@twitter.com","threadId":"37121","inReplyTo":null,"subject":"[PATCH v8 1/4] cache-tree: Create/update cache-tree on checkout","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-12T04:44:33Z","receivedAt":"2014-07-12T04:44: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\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 \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":"245880","messageId":"1405140276-32162-2-git-send-email-dturner@twitter.com","threadId":"37121","inReplyTo":"1405140276-32162-1-git-send-email-dturner@twitter.com","subject":"[PATCH v8 2/4] test-dump-cache-tree: invalid trees are not errors","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-12T04:44:34Z","receivedAt":"2014-07-12T04:44:34Z","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 \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":"245881","messageId":"1405140276-32162-3-git-send-email-dturner@twitter.com","threadId":"37121","inReplyTo":"1405140276-32162-1-git-send-email-dturner@twitter.com","subject":"[PATCH v8 3/4] cache-tree: subdirectory tests","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-12T04:44:35Z","receivedAt":"2014-07-12T04:44:35Z","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 \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":"245882","messageId":"1405140276-32162-4-git-send-email-dturner@twitter.com","threadId":"37121","inReplyTo":"1405140276-32162-1-git-send-email-dturner@twitter.com","subject":"[PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-12T04:44:36Z","receivedAt":"2014-07-12T04:44:36Z","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 \n---\n builtin/commit.c | 16 ++++++-\n t/t0090-cache-tree.sh | 117 +++++++++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 117 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 9cfef6c..d6681c4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -342,6 +342,15 @@ 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\tfd = open(index_lock.filename, O_WRONLY);\n+\t\t\tif (fd >= 0)\n+\t\t\t\tif (write_cache(fd, active_cache, active_nr) == 0) {\n+\t\t\t\t\tclose_lock_file(&index_lock);\n+\t\t\t}\n+\t\t}\n+\t\telse\n+\t\t\tfprintf(stderr, \"FAiled to update main cache tree\\n\");\n \n \t\tcommit_style = COMMIT_NORMAL;\n \t\treturn index_lock.filename;\n@@ -383,8 +392,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 +448,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..48c4240 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/\" filtered &&\n \ttest_cmp \"$1\" filtered\n }\n@@ -16,14 +16,38 @@ 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\tcd \"$subtree\"\n+\t\tgenerate_expected_cache_tree_rec \"$dir$subtree\" \"$dir\" || return 1\n+\t\tcd ..\n+\tdone &&\n+\tdir=$parent\n+}\n+\n+generate_expected_cache_tree () {\n+\t(\n+\t\tgenerate_expected_cache_tree_rec\n+\t)\n+}\n+\n+test_cache_tree () {\n+\tgenerate_expected_cache_tree >expect &&\n \tcmp_cache_tree expect\n }\n \n test_invalid_cache_tree () {\n \tprintf \"invalid %s ()\\n\" \"\" \"$@\" >expect &&\n-\ttest-dump-cache-tree | \\\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@@ -33,14 +57,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 +82,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 +101,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 +117,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 +128,86 @@ 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 --interactive gives cache-tree on partial commit' '\n+\tcat <<-\\EOT >foo.c &&\n+\tint foo()\n+\t{\n+\t\treturn 42;\n+\t}\n+\tint bar()\n+\t{\n+\t\treturn 42;\n+\t}\n+\tEOT\n+\tgit add foo.c &&\n+\ttest_invalid_cache_tree &&\n+\tgit commit -m \"add a file\" &&\n+\ttest_cache_tree &&\n+\tcat <<-\\EOT >foo.c &&\n+\tint foo()\n+\t{\n+\t\treturn 43;\n+\t}\n+\tint bar()\n+\t{\n+\t\treturn 44;\n+\t}\n+\tEOT\n+\t(echo p; echo 1; echo; echo s; echo n; echo y; echo q) |\n+\tgit commit --interactive -m foo &&\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":"245936","messageId":"CACsJy8D0CdS5B5xNSSCk+LToXV9FnHFLkPzJ5f-7NTWiw9yn5w@mail.gmail.com","threadId":"37121","inReplyTo":"1405140276-32162-4-git-send-email-dturner@twitter.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-13T05:09:25Z","receivedAt":"2014-07-13T05:09:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 12, 2014 at 11:44 AM, David Turner wrote:\n> @@ -342,6 +342,15 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n>\n> discard_cache();\n> read_cache_from(index_lock.filename);\n> + if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n> + fd = open(index_lock.filename, O_WRONLY);\n> + if (fd >= 0)\n> + if (write_cache(fd, active_cache, active_nr) == 0) {\n> + close_lock_file(&index_lock);\n\nIf write_cache() returns a negative value, index.lock is probably\ncorrupted. Should we die() instead of moving on and returning\nindex_lock.filename to the caller? The caller may move index.lock to\nindex later on and officially ruin \"index\".\n\n> + }\n> + }\n> + else\n> + fprintf(stderr, \"FAiled to update main cache tree\\n\");\n\nmake the above line something like this for i18n support:\n\nfprintf_ln(stderr, _(\"Failed to update main cache tree\"));\n\n>\n> commit_style = COMMIT_NORMAL;\n> return index_lock.filename;\n-- \nDuy\n"},{"id":"245990","messageId":"xmqqr41oylyo.fsf@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"CACsJy8D0CdS5B5xNSSCk+LToXV9FnHFLkPzJ5f-7NTWiw9yn5w@mail.gmail.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-14T15:54:55Z","receivedAt":"2014-07-14T15:54:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, Jul 12, 2014 at 11:44 AM, David Turner wrote:\n>> @@ -342,6 +342,15 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n>>\n>> discard_cache();\n>> read_cache_from(index_lock.filename);\n>> + if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n>> + fd = open(index_lock.filename, O_WRONLY);\n>> + if (fd >= 0)\n>> + if (write_cache(fd, active_cache, active_nr) == 0) {\n>> + close_lock_file(&index_lock);\n>\n> If write_cache() returns a negative value, index.lock is probably\n> corrupted. Should we die() instead of moving on and returning\n> index_lock.filename to the caller? The caller may move index.lock to\n> index later on and officially ruin \"index\".\n\nPerhaps true, but worse yet, this will not play nicely together with\nyour split index series, no? After taking the lock and writing and\nclosing, we spawn the interactive while still holding the lock, and\nthe \"open\" we see here is because we want to further update the same\nunder the same lock. Perhaps write_locked_index() API in the split\nindex series can notice that the underlying fd in index_lock has\nbeen closed earlier, realize that the call is to re-update the\nindex under the same lock and open the file again for writing?\n\nThen we can update the above \"open() then write_cache()\" sequence\nwith just a call to \"write_locked_index()\".\n"},{"id":"245994","messageId":"53C41456.2000006@ramsay1.demon.co.uk","threadId":"37121","inReplyTo":"xmqqr41oylyo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-14T17:33:10Z","receivedAt":"2014-07-14T17:33:10Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 14/07/14 16:54, Junio C Hamano wrote:\n> Duy Nguyen writes:\n> \n>> On Sat, Jul 12, 2014 at 11:44 AM, David Turner wrote:\n>>> @@ -342,6 +342,15 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n>>>\n>>> discard_cache();\n>>> read_cache_from(index_lock.filename);\n>>> + if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n>>> + fd = open(index_lock.filename, O_WRONLY);\n>>> + if (fd >= 0)\n>>> + if (write_cache(fd, active_cache, active_nr) == 0) {\n>>> + close_lock_file(&index_lock);\n>>\n>> If write_cache() returns a negative value, index.lock is probably\n>> corrupted. Should we die() instead of moving on and returning\n>> index_lock.filename to the caller? The caller may move index.lock to\n>> index later on and officially ruin \"index\".\n> \n> Perhaps true, but worse yet, this will not play nicely together with\n> your split index series, no? After taking the lock and writing and\n> closing, we spawn the interactive while still holding the lock, and\n> the \"open\" we see here is because we want to further update the same\n> under the same lock. Perhaps write_locked_index() API in the split\n> index series can notice that the underlying fd in index_lock has\n> been closed earlier, realize that the call is to re-update the\n> index under the same lock and open the file again for writing?\n\nHmm, I was just about to suggest that there was some negative interplay\nbetween the 'dt/cache-tree-repair' and 'nd/split-index' branches as well.\n\nThe pu branch fails the testsuite for me. In particular, t0090-cache-tree.sh\nfails like so:\n\n $ ./t0090-cache-tree.sh -i -v\n ...\n ok 9 - second commit has cache-tree\n\n expecting success: \n \tcat <<-\\EOT >foo.c &&\n \tint foo()\n \t{\n \t\treturn 42;\n \t}\n \tint bar()\n \t{\n \t\treturn 42;\n \t}\n \tEOT\n \tgit add foo.c &&\n \ttest_invalid_cache_tree &&\n \tgit commit -m \"add a file\" &&\n \ttest_cache_tree &&\n \tcat <<-\\EOT >foo.c &&\n \tint foo()\n \t{\n \t\treturn 43;\n \t}\n \tint bar()\n \t{\n \t\treturn 44;\n \t}\n \tEOT\n \t(echo p; echo 1; echo; echo s; echo n; echo y; echo q) |\n \tgit commit --interactive -m foo &&\n \ttest_cache_tree\n \n [master d1075a6] add a file\n Author: A U Thor \n 1 file changed, 8 insertions(+)\n create mode 100644 foo.c\n staged unstaged path\n 1: unchanged +2/-2 foo.c\n \n *** Commands ***\n 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n What now> staged unstaged path\n 1: unchanged +2/-2 [f]oo.c\n Patch update>> staged unstaged path\n * 1: unchanged +2/-2 [f]oo.c\n Patch update>> diff --git a/foo.c b/foo.c\n index 75522e2..3f7f049 100644\n --- a/foo.c\n +++ b/foo.c\n @@ -1,8 +1,8 @@\n int foo()\n {\n -return 42;\n +return 43;\n }\n int bar()\n {\n -return 42;\n +return 44;\n }\n Stage this hunk [y,n,q,a,d,/,s,e,?]? Split into 2 hunks.\n @@ -1,6 +1,6 @@\n int foo()\n {\n -return 42;\n +return 43;\n }\n int bar()\n {\n Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]? @@ -4,5 +4,5 @@\n }\n int bar()\n {\n -return 42;\n +return 44;\n }\n Stage this hunk [y,n,q,a,d,/,K,g,e,?]? \n *** Commands ***\n 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n What now> Bye.\n [master 65d7dde] foo\n Author: A U Thor \n 1 file changed, 1 insertion(+), 1 deletion(-)\n --- expect\t2014-07-14 17:10:13.755209229 +0000\n +++ filtered\t2014-07-14 17:10:13.763209258 +0000\n @@ -1 +1 @@\n -SHA (3 entries, 0 subtrees)\n +invalid (0 subtrees)\n not ok 10 - commit --interactive gives cache-tree on partial commit\n #\t\n #\t\tcat <<-\\EOT >foo.c &&\n #\t\tint foo()\n #\t\t{\n #\t\t\treturn 42;\n #\t\t}\n #\t\tint bar()\n #\t\t{\n #\t\t\treturn 42;\n #\t\t}\n #\t\tEOT\n #\t\tgit add foo.c &&\n #\t\ttest_invalid_cache_tree &&\n #\t\tgit commit -m \"add a file\" &&\n #\t\ttest_cache_tree &&\n #\t\tcat <<-\\EOT >foo.c &&\n #\t\tint foo()\n #\t\t{\n #\t\t\treturn 43;\n #\t\t}\n #\t\tint bar()\n #\t\t{\n #\t\t\treturn 44;\n #\t\t}\n #\t\tEOT\n #\t\t(echo p; echo 1; echo; echo s; echo n; echo y; echo q) |\n #\t\tgit commit --interactive -m foo &&\n #\t\ttest_cache_tree\n #\t\n $ \n\nNote that I haven't even looked at the test failure itself yet.\n\nHowever, I noticed that commit 002ccda (\"cache-tree: write updated\ncache-tree after commit\", 11-07-2014) passes that test just fine, but\nthat the merge commit 7608c87e fails. Looking at the details of the\nmerge resolution, made me think of Duy's split index work.\n\nI probably won't look at this further tonight, so this is just a\nheads-up on a possible problem.\n\nHTH\n\nATB,\nRamsay Jones\n"},{"id":"245995","messageId":"xmqqzjgbygy3.fsf@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"xmqqr41oylyo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-14T17:43:16Z","receivedAt":"2014-07-14T17:43:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Duy Nguyen writes:\n>\n>> On Sat, Jul 12, 2014 at 11:44 AM, David Turner wrote:\n>>> @@ -342,6 +342,15 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n>>>\n>>> discard_cache();\n>>> read_cache_from(index_lock.filename);\n>>> + if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n>>> + fd = open(index_lock.filename, O_WRONLY);\n>>> + if (fd >= 0)\n>>> + if (write_cache(fd, active_cache, active_nr) == 0) {\n>>> + close_lock_file(&index_lock);\n>>\n>> If write_cache() returns a negative value, index.lock is probably\n>> corrupted. Should we die() instead of moving on and returning\n>> index_lock.filename to the caller? The caller may move index.lock to\n>> index later on and officially ruin \"index\".\n>\n> Perhaps true, but worse yet, this will not play nicely together with\n> your split index series, no? After taking the lock and writing and\n> closing, we spawn the interactive while still holding the lock, and\n> the \"open\" we see here is because we want to further update the same\n> under the same lock. Perhaps write_locked_index() API in the split\n> index series can notice that the underlying fd in index_lock has\n> been closed earlier, realize that the call is to re-update the\n> index under the same lock and open the file again for writing?\n>\n> Then we can update the above \"open() then write_cache()\" sequence\n> with just a call to \"write_locked_index()\".\n\nOr perhaps introduce reopen_locked_file() and have these rare\ncallers that want to re-update after they closed the file call it,\nin order to avoid mistake?\n\nPerhaps something like this on top of your series?\n\n-- >8 --\nSubject: [PATCH] lockfile: allow reopening a closed but still locked file\n\nIn some code paths (e.g. giving \"add -i\" to prepare the contents to\nbe committed interactively inside \"commit -p\") where a caller takes\na lock, writes the new content, give chance for others to use it\nwhile still holding the lock, and then releases the lock when all is\ndone. As an extension, allow the caller to re-update an already\nclosed file while still holding the lock (i.e. not yet committed) by\nre-opening the file, to be followed by updating the contents and\nthen by the usual close_lock_file() or commit_lock_file().\n\nThis is necessary if we want to add code to rebuild the cache-tree\nand write the resulting index out after \"add -i\" returns the control\nto \"commit -p\", for example.\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 7 ++++---\n lockfile.c | 51 +++++++++++++++++++++++++++++++++++++--------------\n 2 files changed, 41 insertions(+), 17 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c6b7770..a9344cf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -567,12 +567,13 @@ extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\n-extern void update_index_if_able(struct index_state *, struct lock_file *);\n+extern int reopen_lock_file(struct lock_file *);\n+extern int close_lock_file(struct lock_file *);\n+extern void rollback_lock_file(struct lock_file *);\n \n+extern void update_index_if_able(struct index_state *, struct lock_file *);\n extern int hold_locked_index(struct lock_file *, int);\n extern void set_alternate_index_output(const char *);\n-extern int close_lock_file(struct lock_file *);\n-extern void rollback_lock_file(struct lock_file *);\n extern int delete_ref(const char *, const unsigned char *sha1, int delopt);\n \n /* Environment bits from configuration mechanism */\ndiff --git a/lockfile.c b/lockfile.c\nindex b706614..618a4b2 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -120,21 +120,8 @@ static char *resolve_symlink(char *p, size_t s)\n \treturn p;\n }\n \n-\n-static int lock_file(struct lock_file *lk, const char *path, int flags)\n+static int lock_file_open(struct lock_file *lk)\n {\n-\t/*\n-\t * subtract 5 from size to make sure there's room for adding\n-\t * \".lock\" for the lock file name\n-\t */\n-\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n-\n-\tif (strlen(path) >= max_path_len)\n-\t\treturn -1;\n-\tstrcpy(lk->filename, path);\n-\tif (!(flags & LOCK_NODEREF))\n-\t\tresolve_symlink(lk->filename, max_path_len);\n-\tstrcat(lk->filename, \".lock\");\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n \t\tif (!lock_file_list) {\n@@ -156,6 +143,23 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \treturn lk->fd;\n }\n \n+static int lock_file(struct lock_file *lk, const char *path, int flags)\n+{\n+\t/*\n+\t * subtract 5 from size to make sure there's room for adding\n+\t * \".lock\" for the lock file name\n+\t */\n+\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n+\n+\tif (strlen(path) >= max_path_len)\n+\t\treturn -1;\n+\tstrcpy(lk->filename, path);\n+\tif (!(flags & LOCK_NODEREF))\n+\t\tresolve_symlink(lk->filename, max_path_len);\n+\tstrcat(lk->filename, \".lock\");\n+\treturn lock_file_open(lk);\n+}\n+\n static char *unable_to_lock_message(const char *path, int err)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -221,6 +225,25 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \treturn fd;\n }\n \n+/*\n+ * After calling close_lock_file() to flush the contents to the\n+ * disk, without releasing the lock, this can be called to reopen\n+ * to update the contents of the file. An expected calling sequence\n+ * is\n+ *\n+ * hold_lock_file(lk), write(lk->fd), close_lock_file(lk),\n+ * reopen_lock_file(lk), write(lk->fd), close_lock_file(lk),\n+ * commit_lock_file(lk).\n+ */\n+int reopen_lock_file(struct lock_file *lk)\n+{\n+\tif (0 <= lk->fd)\n+\t\tdie(_(\"BUG: attempt to reopen an unclosed lockfile\"));\n+\tif (!lk->filename[0])\n+\t\tdie(_(\"BUG: attempt to reopen a committed lockfile\"));\n+\treturn lock_file_open(lk);\n+}\n+\n int close_lock_file(struct lock_file *lk)\n {\n \tint fd = lk->fd;\n-- \n2.0.1-812-g56968e3\n"},{"id":"245998","messageId":"xmqqvbqzygkm.fsf@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"53C41456.2000006@ramsay1.demon.co.uk","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-14T17:51:21Z","receivedAt":"2014-07-14T17:51:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> that the merge commit 7608c87e fails. Looking at the details of the\n> merge resolution, made me think of Duy's split index work.\n\nYes, there is a deliberately dropped hunk from dt/cache-tree-repair\nin that merge, because the topic relied on being able to say \"here\nis the file descriptor, write the index to it\", which no longer is\navailable with the split-index topic.\n"},{"id":"246002","messageId":"53C42453.1090109@ramsay1.demon.co.uk","threadId":"37121","inReplyTo":"xmqqvbqzygkm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-14T18:41:23Z","receivedAt":"2014-07-14T18:41:23Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 14/07/14 18:51, Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n>> that the merge commit 7608c87e fails. Looking at the details of the\n>> merge resolution, made me think of Duy's split index work.\n> \n> Yes, there is a deliberately dropped hunk from dt/cache-tree-repair\n> in that merge, because the topic relied on being able to say \"here\n> is the file descriptor, write the index to it\", which no longer is\n> available with the split-index topic.\n\nAh, OK. Sounds like everything is under control then.\n\nThanks.\n\nATB,\nRamsay Jones\n"},{"id":"246007","messageId":"xmqqr41ny9pd.fsf_-_@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"xmqqzjgbygy3.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] lockfile: allow reopening a closed but still locked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-14T20:19:42Z","receivedAt":"2014-07-14T20:19:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In some code paths (e.g. giving \"add -i\" to prepare the contents to\nbe committed interactively inside \"commit -p\") where a caller takes\na lock, writes the new content, give chance for others to use it\nwhile still holding the lock, and then releases the lock when all is\ndone. As an extension, allow the caller to re-update an already\nclosed file while still holding the lock (i.e. not yet committed) by\nre-opening the file, to be followed by updating the contents and\nthen by the usual close_lock_file() or commit_lock_file().\n\nThis is necessary if we want to add code to rebuild the cache-tree\nand write the resulting index out after \"add -i\" returns the control\nto \"commit -p\", for example.\n\nSigned-off-by: Junio C Hamano \n---\n\n * I was being silly in the first attemt as usual ;-) There is not\n much to be shared between the reopen and initial open code path\n in that we do not want to give O_EXCL to open(), we know the file\n exists so O_CREAT is not necessary, we know we have added it to\n the unlocked-atexit list and we already know that we have futzed\n with its permission bits.\n\n With this added on top of the nd/split-index series, the conflict\n resolution for the \"interactive\" codepath in builtin/commit.c\n with dt/cache-tree-repair topic would become like this:\n\n @@ -340,6 +340,13 @@ static char *prepare_index(int argc, const char\n\n discard_cache();\n read_cache_from(index_lock.filename);\n + if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n + if (reopen_lock_file(&index_lock) < 0)\n + die(_(\"unable to write index file\"));\n + if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n + die(_(\"unable to update temporary index\"));\n + } else\n + warning(_(\"Failed to update main cache tree\"));\n\n commit_style = COMMIT_NORMAL;\n return index_lock.filename;\n\n cache.h | 1 +\n lockfile.c | 10 ++++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex c6b7770..b780794 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -567,6 +567,7 @@ extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\n+extern int reopen_lock_file(struct lock_file *);\n extern void update_index_if_able(struct index_state *, struct lock_file *);\n \n extern int hold_locked_index(struct lock_file *, int);\ndiff --git a/lockfile.c b/lockfile.c\nindex b706614..9c12ec5 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -228,6 +228,16 @@ int close_lock_file(struct lock_file *lk)\n \treturn close(fd);\n }\n \n+int reopen_lock_file(struct lock_file *lk)\n+{\n+\tif (0 <= lk->fd)\n+\t\tdie(_(\"BUG: reopen a lockfile that is still open\"));\n+\tif (!lk->filename[0])\n+\t\tdie(_(\"BUG: reopen a lockfile that has been committed\"));\n+\tlk->fd = open(lk->filename, O_WRONLY);\n+\treturn lk->fd;\n+}\n+\n int commit_lock_file(struct lock_file *lk)\n {\n \tchar result_file[PATH_MAX];\n-- \n2.0.1-814-g49d294e\n"},{"id":"246036","messageId":"xmqq8unvy4bf.fsf@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"53C42453.1090109@ramsay1.demon.co.uk","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-14T22:16:04Z","receivedAt":"2014-07-14T22:16:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> On 14/07/14 18:51, Junio C Hamano wrote:\n>> Ramsay Jones writes:\n>> \n>>> that the merge commit 7608c87e fails. Looking at the details of the\n>>> merge resolution, made me think of Duy's split index work.\n>> \n>> Yes, there is a deliberately dropped hunk from dt/cache-tree-repair\n>> in that merge, because the topic relied on being able to say \"here\n>> is the file descriptor, write the index to it\", which no longer is\n>> available with the split-index topic.\n>\n> Ah, OK. Sounds like everything is under control then.\n\nWasn't, but now I think it is ;-)\n\nDavid, could you please double check the conflict resolution at\n882426ea (Merge branch 'dt/cache-tree-repair' into jch, 2014-07-14),\nat about the middle between master..pu? By eyeballing\n\n git diff 882426ea^ 882426ea\n\nwe should see what your series would have done if it were based on\ntop of the nd/split-index topic. The most iffy is the first hunk of\nchange to builtin/commit.c, which is more or less my rewrite of what\nyou did on top of 'master'.\n\nThe change to builtin/checkout.c also seems somewhat iffy in that we\ntreat the_index.cache_tree (aka \"active_cache_tree\") as if cache\ntrees are something we can manipulate independent of a particular\nindex_state (which has been the rule for a long time), even though\nin the world order after nd/split-index topic, cache_tree_update()\ncan no longer be used on a cache-tree that is not associated to a\nparticular index_state. It is not a problem with your series, but\ncomes from nd/split-index topic, and it might indicate a slight\nunevenness of the API (i.e. we may want to either insist that the\npublic API to muck with a cache-tree outside cache-tree.c must be\naccessed via an index-state and never via a bare cache-tree\nstructure, by insisting that cache_tree_fully_valid() to take a\npointer to an index-state as well; or we may want to go the other\nway and allow API users to pass a bare cache-tree without the\nindex-state when the latter is not absolutely necessary, by changing\ncache_tree_update() to take a cache-tree, not an index-state).\n"},{"id":"246039","messageId":"1405377128.3775.34.camel@stross","threadId":"37121","inReplyTo":"xmqq8unvy4bf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-07-14T22:32:08Z","receivedAt":"2014-07-14T22:32:08Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2014-07-14 at 15:16 -0700, Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n> > On 14/07/14 18:51, Junio C Hamano wrote:\n> >> Ramsay Jones writes:\n> >> \n> >>> that the merge commit 7608c87e fails. Looking at the details of the\n> >>> merge resolution, made me think of Duy's split index work.\n> >> \n> >> Yes, there is a deliberately dropped hunk from dt/cache-tree-repair\n> >> in that merge, because the topic relied on being able to say \"here\n> >> is the file descriptor, write the index to it\", which no longer is\n> >> available with the split-index topic.\n> >\n> > Ah, OK. Sounds like everything is under control then.\n> \n> Wasn't, but now I think it is ;-)\n> \n> David, could you please double check the conflict resolution at\n> 882426ea (Merge branch 'dt/cache-tree-repair' into jch, 2014-07-14),\n> at about the middle between master..pu? By eyeballing\n> \n> git diff 882426ea^ 882426ea\n> \n> we should see what your series would have done if it were based on\n> top of the nd/split-index topic. The most iffy is the first hunk of\n> change to builtin/commit.c, which is more or less my rewrite of what\n> you did on top of 'master'.\n\nLGTM. And the tests all pass.\n\n> The change to builtin/checkout.c also seems somewhat iffy in that we\n> treat the_index.cache_tree (aka \"active_cache_tree\") as if cache\n> trees are something we can manipulate independent of a particular\n> index_state (which has been the rule for a long time), even though\n> in the world order after nd/split-index topic, cache_tree_update()\n> can no longer be used on a cache-tree that is not associated to a\n> particular index_state. It is not a problem with your series, but\n> comes from nd/split-index topic, and it might indicate a slight\n> unevenness of the API (i.e. we may want to either insist that the\n> public API to muck with a cache-tree outside cache-tree.c must be\n> accessed via an index-state and never via a bare cache-tree\n> structure, by insisting that cache_tree_fully_valid() to take a\n> pointer to an index-state as well; or we may want to go the other\n> way and allow API users to pass a bare cache-tree without the\n> index-state when the latter is not absolutely necessary, by changing\n> cache_tree_update() to take a cache-tree, not an index-state).\n\nI agree that some sort of API updates like would be an improvement. But\nthis seems to work for now.\n"},{"id":"246041","messageId":"CACsJy8BvQYyXWB7YfoonfEeqi9gStEZHqKC_JNAhxZbLJas54w@mail.gmail.com","threadId":"37121","inReplyTo":"xmqq8unvy4bf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-15T02:15:53Z","receivedAt":"2014-07-15T02:15:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 15, 2014 at 5:16 AM, Junio C Hamano wrote:\n> Ramsay Jones writes:\n>\n>> On 14/07/14 18:51, Junio C Hamano wrote:\n>>> Ramsay Jones writes:\n>>>\n>>>> that the merge commit 7608c87e fails. Looking at the details of the\n>>>> merge resolution, made me think of Duy's split index work.\n>>>\n>>> Yes, there is a deliberately dropped hunk from dt/cache-tree-repair\n>>> in that merge, because the topic relied on being able to say \"here\n>>> is the file descriptor, write the index to it\", which no longer is\n>>> available with the split-index topic.\n>>\n>> Ah, OK. Sounds like everything is under control then.\n>\n> Wasn't, but now I think it is ;-)\n>\n> David, could you please double check the conflict resolution at\n> 882426ea (Merge branch 'dt/cache-tree-repair' into jch, 2014-07-14),\n> at about the middle between master..pu? By eyeballing\n>\n> git diff 882426ea^ 882426ea\n>\n> we should see what your series would have done if it were based on\n> top of the nd/split-index topic. The most iffy is the first hunk of\n> change to builtin/commit.c, which is more or less my rewrite of what\n> you did on top of 'master'.\n\nIt makes me wonder if a cleaner way of rebuilding cache-treei in this\ncase is from git-add--interactive.perl, or by simply spawn \"git\nupdate-index --rebuild-cache-tree\" after running\ngit-add--interactive.perl.\n-- \nDuy\n"},{"id":"246043","messageId":"CAPc5daVH=i72Y4dA8TefPLfFB79Cvw7STPnQf_f10cBeYbg2ug@mail.gmail.com","threadId":"37121","inReplyTo":"CACsJy8BvQYyXWB7YfoonfEeqi9gStEZHqKC_JNAhxZbLJas54w@mail.gmail.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T06:38:06Z","receivedAt":"2014-07-15T06:38:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Jul 14, 2014 at 7:15 PM, Duy Nguyen wrote:\n>\n> It makes me wonder if a cleaner way of rebuilding cache-treei in this\n> case is from git-add--interactive.perl, or by simply spawn \"git\n> update-index --rebuild-cache-tree\" after running\n> git-add--interactive.perl.\n\nWe could check if the cache-tree has fully been populated by\n\"add -i\" and limit the rebuilding by \"git commit -p\" main process,\nbut if \"add -i\" did not do so, there is no reason why \"git commit -p\"\nshould not do so, without relying on the implementation of \"add -i\"\nto do so.\n\nAt least to me, what you suggested sounds nothing more than\na cop-out; instead of lifting the limitation of the API by enhancing\nit with reopen-lock-file, punting to shift the burden elsewhere. I\nam not quite sure why that is cleaner, but perhaps I am missing\nsomething.\n\nIn the longer run, it would be plausible that somebody would want\nto rewrite \"git-add -i\" and will make it just a C API call away without\nhaving to spawn a separate process. You cannot punt by saying\n\"make 'add -i' responsible for it\" at that point; you will be doing\nwhat 'add -i' would be doing yourself, no?\n"},{"id":"246045","messageId":"20140715102314.GA8273@lanh","threadId":"37121","inReplyTo":"CAPc5daVH=i72Y4dA8TefPLfFB79Cvw7STPnQf_f10cBeYbg2ug@mail.gmail.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-15T10:23:14Z","receivedAt":"2014-07-15T10:23:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 14, 2014 at 11:38:06PM -0700, Junio C Hamano wrote:\n> On Mon, Jul 14, 2014 at 7:15 PM, Duy Nguyen wrote:\n> >\n> > It makes me wonder if a cleaner way of rebuilding cache-treei in this\n> > case is from git-add--interactive.perl, or by simply spawn \"git\n> > update-index --rebuild-cache-tree\" after running\n> > git-add--interactive.perl.\n> \n> We could check if the cache-tree has fully been populated by\n> \"add -i\" and limit the rebuilding by \"git commit -p\" main process,\n> but if \"add -i\" did not do so, there is no reason why \"git commit -p\"\n> should not do so, without relying on the implementation of \"add -i\"\n> to do so.\n> \n> At least to me, what you suggested sounds nothing more than\n> a cop-out; instead of lifting the limitation of the API by enhancing\n> it with reopen-lock-file, punting to shift the burden elsewhere. I\n> am not quite sure why that is cleaner, but perhaps I am missing\n> something.\n\nReopen-lock-file to me does not sound like an enhancement, at least in\nthe index update context. We have always updated the index by writing\nto a new file, then rename. Now if the new write_locked_index() blows\nup after reopen_lock_file(), we have no way but die() because\nindex_lock.filename can no longer be trusted.\n\nDoing it in a separate process keeps the tradition. And if the second\nwrite fails, we still have good index_lock.filename. We could also do\nthe same here with something like this without new process (lightly\ntested):\n\n-- 8< --\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 606c890..c2b4875 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -340,6 +340,18 @@ 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\tstatic struct lock_file second_index_lock;\n+\t\t\thold_lock_file_for_update(&second_index_lock,\n+\t\t\t\t\t\t index_lock.filename,\n+\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\t\t\tif (write_locked_index(&the_index, &second_index_lock,\n+\t\t\t\t\t COMMIT_LOCK)) {\n+\t\t\t\twarning(_(\"Failed to update main cache tree\"));\n+\t\t\t\trollback_lock_file(&second_index_lock);\n+\t\t\t}\n+\t\t} else\n+\t\t\twarning(_(\"Failed to update main cache tree\"));\n \n \t\tcommit_style = COMMIT_NORMAL;\n \t\treturn index_lock.filename;\n-- 8< --\n\nI was worried about the dependency between second_index_lock and\nindex_lock, but at cleanup, all we do is rollback, which is safe\nregardless of dependencies. Just need to make sure we commit\nsecond_index_lock before index_lock.\n\n> In the longer run, it would be plausible that somebody would want\n> to rewrite \"git-add -i\" and will make it just a C API call away without\n> having to spawn a separate process. You cannot punt by saying\n> \"make 'add -i' responsible for it\" at that point; you will be doing\n> what 'add -i' would be doing yourself, no?\n\nYes, but at current rewrite rate I'd bet it won't happen in the next\ntwo years at least.\n--\nDuy\n"},{"id":"246067","messageId":"xmqqbnsqwoxz.fsf@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"20140715102314.GA8273@lanh","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-15T16:45:44Z","receivedAt":"2014-07-15T16:45:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Mon, Jul 14, 2014 at 11:38:06PM -0700, Junio C Hamano wrote:\n>> On Mon, Jul 14, 2014 at 7:15 PM, Duy Nguyen wrote:\n>> >\n>> > It makes me wonder if a cleaner way of rebuilding cache-treei in this\n>> > case is from git-add--interactive.perl, or by simply spawn \"git\n>> > update-index --rebuild-cache-tree\" after running\n>> > git-add--interactive.perl.\n>> \n>> We could check if the cache-tree has fully been populated by\n>> \"add -i\" and limit the rebuilding by \"git commit -p\" main process,\n>> but if \"add -i\" did not do so, there is no reason why \"git commit -p\"\n>> should not do so, without relying on the implementation of \"add -i\"\n>> to do so.\n>> \n>> At least to me, what you suggested sounds nothing more than\n>> a cop-out; instead of lifting the limitation of the API by enhancing\n>> it with reopen-lock-file, punting to shift the burden elsewhere. I\n>> am not quite sure why that is cleaner, but perhaps I am missing\n>> something.\n>\n> Reopen-lock-file to me does not sound like an enhancement, at least in\n> the index update context. We have always updated the index by writing\n> to a new file, then rename.\n\nTime to step back and think, I think.\n\nWhat is the real point of \"writing into *.lock and renaming\"? It\nserves two purposes: (1) everybody adheres to that convention---if\nwe managed to take the lock \"index.lock\", nobody else will compete\nand interfere with us until we rename it to \"index\". (2) in the\nmeantime, others can still read the old contents in the original\n\"index\".\n\nWith \"take lock\", \"write to a temporary\", \"commit by rename or\nrollback by delete\", we can have the usual transactional semantics.\nWhile we are working on it, nobody is allowed to muck with the same\nfile, because everybody pays attention to *.lock. People will not\nsee what we are doing until we release the lock because we are not\nwriting into the primary file. And people can see what we did in\nits entirety once we are done because we close and rename to commit\nour changes atomically.\n\nThink what CLOSE_LOCK adds to that and you would appreciate its\nusefulness and at the same time realize its limitation. By allowing\nus to flush what we wrote to the disk without releasing the lock, we\ncan give others (e.g. subprograms we spawn) controlled access to the\nnew contents we prepared before we commit the result to the outside\nworld. The access is controlled because we are in control when we\ntell these others to peek into or update the temporary file \"*.lock\".\n\nThe original implementaiton of CLOSE_LOCK is limited in that you can\ndo so only once; you take a lock, you do your thing, you close, you\nlet (one or more) others see it, and you commit (or rollback). You\ncannot do another of your thing once you close with the original\nimplementation because there was no way to reopen.\n\nWhat do you gain by your proposal to lock \"index.lock\" file? We\nknow we already have \"index.lock\", so nobody should be competing on\nmucking with its contents with us and we gain nothing by writing\ninto index.lock.lock and renaming it to index.lock. We are in total\ncontrol of the lifetime of index.lock, when we spawn subprograms on\nit to let them write to it, when we ourselves write to it, when we\nspawn subprograms on it to let them read from it, all under the lock\nwe have on the \"index\", i.e. \"index.lock\".\n\nThe only thing use of another temporary file (that does not have to\nbe a lock on \"index.lock\", by the way, because we have total control\nover when and who updates the file while we hold the \"index.lock\")\nwould give you is that it allows you to make the success of the \"do\nanother of your thing\" step optional. While holding the lock, you\nclose and let \"add -i\" work on it, and after it returns, instead of\nreopening, you write into yet another \"index.lock.lock\", expecting\nthat it might fail and when it fails you can roll it back, leaving\nthe contents \"add -i\" left in \"index.lock\" intact. If you do not\nuse the extra temporary file, you start from \"index.lock\" left by\n\"add -i\", write the updated index into \"index.lock\" and if you fail\nto write, you have to roll back the entire \"index\"---you lose the\noption to use the index left by \"add -i\" without repopulated\ncache-tree. But in the index update context, I do not think such a\ncomplexity is not necessary. If something fails, we should fail and\nroll back the entire \"index\".\n\n> Now if the new write_locked_index() blows\n> up after reopen_lock_file(), we have no way but die() because\n> index_lock.filename can no longer be trusted.\n\nIf that is the case, lock.filename[] is getting clobbered by\nsomebody too early, isn't it? I think Michael's mh/lockfile topic\n(dropped from my tree due to getting stale without rerolling) used a\nseparate flag to indicate the validity without mucking with the\nfilename member, and we may want to do something like that as the\nright solution to that problem.\n"},{"id":"246153","messageId":"CACsJy8BsoYP40K7vFoZhgXvy32vkHjhwCMYLFM6ishXcVXdzAA@mail.gmail.com","threadId":"37121","inReplyTo":"xmqqbnsqwoxz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-16T10:18:31Z","receivedAt":"2014-07-16T10:18:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 15, 2014 at 11:45 PM, Junio C Hamano wrote:\n> What is the real point of \"writing into *.lock and renaming\"? It\n> serves two purposes: (1) everybody adheres to that convention---if\n> we managed to take the lock \"index.lock\", nobody else will compete\n> and interfere with us until we rename it to \"index\". (2) in the\n> meantime, others can still read the old contents in the original\n> \"index\".\n>\n> With \"take lock\", \"write to a temporary\", \"commit by rename or\n> rollback by delete\", we can have the usual transactional semantics.\n> While we are working on it, nobody is allowed to muck with the same\n> file, because everybody pays attention to *.lock. People will not\n> see what we are doing until we release the lock because we are not\n> writing into the primary file. And people can see what we did in\n> its entirety once we are done because we close and rename to commit\n> our changes atomically.\n\nTrue.\n\n> Think what CLOSE_LOCK adds to that and you would appreciate its\n> usefulness and at the same time realize its limitation. By allowing\n> us to flush what we wrote to the disk without releasing the lock, we\n> can give others (e.g. subprograms we spawn) controlled access to the\n> new contents we prepared before we commit the result to the outside\n> world. The access is controlled because we are in control when we\n> tell these others to peek into or update the temporary file \"*.lock\".\n>\n> The original implementaiton of CLOSE_LOCK is limited in that you can\n> do so only once; you take a lock, you do your thing, you close, you\n> let (one or more) others see it, and you commit (or rollback). You\n> cannot do another of your thing once you close with the original\n> implementation because there was no way to reopen.\n\nThis is probably where our opinions differ. Yes, if you are sure\nnobody else is looking at the lock file any more, then you can do\nwhatever you want. And because this is a .lock file, nobody is\nsupposed to look at it unless you tell them too (in contrast\n$GIT_DIR/index can be read at any time). The format of the index makes\nit impossible to just edit one byte and be done with it. You always\nwrite a full new file. By sticking to transaction-style update, you\nneed no extra code, and you have a back up file as a side effect.\n\n> What do you gain by your proposal to lock \"index.lock\" file? We\n> know we already have \"index.lock\", so nobody should be competing on\n> mucking with its contents with us and we gain nothing by writing\n> into index.lock.lock and renaming it to index.lock. We are in total\n> control of the lifetime of index.lock, when we spawn subprograms on\n> it to let them write to it, when we ourselves write to it, when we\n> spawn subprograms on it to let them read from it, all under the lock\n> we have on the \"index\", i.e. \"index.lock\".\n>\n> The only thing use of another temporary file (that does not have to\n> be a lock on \"index.lock\", by the way, because we have total control\n> over when and who updates the file while we hold the \"index.lock\")\n> would give you is that it allows you to make the success of the \"do\n> another of your thing\" step optional. While holding the lock, you\n> close and let \"add -i\" work on it, and after it returns, instead of\n> reopening, you write into yet another \"index.lock.lock\", expecting\n> that it might fail and when it fails you can roll it back, leaving\n> the contents \"add -i\" left in \"index.lock\" intact. If you do not\n> use the extra temporary file, you start from \"index.lock\" left by\n> \"add -i\", write the updated index into \"index.lock\" and if you fail\n> to write, you have to roll back the entire \"index\"---you lose the\n> option to use the index left by \"add -i\" without repopulated\n> cache-tree. But in the index update context, I do not think such a\n> complexity is not necessary. If something fails, we should fail and\n> roll back the entire \"index\".\n\nI probably look at the problem from a wrong angle. To me the result of\n\"commit -p\" is precious. I'm not a big user of \"commit -p\" myself as I\nprefer \"add -p\" but it's the same: it'd be frustrating if after you\nhave carefully added your chunks, the program aborts and you have to\nstart over. And not just a few chunks. Think of reviewing .po files\nand approve strings by the means of adding them to the index. Perhaps\nbecause _I_ as a developer see this cache-tree update step optional\nand react to it unnecessarily. Ordinary users won't see any\ndifference. And perhaps a better way to save the result of \"commit/add\n-p\" is some sort of index log, not be over-protective at this\n\"interactive commit\" code block.\n\nI don't feel strongly either way. So your call.\n-- \nDuy\n"},{"id":"246172","messageId":"xmqq38e1tdi7.fsf@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"CACsJy8BsoYP40K7vFoZhgXvy32vkHjhwCMYLFM6ishXcVXdzAA@mail.gmail.com","subject":"Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-16T17:33:20Z","receivedAt":"2014-07-16T17:33:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> .... If you do not\n>> use the extra temporary file, you start from \"index.lock\" left by\n>> \"add -i\", write the updated index into \"index.lock\" and if you fail\n>> to write, you have to roll back the entire \"index\"---you lose the\n>> option to use the index left by \"add -i\" without repopulated\n>> cache-tree. But in the index update context, I do not think such a\n>> complexity is not necessary. If something fails, we should fail and\n>> roll back the entire \"index\".\n>\n> I probably look at the problem from a wrong angle. To me the result of\n> \"commit -p\" is precious. I'm not a big user of \"commit -p\" myself as I\n> prefer \"add -p\" but it's the same...\n\nOh, we agree that the result of \"commit -p\" is precious.\n\nBut the point of David's series is to change the definition of the\n\"precious result\" to not just \"commit is made as asked\", but now\nalso to include that \"the index the user will use for continued work\nwill have populated cache-tree\". The series thinks both are precious.\n\nAs you can probably read from my review responses, I am not sold to\nthe idea that spending extra cycles to pre-populate cache-tree is\n100% good idea, but if we _were_ to accept that it is a good idea,\nit logically follows that failing to populate cache-tree is just as\na failure as failing to commit.\n\nIn any case, it is unlikely for writing out the updated index with\nrefreshed cache-tree to fail and blow away the partially built index\n(we do not even attempt to reopen/update if we cannot prepare\nin-core cache-tree), so I do not think it is such a big deal either\nway.\n"},{"id":"248680","messageId":"20140831120703.GA1240@serenity.lan","threadId":"37121","inReplyTo":"1405140276-32162-1-git-send-email-dturner@twitter.com","subject":"Re: [PATCH v8 1/4] cache-tree: Create/update cache-tree on checkou","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-08-31T12:07:03Z","receivedAt":"2014-08-31T12:07:03Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jul 11, 2014 at 09:44:33PM -0700, David Turner wrote:\n> When git checkout checks out a branch, create or update the\n> cache-tree so that subsequent operations are faster.\n> \n> update_main_cache_tree learned a new flag, WRITE_TREE_REPAIR. When\n> WRITE_TREE_REPAIR is set, portions of the cache-tree which do not\n> correspond to existing tree objects are invalidated (and portions which\n> do are marked as valid). No new tree objects are created.\n> \n> Signed-off-by: David Turner \n> ---\n\nThis causes an incorrect error message to be printed when switching\nbranches with staged changes in a subdirectory. The test case is pretty\nsimple:\n\n\tgit init test &&\n\tcd test &&\n\tmkdir sub &&\n\techo one >sub/one &&\n\tgit add sub/one &&\n\tgit commit -m one &&\n\techo two >sub/two &&\n\tgit add sub/two &&\n\tgit checkout -b test\n\nAfter this commit the output is:\n\n\terror: invalid object 040000 0000000000000000000000000000000000000000 for 'bar'\n\tA bar/quux\n\tSwitched to branch 'test'\n\nbut the \"error:\" line should not be there.\n\nEverything still works, but I think the error message is a bit scary\nconsidering that this isn't actually an error.\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> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 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\"));\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 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);\n> diff --git a/cache-tree.h b/cache-tree.h\n> index 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)\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> -- \n> 2.0.0.390.gcb682f8\n"},{"id":"248705","messageId":"1409604568.3057.8.camel@leckie","threadId":"37121","inReplyTo":"20140831120703.GA1240@serenity.lan","subject":"Re: [PATCH v8 1/4] cache-tree: Create/update cache-tree on checkou","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-09-01T20:49:28Z","receivedAt":"2014-09-01T20:49:28Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Sun, 2014-08-31 at 13:07 +0100, John Keeping wrote:\n> On Fri, Jul 11, 2014 at 09:44:33PM -0700, David Turner wrote:\n> > When git checkout checks out a branch, create or update the\n> > cache-tree so that subsequent operations are faster.\n> > \n> > update_main_cache_tree learned a new flag, WRITE_TREE_REPAIR. When\n> > WRITE_TREE_REPAIR is set, portions of the cache-tree which do not\n> > correspond to existing tree objects are invalidated (and portions which\n> > do are marked as valid). No new tree objects are created.\n> > \n> > Signed-off-by: David Turner \n> > ---\n> \n> This causes an incorrect error message to be printed when switching\n> branches with staged changes in a subdirectory. The test case is pretty\n> simple:\n\n\nI tried to reproduce this problem, but I could not. I tried to\nreproduce against just this patch 1/4, and also against all 4 of the\npatches. Can you reproduce this on Junio's 'next' branch, which\nincludes this series of patches?\n\nDo you have some sort of unusual configuration that might cause\ndifferent results? \n\nThanks.\n"},{"id":"248706","messageId":"20140901221357.GD1240@serenity.lan","threadId":"37121","inReplyTo":"1409604568.3057.8.camel@leckie","subject":"Re: [PATCH v8 1/4] cache-tree: Create/update cache-tree on checkou","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-09-01T22:13:57Z","receivedAt":"2014-09-01T22:13:57Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Sep 01, 2014 at 04:49:28PM -0400, David Turner wrote:\n> On Sun, 2014-08-31 at 13:07 +0100, John Keeping wrote:\n> > On Fri, Jul 11, 2014 at 09:44:33PM -0700, David Turner wrote:\n> > > When git checkout checks out a branch, create or update the\n> > > cache-tree so that subsequent operations are faster.\n> > > \n> > > update_main_cache_tree learned a new flag, WRITE_TREE_REPAIR. When\n> > > WRITE_TREE_REPAIR is set, portions of the cache-tree which do not\n> > > correspond to existing tree objects are invalidated (and portions which\n> > > do are marked as valid). No new tree objects are created.\n> > > \n> > > Signed-off-by: David Turner \n> > > ---\n> > \n> > This causes an incorrect error message to be printed when switching\n> > branches with staged changes in a subdirectory. The test case is pretty\n> > simple:\n> \n> \n> I tried to reproduce this problem, but I could not. I tried to\n> reproduce against just this patch 1/4, and also against all 4 of the\n> patches. Can you reproduce this on Junio's 'next' branch, which\n> includes this series of patches?\n> \n> Do you have some sort of unusual configuration that might cause\n> different results? \n\nI don't think so. I wondered if \"status.branch=true\" in the config\ncould do it, but there's no difference turning that off. The rest of\nthe config looks like this:\n\n-- >8 --\ncore.excludesfile=~/.gitexcludes\npush.default=upstream\ncolor.ui=auto\n[snip alias.*]\n[snip user.*]\n[snip url.*.insteadof]\n[snip sendemail.*]\nrebase.autosquash=true\nmailinfo.scissors=true\nformat.thread=true\nrerere.enabled=true\nstatus.branch=true\npull.ff=true\ncredential.helper=cache\ndiff.tool=vimdiff\nmerge.tool=vimdiff\ncore.repositoryformatversion=0\ncore.filemode=true\ncore.bare=false\ncore.logallrefupdates=true\n-- 8< --\n\nThe following script (run from the Git source directory) bisects down\nto:\n\n\taecf567cbfb6ab46e82f7f5df36fb6a2dd5bee69 is the first bad commit\n\tcommit aecf567cbfb6ab46e82f7f5df36fb6a2dd5bee69\n\tAuthor: David Turner \n\tDate: Sat Jul 5 21:06:56 2014 -0700\n\n\t cache-tree: create/update cache-tree on checkout\n\n-- >8 --\n#!/bin/sh\ngit init test &&\n(\n\tcd test &&\n\tmkdir sub &&\n\techo one >sub/one &&\n\tgit add sub/one &&\n\tgit commit -m one &&\n\techo two >sub/two &&\n\tgit add sub/two &&\n\tgit branch other\n) &&\ncat >BISECT.sh <<\\EOF &&\n#!/bin/sh\nmake -j4 &&\ncd test &&\n../bin-wrappers/git checkout other 2>&1 | grep 'error:'\nresult=$?\ngit checkout master\nif test $result = 0\nthen\n\texit 1\nelse\n\texit 0\nfi\nEOF\nchmod +x BISECT.sh &&\ngit bisect start origin/next origin/master &&\ngit bisect run ./BISECT.sh\n"},{"id":"248737","messageId":"xmqqfvg9zqw9.fsf@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"20140831120703.GA1240@serenity.lan","subject":"Re: [PATCH v8 1/4] cache-tree: Create/update cache-tree on checkou","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-02T20:52:38Z","receivedAt":"2014-09-02T20:52:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> On Fri, Jul 11, 2014 at 09:44:33PM -0700, David Turner wrote:\n>> When git checkout checks out a branch, create or update the\n>> cache-tree so that subsequent operations are faster.\n>> \n>> update_main_cache_tree learned a new flag, WRITE_TREE_REPAIR. When\n>> WRITE_TREE_REPAIR is set, portions of the cache-tree which do not\n>> correspond to existing tree objects are invalidated (and portions which\n>> do are marked as valid). No new tree objects are created.\n>> \n>> Signed-off-by: David Turner \n>> ---\n>\n> This causes an incorrect error message to be printed when switching\n> branches with staged changes in a subdirectory. The test case is pretty\n> simple:\n>\n> \tgit init test &&\n> \tcd test &&\n> \tmkdir sub &&\n> \techo one >sub/one &&\n> \tgit add sub/one &&\n> \tgit commit -m one &&\n> \techo two >sub/two &&\n> \tgit add sub/two &&\n> \tgit checkout -b test\n>\n> After this commit the output is:\n>\n> \terror: invalid object 040000 0000000000000000000000000000000000000000 for 'bar'\n> \tA bar/quux\n> \tSwitched to branch 'test'\n>\n> but the \"error:\" line should not be there.\n\nYeah, this seems to be broken and I am unhappy that I didn't notice\nit myself as I always use a version that is somewhat ahead of 'next'\nmyself.\n"},{"id":"248760","messageId":"xmqq8um1zq1q.fsf@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"xmqqfvg9zqw9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 1/4] cache-tree: Create/update cache-tree on checkou","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-02T21:10:57Z","receivedAt":"2014-09-02T21:10:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> John Keeping writes:\n>\n>> On Fri, Jul 11, 2014 at 09:44:33PM -0700, David Turner wrote:\n>>> When git checkout checks out a branch, create or update the\n>>> cache-tree so that subsequent operations are faster.\n>>> \n>>> update_main_cache_tree learned a new flag, WRITE_TREE_REPAIR. When\n>>> WRITE_TREE_REPAIR is set, portions of the cache-tree which do not\n>>> correspond to existing tree objects are invalidated (and portions which\n>>> do are marked as valid). No new tree objects are created.\n>>> \n>>> Signed-off-by: David Turner \n>>> ---\n>>\n>> This causes an incorrect error message to be printed when switching\n>> branches with staged changes in a subdirectory. The test case is pretty\n>> simple:\n>>\n>> \tgit init test &&\n>> \tcd test &&\n>> \tmkdir sub &&\n>> \techo one >sub/one &&\n>> \tgit add sub/one &&\n>> \tgit commit -m one &&\n>> \techo two >sub/two &&\n>> \tgit add sub/two &&\n>> \tgit checkout -b test\n>>\n>> After this commit the output is:\n>>\n>> \terror: invalid object 040000 0000000000000000000000000000000000000000 for 'bar'\n>> \tA bar/quux\n>> \tSwitched to branch 'test'\n>>\n>> but the \"error:\" line should not be there.\n>\n> Yeah, this seems to be broken and I am unhappy that I didn't notice\n> it myself as I always use a version that is somewhat ahead of 'next'\n> myself.\n\nPerhaps like this, to make sure that we do not throw a garbage\nobject name into the cache tree when we avoid creating an unwanted\ntree object?\n\nAll the tests added by the series seems to pass, so I am assuming\nthat this will not break the \"repair\" codepath when it should kick\nin.\n\nWe may want to add your test to t0090 as well.\n\n cache-tree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex f951d7d..e3baf42 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -398,7 +398,7 @@ static int update_one(struct cache_tree *it,\n \t\tit->entry_count, it->subtree_nr,\n \t\tsha1_to_hex(it->sha1));\n #endif\n-\treturn i;\n+\treturn to_invalidate ? -1 : i;\n }\n \n int cache_tree_update(struct cache_tree *it,\n"},{"id":"248762","messageId":"xmqq4mwpzpfw.fsf_-_@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"xmqq8um1zq1q.fsf@gitster.dls.corp.google.com","subject":"[PATCH] cache-tree: propagate invalidation up when punting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-02T21:24:03Z","receivedAt":"2014-09-02T21:24:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We punt from \"repair\"ing the hash tree during a branch switching if\nit involves having to create a new tree object that does not yet\nexist in the object store. \"mkdir dir && >dir/file && git add dir\"\nfollowed by \"git checkout\" is one example, when a tree that records\nthe state of such \"dir/\" is not in the object store.\n\nHowever, after discovering that we do not have a tree object that\nrecords the state of \"dir/\", we failed to propagate the fact up the\ncallchain to stop the code to attempt populating the level that has\n\"dir/\" as its immediate subdirectory. This led the caller detect\nand report a non-existent error.\n\nReported-by: John Keeping \nSigned-off-by: Junio C Hamano \n---\n cache-tree.c | 2 +-\n t/t0090-cache-tree.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex f951d7d..e3baf42 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -398,7 +398,7 @@ static int update_one(struct cache_tree *it,\n \t\tit->entry_count, it->subtree_nr,\n \t\tsha1_to_hex(it->sha1));\n #endif\n-\treturn i;\n+\treturn to_invalidate ? -1 : i;\n }\n \n int cache_tree_update(struct cache_tree *it,\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 48c4240..f9648a8 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -210,4 +210,12 @@ test_expect_success 'partial commit gives cache-tree' '\n \ttest_cache_tree\n '\n \n+test_expect_success 'no phantom error when switching trees' '\n+\tmkdir newdir &&\n+\t>newdir/one &&\n+\tgit add newdir/one &&\n+\tgit checkout 2>errors &&\n+\t! test -s errors\n+'\n+\n test_done\n-- \n2.1.0-391-g57244f3\n"},{"id":"248764","messageId":"xmqqvbp5y7ef.fsf_-_@gitster.dls.corp.google.com","threadId":"37121","inReplyTo":"xmqq4mwpzpfw.fsf_-_@gitster.dls.corp.google.com","subject":"[PATCH v2] cache-tree: do not try to use an invalidated subtree info to build a tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-02T22:39:04Z","receivedAt":"2014-09-02T22:39:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We punt from repairing the cache-tree during a branch switching if\nit involves having to create a new tree object that does not yet\nexist in the object store. \"mkdir dir && >dir/file && git add dir\"\nfollowed by \"git checkout\" is one example, when a tree that records\nthe state of such \"dir/\" is not in the object store.\n\nHowever, after discovering that we do not have a tree object that\nrecords the state of \"dir/\", the caller failed to remember the fact\nthat it noticed the cache-tree entry it received for \"dir/\" is\ninvalidated, it already knows it should not be populating the level\ncallchain to stop the code to attempt populating the level that has\n\"dir/\" as its immediate subdirectory, and it is not an error at all\nfor the sublevel cache-tree entry gave it a bogus object name it\nshouldn't even look at.\n\nThis led the caller detect and report a non-existent error. The end\nresult was the same and we avoided stuffing a non-existent tree to\nthe cache-tree, but we shouldn't have issued an alarming error\nmessage to the user.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Second try. The level that has intent-to-add entries needs to be\n kept invalidated but the level above it needs to treat as if the\n i-t-a entries do not exist and build the whole tree; a directory\n that does not yet have corresponding tree object while repairing\n the cache-tree needs to invalidate itself *and* propagate the\n (in)validity upwards. They have to be treated differently but\n the first attempt failed to do so.\n\n cache-tree.c | 7 ++++++-\n t/t0090-cache-tree.sh | 8 ++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex f951d7d..57597ac 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -316,6 +316,7 @@ static int update_one(struct cache_tree *it,\n \t\tint pathlen, entlen;\n \t\tconst unsigned char *sha1;\n \t\tunsigned mode;\n+\t\tint expected_missing = 0;\n \n \t\tpath = ce->name;\n \t\tpathlen = ce_namelen(ce);\n@@ -332,8 +333,10 @@ static int update_one(struct cache_tree *it,\n \t\t\ti += sub->count;\n \t\t\tsha1 = sub->cache_tree->sha1;\n \t\t\tmode = S_IFDIR;\n-\t\t\tif (sub->cache_tree->entry_count < 0)\n+\t\t\tif (sub->cache_tree->entry_count < 0) {\n \t\t\t\tto_invalidate = 1;\n+\t\t\t\texpected_missing = 1;\n+\t\t\t}\n \t\t}\n \t\telse {\n \t\t\tsha1 = ce->sha1;\n@@ -343,6 +346,8 @@ static int update_one(struct cache_tree *it,\n \t\t}\n \t\tif (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {\n \t\t\tstrbuf_release(&buffer);\n+\t\t\tif (expected_missing)\n+\t\t\t\treturn -1;\n \t\t\treturn error(\"invalid object %06o %s for '%.*s'\",\n \t\t\t\tmode, sha1_to_hex(sha1), entlen+baselen, path);\n \t\t}\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 48c4240..f9648a8 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -210,4 +210,12 @@ test_expect_success 'partial commit gives cache-tree' '\n \ttest_cache_tree\n '\n \n+test_expect_success 'no phantom error when switching trees' '\n+\tmkdir newdir &&\n+\t>newdir/one &&\n+\tgit add newdir/one &&\n+\tgit checkout 2>errors &&\n+\t! test -s errors\n+'\n+\n test_done\n-- \n2.1.0-391-g57244f3\n"},{"id":"248768","messageId":"1409712991.3057.10.camel@leckie","threadId":"37121","inReplyTo":"xmqqvbp5y7ef.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] cache-tree: do not try to use an invalidated subtree info to build a tree","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-09-03T02:56:31Z","receivedAt":"2014-09-03T02:56:31Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-09-02 at 15:39 -0700, Junio C Hamano wrote:\n> We punt from repairing the cache-tree during a branch switching if\n> it involves having to create a new tree object that does not yet\n> exist in the object store. \"mkdir dir && >dir/file && git add dir\"\n> followed by \"git checkout\" is one example, when a tree that records\n> the state of such \"dir/\" is not in the object store.\n> \n> However, after discovering that we do not have a tree object that\n> records the state of \"dir/\", the caller failed to remember the fact\n> that it noticed the cache-tree entry it received for \"dir/\" is\n> invalidated, it already knows it should not be populating the level\n> callchain to stop the code to attempt populating the level that has\n> \"dir/\" as its immediate subdirectory, and it is not an error at all\n> for the sublevel cache-tree entry gave it a bogus object name it\n> shouldn't even look at.\n> \n> This led the caller detect and report a non-existent error. The end\n> result was the same and we avoided stuffing a non-existent tree to\n> the cache-tree, but we shouldn't have issued an alarming error\n> message to the user.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * Second try. The level that has intent-to-add entries needs to be\n> kept invalidated but the level above it needs to treat as if the\n> i-t-a entries do not exist and build the whole tree; a directory\n> that does not yet have corresponding tree object while repairing\n> the cache-tree needs to invalidate itself *and* propagate the\n> (in)validity upwards. They have to be treated differently but\n> the first attempt failed to do so.\n> \n> cache-tree.c | 7 ++++++-\n> t/t0090-cache-tree.sh | 8 ++++++++\n> 2 files changed, 14 insertions(+), 1 deletion(-)\n> \n> diff --git a/cache-tree.c b/cache-tree.c\n> index f951d7d..57597ac 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -316,6 +316,7 @@ static int update_one(struct cache_tree *it,\n> \t\tint pathlen, entlen;\n> \t\tconst unsigned char *sha1;\n> \t\tunsigned mode;\n> +\t\tint expected_missing = 0;\n> \n> \t\tpath = ce->name;\n> \t\tpathlen = ce_namelen(ce);\n> @@ -332,8 +333,10 @@ static int update_one(struct cache_tree *it,\n> \t\t\ti += sub->count;\n> \t\t\tsha1 = sub->cache_tree->sha1;\n> \t\t\tmode = S_IFDIR;\n> -\t\t\tif (sub->cache_tree->entry_count < 0)\n> +\t\t\tif (sub->cache_tree->entry_count < 0) {\n> \t\t\t\tto_invalidate = 1;\n> +\t\t\t\texpected_missing = 1;\n> +\t\t\t}\n> \t\t}\n> \t\telse {\n> \t\t\tsha1 = ce->sha1;\n> @@ -343,6 +346,8 @@ static int update_one(struct cache_tree *it,\n> \t\t}\n> \t\tif (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {\n> \t\t\tstrbuf_release(&buffer);\n> +\t\t\tif (expected_missing)\n> +\t\t\t\treturn -1;\n> \t\t\treturn error(\"invalid object %06o %s for '%.*s'\",\n> \t\t\t\tmode, sha1_to_hex(sha1), entlen+baselen, path);\n> \t\t}\n> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n> index 48c4240..f9648a8 100755\n> --- a/t/t0090-cache-tree.sh\n> +++ b/t/t0090-cache-tree.sh\n> @@ -210,4 +210,12 @@ test_expect_success 'partial commit gives cache-tree' '\n> \ttest_cache_tree\n> '\n> \n> +test_expect_success 'no phantom error when switching trees' '\n> +\tmkdir newdir &&\n> +\t>newdir/one &&\n> +\tgit add newdir/one &&\n> +\tgit checkout 2>errors &&\n> +\t! test -s errors\n> +'\n> +\n> test_done\n\nLGTM.\n"},{"id":"248781","messageId":"CAPig+cQQH_W+J7=B4PWfUVfJpSqN69nLtK8PR3BPmokEBpD2cg@mail.gmail.com","threadId":"37121","inReplyTo":"xmqqvbp5y7ef.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] cache-tree: do not try to use an invalidated subtree info to build a tree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-03T12:02:58Z","receivedAt":"2014-09-03T12:02:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 2, 2014 at 6:39 PM, Junio C Hamano wrote:\n> We punt from repairing the cache-tree during a branch switching if\n> it involves having to create a new tree object that does not yet\n> exist in the object store. \"mkdir dir && >dir/file && git add dir\"\n> followed by \"git checkout\" is one example, when a tree that records\n> the state of such \"dir/\" is not in the object store.\n>\n> However, after discovering that we do not have a tree object that\n> records the state of \"dir/\", the caller failed to remember the fact\n> that it noticed the cache-tree entry it received for \"dir/\" is\n> invalidated, it already knows it should not be populating the level\n> callchain to stop the code to attempt populating the level that has\n> \"dir/\" as its immediate subdirectory, and it is not an error at all\n> for the sublevel cache-tree entry gave it a bogus object name it\n> shouldn't even look at.\n>\n> This led the caller detect and report a non-existent error. The end\n\ns/caller/caller to/\n\n> result was the same and we avoided stuffing a non-existent tree to\n> the cache-tree, but we shouldn't have issued an alarming error\n> message to the user.\n>\n> Signed-off-by: Junio C Hamano \n"}]}