# [PATCH 1/3] cache-tree: Create/update cache-tree on checkout 7 messages from 2014-07-01 to 2014-07-01. Participants: David Turner, Junio C Hamano. Thread: https://gitlist.dev/t/37034 ## David Turner, 2014-07-01 19:14 Subject: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout Message-ID: <1404242075-7068-1-git-send-email-dturner@twitter.com> URL: https://gitlist.dev/e/1404242075-7068-1-git-send-email-dturner%40twitter.com ``` When git checkout checks out a branch, create or update the cache-tree so that subsequent operations are faster. Signed-off-by: David Turner --- builtin/checkout.c | 8 ++++++++ cache-tree.c | 5 +++-- t/t0090-cache-tree.sh | 19 ++++++++++++++++--- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 07cf555..a023a86 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts *opts, } } + if (!active_cache_tree) + active_cache_tree = cache_tree(); + + if (!cache_tree_fully_valid(active_cache_tree)) + cache_tree_update(active_cache_tree, + (const struct cache_entry * const *)active_cache, + active_nr, 0); + if (write_cache(newfd, active_cache, active_nr) || commit_locked_index(lock_file)) die(_("unable to write new index file")); diff --git a/cache-tree.c b/cache-tree.c index 7fa524a..28d2356 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -612,9 +612,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix) cache_tree_find(active_cache_tree, prefix); if (!subtree) return WRITE_TREE_PREFIX_ERROR; - hashcpy(sha1, subtree->sha1); + if (sha1) + hashcpy(sha1, subtree->sha1); } - else + else if (sha1) hashcpy(sha1, active_cache_tree->sha1); if (0 <= newfd) diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh index 6c33e28..98fb1ab 100755 --- a/t/t0090-cache-tree.sh +++ b/t/t0090-cache-tree.sh @@ -44,14 +44,14 @@ test_expect_success 'read-tree HEAD establishes cache-tree' ' test_expect_success 'git-add invalidates cache-tree' ' test_when_finished "git reset --hard; git read-tree HEAD" && - echo "I changed this file" > foo && + echo "I changed this file" >foo && git add foo && test_invalid_cache_tree ' test_expect_success 'update-index invalidates cache-tree' ' test_when_finished "git reset --hard; git read-tree HEAD" && - echo "I changed this file" > foo && + echo "I changed this file" >foo && git update-index --add foo && test_invalid_cache_tree ' @@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' ' test_shallow_cache_tree ' -test_expect_failure 'checkout gives cache-tree' ' +test_expect_success 'checkout gives cache-tree' ' + git tag current && git checkout HEAD^ && test_shallow_cache_tree ' +test_expect_success 'checkout -b gives cache-tree' ' + git checkout current && + git checkout -b prev HEAD^ && + test_shallow_cache_tree +' + +test_expect_success 'checkout -B gives cache-tree' ' + git checkout current && + git checkout -B prev HEAD^ && + test_shallow_cache_tree +' + test_done -- 2.0.0.390.gcb682f8 ``` ## David Turner, 2014-07-01 19:14 Subject: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code Message-ID: <1404242075-7068-2-git-send-email-dturner@twitter.com> URL: https://gitlist.dev/e/1404242075-7068-2-git-send-email-dturner%40twitter.com In-Reply-To: <1404242075-7068-1-git-send-email-dturner@twitter.com> ``` Make test-dump-cache-tree more useful for testing. Do not treat known invalid trees as errors (and do not produce non-zero exit code), because we can fall back to the non-cache-tree codepath. Signed-off-by: David Turner --- t/t0090-cache-tree.sh | 28 +++++++++++++++++++++++++--- test-dump-cache-tree.c | 24 ++++++++++++------------ 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh index 98fb1ab..8437c5f 100755 --- a/t/t0090-cache-tree.sh +++ b/t/t0090-cache-tree.sh @@ -21,10 +21,13 @@ test_shallow_cache_tree () { cmp_cache_tree expect } +# Test that the cache-tree for a given directory is invalid. +# If no directory is given, check that the root is invalid test_invalid_cache_tree () { - echo "invalid (0 subtrees)" >expect && - printf "SHA #(ref) (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >>expect && - cmp_cache_tree expect + test-dump-cache-tree >actual && + sed -e "s/$_x40/SHA/" -e "s/[0-9]* subtrees//g" filtered && + expect=$(printf "invalid $1 ()\n") && + fgrep "$expect" filtered } test_no_cache_tree () { @@ -49,6 +52,25 @@ test_expect_success 'git-add invalidates cache-tree' ' test_invalid_cache_tree ' +test_expect_success 'git-add in subdir invalidates cache-tree' ' + test_when_finished "git reset --hard; git read-tree HEAD" && + mkdir dirx && + echo "I changed this file" >dirx/foo && + git add dirx/foo && + test_invalid_cache_tree +' + +test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' ' + git tag no-children && + test_when_finished "git reset --hard no-children; git read-tree HEAD" && + mkdir dir1 dir2 && + test_commit dir1/a && + test_commit dir2/b && + echo "I changed this file" >dir1/a && + git add dir1/a && + test_invalid_cache_tree dir1/ +' + test_expect_success 'update-index invalidates cache-tree' ' test_when_finished "git reset --hard; git read-tree HEAD" && echo "I changed this file" >foo && diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c index 47eab97..ad42ca1 100644 --- a/test-dump-cache-tree.c +++ b/test-dump-cache-tree.c @@ -6,12 +6,12 @@ static void dump_one(struct cache_tree *it, const char *pfx, const char *x) { if (it->entry_count < 0) - printf("%-40s %s%s (%d subtrees)\n", - "invalid", x, pfx, it->subtree_nr); + printf("%-40s %s (%d subtrees)%s\n", + "invalid", pfx, it->subtree_nr, x); else - printf("%s %s%s (%d entries, %d subtrees)\n", - sha1_to_hex(it->sha1), x, pfx, - it->entry_count, it->subtree_nr); + printf("%s %s (%d entries, %d subtrees)%s\n", + sha1_to_hex(it->sha1), pfx, + it->entry_count, it->subtree_nr, x); } static int dump_cache_tree(struct cache_tree *it, @@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it, /* missing in either */ return 0; - if (it->entry_count < 0) { + if (it->entry_count < 0) + /* invalid */ dump_one(it, pfx, ""); - dump_one(ref, pfx, "#(ref) "); - if (it->subtree_nr != ref->subtree_nr) - errs = 1; - } else { - dump_one(it, pfx, ""); if (hashcmp(it->sha1, ref->sha1) || ref->entry_count != it->entry_count || ref->subtree_nr != it->subtree_nr) { - dump_one(ref, pfx, "#(ref) "); + /* claims to be valid but is lying */ + dump_one(ref, pfx, " #(error)"); errs = 1; + } else { + /* is actually valid */ + dump_one(it, pfx, ""); } } -- 2.0.0.390.gcb682f8 ``` ## David Turner, 2014-07-01 19:14 Subject: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit Message-ID: <1404242075-7068-3-git-send-email-dturner@twitter.com> URL: https://gitlist.dev/e/1404242075-7068-3-git-send-email-dturner%40twitter.com In-Reply-To: <1404242075-7068-1-git-send-email-dturner@twitter.com> ``` During the commit process, the cache-tree is updated. We need to write this updated cache-tree so that it's ready for subsequent commands. Add test code which demonstrates that git commit now writes the cache tree. Also demonstrate that cache-tree invalidation is correct. Signed-off-by: David Turner --- builtin/commit.c | 15 ++++++------ t/t0090-cache-tree.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 9cfef6c..dbd4f4b 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, discard_cache(); read_cache_from(index_lock.filename); + if (update_main_cache_tree(WRITE_TREE_SILENT) >= 0) + write_cache(fd, active_cache, active_nr); commit_style = COMMIT_NORMAL; return index_lock.filename; @@ -383,14 +385,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, if (!only && !pathspec.nr) { fd = hold_locked_index(&index_lock, 1); refresh_cache_or_die(refresh_flags); - if (active_cache_changed) { - update_main_cache_tree(WRITE_TREE_SILENT); - if (write_cache(fd, active_cache, active_nr) || - commit_locked_index(&index_lock)) - die(_("unable to write new_index file")); - } else { - rollback_lock_file(&index_lock); - } + update_main_cache_tree(WRITE_TREE_SILENT); + if (write_cache(fd, active_cache, active_nr) || + commit_locked_index(&index_lock)) + die(_("unable to write new_index file")); commit_style = COMMIT_AS_IS; return get_index_file(); } @@ -435,6 +433,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, fd = hold_locked_index(&index_lock, 1); add_remove_files(&partial); refresh_cache(REFRESH_QUIET); + update_main_cache_tree(WRITE_TREE_SILENT); if (write_cache(fd, active_cache, active_nr) || close_lock_file(&index_lock)) die(_("unable to write new_index file")); diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh index 8437c5f..625157e 100755 --- a/t/t0090-cache-tree.sh +++ b/t/t0090-cache-tree.sh @@ -13,11 +13,35 @@ cmp_cache_tree () { test_cmp "$1" filtered } +grep_nonmatch_ok () { + grep $@ + test "$?" = "2" && return 1 + return 0 +} + # We don't bother with actually checking the SHA1: # test-dump-cache-tree already verifies that all existing data is # correct. +generate_expected_cache_tree () { + dir="$1${1:+/}" && + parent="$2" && + # ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux + # We want to count only foo because it's the only direct child + subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) && + subtree_count=$(echo "$subtrees"|grep_nonmatch_ok -c .) && + entries=$(git ls-files|wc -l) && + printf "SHA $dir (%d entries, %d subtrees)\n" $entries $subtree_count && + for subtree in $subtrees + do + cd "$subtree" + generate_expected_cache_tree "$dir$subtree" $dir || return 1 + cd .. + done && + dir=$parent +} + test_shallow_cache_tree () { - printf "SHA (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect && + generate_expected_cache_tree >expect && cmp_cache_tree expect } @@ -35,7 +59,7 @@ test_no_cache_tree () { cmp_cache_tree expect } -test_expect_failure 'initial commit has cache-tree' ' +test_expect_success 'initial commit has cache-tree' ' test_commit foo && test_shallow_cache_tree ' @@ -60,6 +84,18 @@ test_expect_success 'git-add in subdir invalidates cache-tree' ' test_invalid_cache_tree ' +cat >before <<\EOF +SHA (3 entries, 2 subtrees) +SHA dir1/ (1 entries, 0 subtrees) +SHA dir2/ (1 entries, 0 subtrees) +EOF + +cat >expect <<\EOF +invalid (2 subtrees) +invalid dir1/ (0 subtrees) +SHA dir2/ (1 entries, 0 subtrees) +EOF + test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' ' git tag no-children && test_when_finished "git reset --hard no-children; git read-tree HEAD" && @@ -67,8 +103,10 @@ test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' ' test_commit dir1/a && test_commit dir2/b && echo "I changed this file" >dir1/a && + cmp_cache_tree before && + echo "I changed this file" >dir1/a && git add dir1/a && - test_invalid_cache_tree dir1/ + cmp_cache_tree expect ' test_expect_success 'update-index invalidates cache-tree' ' @@ -95,6 +133,14 @@ test_expect_success 'second commit has cache-tree' ' test_shallow_cache_tree ' +test_expect_success 'commit in child dir has cache-tree' ' + mkdir dir && + >dir/child.t && + git add dir/child.t && + git commit -m dir/child.t && + test_shallow_cache_tree +' + test_expect_success 'reset --hard gives cache-tree' ' test-scrap-cache-tree && git reset --hard && @@ -125,4 +171,15 @@ test_expect_success 'checkout -B gives cache-tree' ' test_shallow_cache_tree ' +test_expect_success 'partial commit gives cache-tree' ' + git checkout -b partial no-children && + test_commit one && + test_commit two && + echo "some change" >one.t && + git add one.t && + echo "some other change" >two.t && + git commit two.t -m partial && + test_shallow_cache_tree +' + test_done -- 2.0.0.390.gcb682f8 ``` ## Junio C Hamano, 2014-07-01 21:08 Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout Message-ID: URL: https://gitlist.dev/e/xmqqzjgsvlb3.fsf%40gitster.dls.corp.google.com In-Reply-To: <1404242075-7068-1-git-send-email-dturner@twitter.com> ``` David Turner writes: > When git checkout checks out a branch, create or update the > cache-tree so that subsequent operations are faster. > > Signed-off-by: David Turner > --- Could you number your patches e.g. [PATCH v4 1/3] and/or summarize below the three-dash line what got updated since the last round? The readers can guess without when one is sending a reroll every other day or less frequently, but with rerolls more often than that, it gets unwieldy to check which points raised during the review have been addressed. Thanks. > builtin/checkout.c | 8 ++++++++ > cache-tree.c | 5 +++-- > t/t0090-cache-tree.sh | 19 ++++++++++++++++--- > 3 files changed, 27 insertions(+), 5 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 07cf555..a023a86 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts *opts, > } > } > > + if (!active_cache_tree) > + active_cache_tree = cache_tree(); > + > + if (!cache_tree_fully_valid(active_cache_tree)) > + cache_tree_update(active_cache_tree, > + (const struct cache_entry * const *)active_cache, > + active_nr, 0); > + > if (write_cache(newfd, active_cache, active_nr) || > commit_locked_index(lock_file)) > die(_("unable to write new index file")); > diff --git a/cache-tree.c b/cache-tree.c > index 7fa524a..28d2356 100644 > --- a/cache-tree.c > +++ b/cache-tree.c > @@ -612,9 +612,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix) > cache_tree_find(active_cache_tree, prefix); > if (!subtree) > return WRITE_TREE_PREFIX_ERROR; > - hashcpy(sha1, subtree->sha1); > + if (sha1) > + hashcpy(sha1, subtree->sha1); > } > - else > + else if (sha1) > hashcpy(sha1, active_cache_tree->sha1); > > if (0 <= newfd) > diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh > index 6c33e28..98fb1ab 100755 > --- a/t/t0090-cache-tree.sh > +++ b/t/t0090-cache-tree.sh > @@ -44,14 +44,14 @@ test_expect_success 'read-tree HEAD establishes cache-tree' ' > > test_expect_success 'git-add invalidates cache-tree' ' > test_when_finished "git reset --hard; git read-tree HEAD" && > - echo "I changed this file" > foo && > + echo "I changed this file" >foo && > git add foo && > test_invalid_cache_tree > ' > > test_expect_success 'update-index invalidates cache-tree' ' > test_when_finished "git reset --hard; git read-tree HEAD" && > - echo "I changed this file" > foo && > + echo "I changed this file" >foo && > git update-index --add foo && > test_invalid_cache_tree > ' > @@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' ' > test_shallow_cache_tree > ' > > -test_expect_failure 'checkout gives cache-tree' ' > +test_expect_success 'checkout gives cache-tree' ' > + git tag current && > git checkout HEAD^ && > test_shallow_cache_tree > ' > > +test_expect_success 'checkout -b gives cache-tree' ' > + git checkout current && > + git checkout -b prev HEAD^ && > + test_shallow_cache_tree > +' > + > +test_expect_success 'checkout -B gives cache-tree' ' > + git checkout current && > + git checkout -B prev HEAD^ && > + test_shallow_cache_tree > +' > + > test_done ``` ## Junio C Hamano, 2014-07-01 21:42 Subject: Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code Message-ID: URL: https://gitlist.dev/e/xmqqpphovjqk.fsf%40gitster.dls.corp.google.com In-Reply-To: <1404242075-7068-2-git-send-email-dturner@twitter.com> ``` David Turner writes: > Make test-dump-cache-tree more useful for testing. Do not treat known > invalid trees as errors (and do not produce non-zero exit code), > because we can fall back to the non-cache-tree codepath. Under-explained. "more useful" is subjective so I won't complain about it being not explained enough, but I cannot quite parse and understand the second sentence. It is not "we treat known invalid trees as errors". I think what you meant is "we insist that a cache-tree entry, even if it is an invalidated one, must record the correct number of subtrees and signal errors otherwise, which is wrong". I honestly cannot guess what you meant to say by "because we can ...". > diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c > index 47eab97..ad42ca1 100644 > --- a/test-dump-cache-tree.c > +++ b/test-dump-cache-tree.c > @@ -6,12 +6,12 @@ > static void dump_one(struct cache_tree *it, const char *pfx, const char *x) > { > if (it->entry_count < 0) > - printf("%-40s %s%s (%d subtrees)\n", > - "invalid", x, pfx, it->subtree_nr); > + printf("%-40s %s (%d subtrees)%s\n", > + "invalid", pfx, it->subtree_nr, x); > else > - printf("%s %s%s (%d entries, %d subtrees)\n", > - sha1_to_hex(it->sha1), x, pfx, > - it->entry_count, it->subtree_nr); > + printf("%s %s (%d entries, %d subtrees)%s\n", > + sha1_to_hex(it->sha1), pfx, > + it->entry_count, it->subtree_nr, x); I am guessing this is related to the "more useful", but I cannot offhand tell what this output shuffling is about. It would be better to illustrate in the log message to support the "more useful" claim by showing how improved/readable the output got with this change. > } > > static int dump_cache_tree(struct cache_tree *it, > @@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it, > /* missing in either */ > return 0; > > - if (it->entry_count < 0) { > + if (it->entry_count < 0) > + /* invalid */ > dump_one(it, pfx, ""); > - dump_one(ref, pfx, "#(ref) "); Unfortunately this is not quite what we would want; this "#(ref)" output is so that we can see what tree object we should be referring to, while debugging, if this entry were not invalidated; losing that does not "Improve output"---it loses information from the output. > - if (it->subtree_nr != ref->subtree_nr) > - errs = 1; I am guessing that this is the change you did not explain quite enough with "do not treat ... as errors". This code expects that even an invalidated cache-tree entry knows how many subtrees it has, which is unreasonable. I do not recall offhand if we used to have some code to ensure that such an invariant holds or not, but when invalidating a directory (say "t/") by adding a new subdirectory and a new file in it (e.g. "t/dir/file") to the index, we do not do anything other than to invalidate "t/" and "t/dir/", and I do not think the codepath recounts the number of subdirectories to adjust subtree_nr in any way to match the reality, so removal of this check is the right thing to do. > - } > else { > - dump_one(it, pfx, ""); > if (hashcmp(it->sha1, ref->sha1) || > ref->entry_count != it->entry_count || > ref->subtree_nr != it->subtree_nr) { > - dump_one(ref, pfx, "#(ref) "); > + /* claims to be valid but is lying */ > + dump_one(ref, pfx, " #(error)"); > errs = 1; > + } else { > + /* is actually valid */ > + dump_one(it, pfx, ""); > } > } ``` ## Junio C Hamano, 2014-07-01 22:45 Subject: Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit Message-ID: URL: https://gitlist.dev/e/xmqqlhscvgts.fsf%40gitster.dls.corp.google.com In-Reply-To: <1404242075-7068-3-git-send-email-dturner@twitter.com> ``` David Turner writes: > During the commit process, the cache-tree is updated. We need to write > this updated cache-tree so that it's ready for subsequent commands. > > Add test code which demonstrates that git commit now writes the cache > tree. Also demonstrate that cache-tree invalidation is correct. > > Signed-off-by: David Turner > --- > builtin/commit.c | 15 ++++++------ > t/t0090-cache-tree.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 67 insertions(+), 11 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 9cfef6c..dbd4f4b 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, > > discard_cache(); > read_cache_from(index_lock.filename); > + if (update_main_cache_tree(WRITE_TREE_SILENT) >= 0) > + write_cache(fd, active_cache, active_nr); OK, interactive-add may leave the cache-tree not quite populated; we are going to write out a tree from the cache so we need to update the in-core cache tree anyway, so calling update-main-cache-tree here would not hurt (it will speed up the write-cache-as-tree we will eventually call). We might want to see if we are really changing anything, though. What happens if the interactive-add gave us an index with fully valid cache-tree? Is that rare enough not to matter (not a rhetorical question)? > @@ -383,14 +385,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, > if (!only && !pathspec.nr) { > fd = hold_locked_index(&index_lock, 1); > refresh_cache_or_die(refresh_flags); > - if (active_cache_changed) { > - update_main_cache_tree(WRITE_TREE_SILENT); > - if (write_cache(fd, active_cache, active_nr) || > - commit_locked_index(&index_lock)) > - die(_("unable to write new_index file")); > - } else { > - rollback_lock_file(&index_lock); > - } > + update_main_cache_tree(WRITE_TREE_SILENT); > + if (write_cache(fd, active_cache, active_nr) || > + commit_locked_index(&index_lock)) > + die(_("unable to write new_index file")); How about doing this part like the following instead, so that we can avoid the overhead of uselessly rewriting the index file when we do not have to? diff --git a/builtin/commit.c b/builtin/commit.c index 5e2221c..7d730a5 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -383,8 +383,11 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, if (!only && !pathspec.nr) { fd = hold_locked_index(&index_lock, 1); refresh_cache_or_die(refresh_flags); - if (active_cache_changed) { + if (active_cache_changed || !cache_tree_fully_valid(active_cache_tree)) { update_main_cache_tree(WRITE_TREE_SILENT); + active_cache_changed = 1; + } + if (active_cache_changed) { if (write_cache(fd, active_cache, active_nr) || commit_locked_index(&index_lock)) die(_("unable to write new_index file")); It makes me wonder if we should teach update_main_cache_tree() to somehow smudge active_cache_changed bit as necessary. Then we do not have to make the call to update-main-cache-tree conditional. > @@ -435,6 +433,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, > fd = hold_locked_index(&index_lock, 1); > add_remove_files(&partial); > refresh_cache(REFRESH_QUIET); > + update_main_cache_tree(WRITE_TREE_SILENT); > if (write_cache(fd, active_cache, active_nr) || > close_lock_file(&index_lock)) > die(_("unable to write new_index file")); This is the index that will be used after we create the commit (which will be created from a temporary index that will be discarded immediately after we create the commit). As we _know_ we are changing something in this code path by calling add_remote_files(), it is fine to call update-main-cache-tree here unconditionally. I didn't notice it when I gave the previous review comment but while reviewing this round, we already do the cache-tree population for "commit -a" in this function, which suggests me that it is the right place to do these changes. Modulo minor niggles, I like this iteration much better than the previous one. Thanks for working on this. ``` ## David Turner, 2014-07-01 22:58 Subject: Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit Message-ID: <1404255484.3109.7.camel@stross> URL: https://gitlist.dev/e/1404255484.3109.7.camel%40stross In-Reply-To: ``` On Tue, 2014-07-01 at 15:45 -0700, Junio C Hamano wrote: > I didn't notice it when I gave the previous review comment but while > reviewing this round, we already do the cache-tree population for > "commit -a" in this function, which suggests me that it is the right > place to do these changes. Modulo minor niggles, I like this > iteration much better than the previous one. > > Thanks for working on this. Thanks for all your comments. I'll send out a v4 when I can, but I probably won't have more time to work on this until next week. ```