Re: [PATCH] cache-tree: do not cache empty trees

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Feb 7, 2011, 09:57 UTC
Message-ID: <20110207095713.GA19653@do>
In-Reply-To: <20110207091740.GA5391@elie>

On Mon, Feb 07, 2011 at 03:17:40AM -0600, Jonathan Nieder wrote:

Show 21 quoted lines

> While this violates some seeming invariants, like
> 
> 1.
> 	git reset --hard
> 	git commit --allow-empty
> 	git rev-parse HEAD^^{tree} >expect
> 	git rev-parse HEAD^{tree} >actual
> 	test_cmp expect actual
> 
> 2.
> 	git reset --hard
> 	git revert HEAD
> 	if git rev-parse HEAD~2
> 	then
> 		git rev-parse HEAD~2^{tree} >expect
> 		git rev-parse HEAD^{tree} >actual
> 		test_cmp expect actual
> 	fi
> 
> , I think it's a good change.  Malformed modes in trees already break
> those false invariants iiuc.

Perhaps it's not a good approach after all. What I wanted was to make pre-1.8.0 tolerate empty trees created by 1.8.0. Perhaps it's better to just let pre-1.8.0 refuse to work with empty trees, forcing users to upgrade to 1.8.0?

The (untested) patch below would make git refuse to create an index from a tree that contains empty trees. Hmm?

diff --git a/cache-tree.c b/cache-tree.c
index f755590..e33998a 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -619,6 +619,8 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)
 		else {
 			struct cache_tree_sub *sub;
 			struct tree *subtree = lookup_tree(entry.sha1);
+			if (!hashcmp(entry.sha1, EMPTY_TREE_SHA1_BIN))
+				die("empty tree .../%s detected!", entry.path);
 			if (!subtree->object.parsed)
 				parse_tree(subtree);
 			sub = cache_tree_sub(it, entry.path);
diff --git a/unpack-trees.c b/unpack-trees.c
index 1ca41b1..0e6738e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -434,6 +434,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long
 	void *buf[MAX_UNPACK_TREES];
 	struct traverse_info newinfo;
 	struct name_entry *p;
+	struct unpack_trees_options *o = info->data;
 
 	p = names;
 	while (!p->mode)
@@ -447,8 +448,11 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long
 
 	for (i = 0; i < n; i++, dirmask >>= 1) {
 		const unsigned char *sha1 = NULL;
-		if (dirmask & 1)
+		if (dirmask & 1) {
 			sha1 = names[i].sha1;
+			if (o->merge && !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
+				return error("empty tree .../%s detected!", p->path);
+		}
 		buf[i] = fill_tree_descriptor(t+i, sha1);
 	}

-- 
Duy