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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 7, 2011, 02:09 UTC
Message-ID: <7v62swwq7s.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1296914835-808-1-git-send-email-pclouds@gmail.com>

Show 13 quoted lines

> 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).

I don't think warning() is warranted for an operation you introduced to keep the internal data structure consistent.

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?