# [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code 7 messages from 2014-06-28 to 2014-06-30. Participants: David Turner, Duy Nguyen, Junio C Hamano. Thread: https://gitlist.dev/t/37014 ## David Turner, 2014-06-28 00:20 Subject: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout Message-ID: <1403914856-3546-1-git-send-email-dturner@twitter.com> URL: https://gitlist.dev/e/1403914856-3546-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 | 4 ++++ cache-tree.c | 22 ++++++++++++---------- cache-tree.h | 1 + t/t0090-cache-tree.sh | 15 ++++++++++++++- 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 07cf555..df791e8 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -553,6 +553,10 @@ static int merge_working_tree(const struct checkout_opts *opts, } } + if (write_cache_as_tree(NULL, WRITE_TREE_DO_NOT_WRITE, "")) { + warn("Unable to write cache_tree"); + } + 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..fb203c6 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -568,17 +568,18 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix) { - int entries, was_valid, newfd; + int entries, was_valid, newfd = -1; struct lock_file *lock_file; - /* - * We can't free this memory, it becomes part of a linked list - * parsed atexit() - */ - lock_file = xcalloc(1, sizeof(struct lock_file)); - - newfd = hold_locked_index(lock_file, 1); + if (!(flags & WRITE_TREE_DO_NOT_WRITE)) { + /* + * We can't free this memory, it becomes part of a linked list + * parsed atexit() + */ + lock_file = xcalloc(1, sizeof(struct lock_file)); + newfd = hold_locked_index(lock_file, 1); + } entries = read_cache(); if (entries < 0) return WRITE_TREE_UNREADABLE_INDEX; @@ -612,9 +613,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/cache-tree.h b/cache-tree.h index f1923ad..daf6640 100644 --- a/cache-tree.h +++ b/cache-tree.h @@ -39,6 +39,7 @@ int update_main_cache_tree(int); #define WRITE_TREE_IGNORE_CACHE_TREE 2 #define WRITE_TREE_DRY_RUN 4 #define WRITE_TREE_SILENT 8 +#define WRITE_TREE_DO_NOT_WRITE 16 /* error return codes */ #define WRITE_TREE_UNREADABLE_INDEX (-1) diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh index 6c33e28..7c60675 100755 --- a/t/t0090-cache-tree.sh +++ b/t/t0090-cache-tree.sh @@ -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-06-28 00:20 Subject: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code Message-ID: <1403914856-3546-2-git-send-email-dturner@twitter.com> URL: https://gitlist.dev/e/1403914856-3546-2-git-send-email-dturner%40twitter.com In-Reply-To: <1403914856-3546-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 7c60675..5383258 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-06-28 00:20 Subject: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit Message-ID: <1403914856-3546-3-git-send-email-dturner@twitter.com> URL: https://gitlist.dev/e/1403914856-3546-3-git-send-email-dturner%40twitter.com In-Reply-To: <1403914856-3546-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 | 6 ++++++ t/t0090-cache-tree.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 9cfef6c..6814e87 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -607,6 +607,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix, const char *hook_arg2 = NULL; int clean_message_contents = (cleanup_mode != CLEANUP_NONE); int old_display_comment_prefix; + static struct lock_file index_lock; + int index_fd; /* This checks and barfs if author is badly specified */ determine_author_info(author_ident); @@ -872,6 +874,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix, error(_("Error building trees")); return 0; } + /* After updating the cache-tree, rewrite the index */ + index_fd = hold_locked_index(&index_lock, 0); + if (index_fd >= 0 && write_index(&the_index, index_fd) >= 0) + commit_locked_index(&index_lock); if (run_commit_hook(use_editor, index_file, "prepare-commit-msg", git_path(commit_editmsg), hook_arg1, hook_arg2, NULL)) diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh index 5383258..ef012b9 100755 --- a/t/t0090-cache-tree.sh +++ b/t/t0090-cache-tree.sh @@ -16,8 +16,31 @@ cmp_cache_tree () { # 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 () { + if [ -n "$1" ] + then + local dir="$1/" + else + local dir="$1" + fi + # 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 + local subtrees=$(git ls-files|egrep '/' |cut -d / -f 1|uniq) && + local subtree_count=$(echo "$subtrees"|grep -c .) && + local files_count=$(git ls-files|grep -v /|wc -l) && + local entries=$(expr "$subtree_count" + "$files_count") && + printf "SHA $dir (%d entries, %d subtrees)\n" $entries $subtree_count && + local subtree && + for subtree in $subtrees + do + cd "$subtree" + generate_expected_cache_tree "$dir$subtree" || return 1 + cd .. + done +} + 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 +58,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,15 +83,28 @@ 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" && mkdir dir1 dir2 && test_commit dir1/a && test_commit dir2/b && + 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 +131,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 && -- 2.0.0.390.gcb682f8 ``` ## Duy Nguyen, 2014-06-29 03:33 Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout Message-ID: URL: https://gitlist.dev/e/CACsJy8D8687QULRBZF_hvwASxOy5ekuPJ997xLSDX6HXDXgZnA%40mail.gmail.com In-Reply-To: <1403914856-3546-1-git-send-email-dturner@twitter.com> ``` On Sat, Jun 28, 2014 at 7:20 AM, David Turner wrote: > 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 | 4 ++++ > cache-tree.c | 22 ++++++++++++---------- > cache-tree.h | 1 + > t/t0090-cache-tree.sh | 15 ++++++++++++++- > 4 files changed, 31 insertions(+), 11 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 07cf555..df791e8 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -553,6 +553,10 @@ static int merge_working_tree(const struct checkout_opts *opts, > } > } > > + if (write_cache_as_tree(NULL, WRITE_TREE_DO_NOT_WRITE, "")) { > + warn("Unable to write cache_tree"); > + } > + I wonder if we should do this in !opts->force code path only. In the opts->force code path we could use prime_cache_tree() (like read-tree), which is supposedly faster (but may need some tests to be sure). prime_cache_tree() could be made a bit faster by doing it during tree traversal in unpack_trees() so we don't have to unpack any tree objects twice. -- Duy ``` ## Junio C Hamano, 2014-06-30 06:31 Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout Message-ID: URL: https://gitlist.dev/e/xmqqvbrizz63.fsf%40gitster.dls.corp.google.com In-Reply-To: <1403914856-3546-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 > --- > builtin/checkout.c | 4 ++++ > cache-tree.c | 22 ++++++++++++---------- > cache-tree.h | 1 + > t/t0090-cache-tree.sh | 15 ++++++++++++++- > 4 files changed, 31 insertions(+), 11 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 07cf555..df791e8 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -553,6 +553,10 @@ static int merge_working_tree(const struct checkout_opts *opts, > } > } > > + if (write_cache_as_tree(NULL, WRITE_TREE_DO_NOT_WRITE, "")) { > + warn("Unable to write cache_tree"); > + } > + This is not even warn-worthy event, isn't it? "checkout -m other" would attempt a three-way merge to replay your local changes relative to your current HEAD in the context of "other" branch and will leave conflicts in the index and in the working tree, and it is perfectly a normal thing that you cannot write the index in such a state as a tree object. Perhaps you should check if the index is unmerged before even attempting to compute the cache tree. Also it looks very strange that write-cache-as-tree, whose *PRIMARY* function is to write a tree, receives a "DO NOT WRITE" option here. I understand that you want a new interface into the cache-tree subsystem that only updates the cache-tree but it somehow smells like a sloppy/lazy refactoring that is not done quite right. ``` ## Junio C Hamano, 2014-06-30 18:10 Subject: Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit Message-ID: URL: https://gitlist.dev/e/xmqqegy6z2t0.fsf%40gitster.dls.corp.google.com In-Reply-To: <1403914856-3546-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 | 6 ++++++ > t/t0090-cache-tree.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 53 insertions(+), 3 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 9cfef6c..6814e87 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -607,6 +607,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > const char *hook_arg2 = NULL; > int clean_message_contents = (cleanup_mode != CLEANUP_NONE); > int old_display_comment_prefix; > + static struct lock_file index_lock; > + int index_fd; > > /* This checks and barfs if author is badly specified */ > determine_author_info(author_ident); > @@ -872,6 +874,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > error(_("Error building trees")); > return 0; > } > + /* After updating the cache-tree, rewrite the index */ > + index_fd = hold_locked_index(&index_lock, 0); > + if (index_fd >= 0 && write_index(&the_index, index_fd) >= 0) > + commit_locked_index(&index_lock); Is this run unconditionally even when we are making a partial commit out of a temporary index file (which will be discarded after we create this commit)? I have a feeling that a better place to populate the cache-tree may be prepare-index which knows the distinction between various modes of commit, but I haven't looked at the code path for a while... ``` ## Junio C Hamano, 2014-06-30 18:13 Subject: Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout Message-ID: URL: https://gitlist.dev/e/xmqqa98uz2nx.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Duy Nguyen writes: > I wonder if we should do this in !opts->force code path only. In the > opts->force code path we could use prime_cache_tree() (like > read-tree), which is supposedly faster... Nobody sane should be constantly running "checkout -f", so even if priming from existing tree objects were faster, it would be adding complexity to the code to optimize for a wrong case. ```