Re: [PATCH] cache-tree: do not cache empty trees
- From
Nguyen Thai Ngoc Duy <pclouds@gmail.com>
- Date
- Feb 7, 2011, 02:36 UTC
- Message-ID
- <AANLkTinCyOq4rmb-tf4B91bK97GWca-4DyC715tUv+zx@mail.gmail.com>
- In-Reply-To
- <7v62swwq7s.fsf@alter.siamese.dyndns.org>
2011/2/7 Junio C Hamano <gitster@pobox.com>:
Show 16 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).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).
Show 7 quoted lines
> 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