From 8cc21ce78c2f3781024117047c0650861f890213 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 20 Apr 2009 03:58:17 -0700 Subject: [PATCH 1/4] read-tree A B: do not corrupt cache-tree An earlier commit aab3b9a (read-tree A B C: do not create a bogus index and do not segfault, 2009-03-12) resurrected the support for an obscure (but useful) feature to read and overlay more than one tree into the index without the -m (merge) option. But the fix was not enough. Exercising this feature exposes a longstanding bug in the code that primes the cache-tree in the index from the tree that was read. The intention was that when we know that the index must exactly match the tree we just read, we prime the entire cache-tree with it. However, the logic to detect that case incorrectly triggered if you read two trees without -m. This resulted in a corrupted cache-tree, and write-tree would have produced an incorrect tree object out of such an index. Signed-off-by: Junio C Hamano --- builtin-read-tree.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/builtin-read-tree.c b/builtin-read-tree.c index 38fef34d3f..e4e0e710c8 100644 --- a/builtin-read-tree.c +++ b/builtin-read-tree.c @@ -211,7 +211,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) case 3: default: opts.fn = threeway_merge; - cache_tree_free(&active_cache_tree); break; } @@ -221,6 +220,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) opts.head_idx = 1; } + cache_tree_free(&active_cache_tree); for (i = 0; i < nr_trees; i++) { struct tree *tree = trees[i]; parse_tree(tree); @@ -235,10 +235,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) * valid cache-tree because the index must match exactly * what came from the tree. */ - if (nr_trees && !opts.prefix && (!opts.merge || (stage == 2))) { - cache_tree_free(&active_cache_tree); + if (nr_trees == 1 && !opts.prefix) prime_cache_tree(); - } if (write_cache(newfd, active_cache, active_nr) || commit_locked_index(&lock_file)) From b9d37a5420446d0db2dc0dc5458a5e50656a4852 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 20 Apr 2009 03:58:18 -0700 Subject: [PATCH 2/4] Move prime_cache_tree() to cache-tree.c The interface to build cache-tree belongs there. Signed-off-by: Junio C Hamano --- builtin-checkout.c | 1 + builtin-read-tree.c | 37 +------------------------------------ cache-tree.c | 34 ++++++++++++++++++++++++++++++++++ cache-tree.h | 4 ++++ 4 files changed, 40 insertions(+), 36 deletions(-) diff --git a/builtin-checkout.c b/builtin-checkout.c index b121fe56de..ffdb33aef5 100644 --- a/builtin-checkout.c +++ b/builtin-checkout.c @@ -5,6 +5,7 @@ #include "commit.h" #include "tree.h" #include "tree-walk.h" +#include "cache-tree.h" #include "unpack-trees.h" #include "dir.h" #include "run-command.h" diff --git a/builtin-read-tree.c b/builtin-read-tree.c index e4e0e710c8..9cd7d0738e 100644 --- a/builtin-read-tree.c +++ b/builtin-read-tree.c @@ -29,41 +29,6 @@ static int list_tree(unsigned char *sha1) return 0; } -static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree) -{ - struct tree_desc desc; - struct name_entry entry; - int cnt; - - hashcpy(it->sha1, tree->object.sha1); - init_tree_desc(&desc, tree->buffer, tree->size); - cnt = 0; - while (tree_entry(&desc, &entry)) { - if (!S_ISDIR(entry.mode)) - cnt++; - else { - struct cache_tree_sub *sub; - struct tree *subtree = lookup_tree(entry.sha1); - if (!subtree->object.parsed) - parse_tree(subtree); - sub = cache_tree_sub(it, entry.path); - sub->cache_tree = cache_tree(); - prime_cache_tree_rec(sub->cache_tree, subtree); - cnt += sub->cache_tree->entry_count; - } - } - it->entry_count = cnt; -} - -static void prime_cache_tree(void) -{ - if (!nr_trees) - return; - active_cache_tree = cache_tree(); - prime_cache_tree_rec(active_cache_tree, trees[0]); - -} - static const char read_tree_usage[] = "git read-tree ( | [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u | -i]] [--exclude-per-directory=] [--index-output=] [ []])"; static struct lock_file lock_file; @@ -236,7 +201,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) * what came from the tree. */ if (nr_trees == 1 && !opts.prefix) - prime_cache_tree(); + prime_cache_tree(&active_cache_tree, trees[0]); if (write_cache(newfd, active_cache, active_nr) || commit_locked_index(&lock_file)) diff --git a/cache-tree.c b/cache-tree.c index 3d8f218a5f..37bf35e636 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -1,5 +1,6 @@ #include "cache.h" #include "tree.h" +#include "tree-walk.h" #include "cache-tree.h" #ifndef DEBUG @@ -591,3 +592,36 @@ int write_cache_as_tree(unsigned char *sha1, int missing_ok, const char *prefix) return 0; } + +static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree) +{ + struct tree_desc desc; + struct name_entry entry; + int cnt; + + hashcpy(it->sha1, tree->object.sha1); + init_tree_desc(&desc, tree->buffer, tree->size); + cnt = 0; + while (tree_entry(&desc, &entry)) { + if (!S_ISDIR(entry.mode)) + cnt++; + else { + struct cache_tree_sub *sub; + struct tree *subtree = lookup_tree(entry.sha1); + if (!subtree->object.parsed) + parse_tree(subtree); + sub = cache_tree_sub(it, entry.path); + sub->cache_tree = cache_tree(); + prime_cache_tree_rec(sub->cache_tree, subtree); + cnt += sub->cache_tree->entry_count; + } + } + it->entry_count = cnt; +} + +void prime_cache_tree(struct cache_tree **it, struct tree *tree) +{ + cache_tree_free(it); + *it = cache_tree(); + prime_cache_tree_rec(*it, tree); +} diff --git a/cache-tree.h b/cache-tree.h index cf8b790874..e958835236 100644 --- a/cache-tree.h +++ b/cache-tree.h @@ -1,6 +1,8 @@ #ifndef CACHE_TREE_H #define CACHE_TREE_H +#include "tree.h" + struct cache_tree; struct cache_tree_sub { struct cache_tree *cache_tree; @@ -33,4 +35,6 @@ int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int, int) #define WRITE_TREE_PREFIX_ERROR (-3) int write_cache_as_tree(unsigned char *sha1, int missing_ok, const char *prefix); +void prime_cache_tree(struct cache_tree **, struct tree *); + #endif From 456156dc068b7664c08e35157e17a6440ec68f32 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 20 Apr 2009 03:58:19 -0700 Subject: [PATCH 3/4] read-tree -m A B: prime cache-tree from the switched-to tree When switching to a new branch with "read-tree -m A B", the resulting index must match tree B and we can prime the cache tree with it. Signed-off-by: Junio C Hamano --- builtin-read-tree.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/builtin-read-tree.c b/builtin-read-tree.c index 9cd7d0738e..391d709704 100644 --- a/builtin-read-tree.c +++ b/builtin-read-tree.c @@ -199,9 +199,14 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) * "-m ent" or "--reset ent" form), we can obtain a fully * valid cache-tree because the index must match exactly * what came from the tree. + * + * The same holds true if we are switching between two trees + * using read-tree -m A B. The index must match B after that. */ if (nr_trees == 1 && !opts.prefix) prime_cache_tree(&active_cache_tree, trees[0]); + else if (nr_trees == 2 && opts.merge) + prime_cache_tree(&active_cache_tree, trees[1]); if (write_cache(newfd, active_cache, active_nr) || commit_locked_index(&lock_file)) From 83ae209bf9708bf1b67dbac4a3629a0003af5dbb Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 20 Apr 2009 03:58:20 -0700 Subject: [PATCH 4/4] checkout branch: prime cache-tree fully When switching to another branch, the earlier code relied on incremental invalidation of cache-tree entries to degrade it. While it is not wrong per-se, we know that the resulting index must fully match the branch we are switching to unless the -m (merge) option is used. We should simply fully re-prime the cache-tree using the new tree object in such a case. And for safety, invalidate the cache-tree as a whole in other cases. Signed-off-by: Junio C Hamano --- builtin-checkout.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/builtin-checkout.c b/builtin-checkout.c index ffdb33aef5..efa1ebfe07 100644 --- a/builtin-checkout.c +++ b/builtin-checkout.c @@ -368,14 +368,17 @@ static int merge_working_tree(struct checkout_opts *opts, int ret; struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file)); int newfd = hold_locked_index(lock_file, 1); + int reprime_cache_tree = 0; if (read_cache() < 0) return error("corrupt index file"); + cache_tree_free(&active_cache_tree); if (opts->force) { ret = reset_tree(new->commit->tree, opts, 1); if (ret) return ret; + reprime_cache_tree = 1; } else { struct tree_desc trees[2]; struct tree *tree; @@ -411,7 +414,9 @@ static int merge_working_tree(struct checkout_opts *opts, init_tree_desc(&trees[1], tree->buffer, tree->size); ret = unpack_trees(2, trees, &topts); - if (ret == -1) { + if (ret != -1) { + reprime_cache_tree = 1; + } else { /* * Unpack couldn't do a trivial merge; either * give up or do a real merge, depending on @@ -455,6 +460,8 @@ static int merge_working_tree(struct checkout_opts *opts, } } + if (reprime_cache_tree) + prime_cache_tree(&active_cache_tree, new->commit->tree); if (write_cache(newfd, active_cache, active_nr) || commit_locked_index(lock_file)) die("unable to write new index file");