From: Nguyen Thai Ngoc Duy Date: Mon, 07 Feb 2011 02:36:37 GMT Subject: Re: [PATCH] cache-tree: do not cache empty trees Message-ID: In-Reply-To: <7v62swwq7s.fsf@alter.siamese.dyndns.org> 2011/2/7 Junio C Hamano : >> diff --git a/cache-tree.c b/cache-tree.c >> index f755590..03732ad 100644 >> --- a/cache-tree.c >> +++ b/cache-tree.c >> @@ -621,9 +621,18 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree) >>                       struct tree *subtree = lookup_tree(entry.sha1); >>                       if (!subtree->object.parsed) >>                               parse_tree(subtree); >> +                     if (!hashcmp(entry.sha1, (unsigned char *)EMPTY_TREE_SHA1_BIN)) { >> +                             warning("empty tree detected! Will be removed in new commits"); >> +                             cnt = -1; >> +                             break; >> +                     } > > You shouldn't need the cast (if you did, then hashcmp() macro should be > fixed so that you don't need to). Or perhaps EMPTY_TREE_SHA1_BIN should be casted to const unsigned char *. That would eliminate 4 typecastings elsewhere. > I don't think warning() is warranted for an operation you introduced to > keep the internal data structure consistent. Worse. I don't think users know an empty tree is added or removed. diff-tree does not show it (or should not, I haven't tested). > Should this comparison done after we parsed the subtree, or should we be > doing that before it? > > If you are adding this new check to a point where we have already parsed > the subtree object, don't you have a better and cheaper way to detect if > the subtree is empty than the 20-byte comparision, namely, perhaps by > looking at subtree->size? OK before is better. -- Duy