Re: [PATCH v3 2/2] alloc.c: remove the redundant commit_count variable
- From
Jeff King <peff@peff.net>
- Date
- Jul 11, 2014, 00:30 UTC
- Message-ID
- <20140711003053.GB11360@sigill.intra.peff.net>
- In-Reply-To
- <53BF28F4.8050704@ramsay1.demon.co.uk>
On Fri, Jul 11, 2014 at 12:59:48AM +0100, Ramsay Jones wrote:
Show 5 quoted lines
> The 'commit_count' static variable is used in alloc_commit_node() > to set the 'index' field of the commit structure to a unique value. > This variable assumes the same value as the 'count' field of the > 'commit_state' allocator state structure, which may be used in its > place.
I don't think we want to do this, because there is a bug in the current code that I have not reported yet. :)
The code you're touching here was trying to make sure that each commit gets a unique index, under the assumption that commits only get allocated via alloc_commit_node. But I think that assumption is wrong. We can also get commit objects by allocating an OBJ_NONE (e.g., via lookup_unknown_object) and then converting it into an OBJ_COMMIT when we find out what it is.
We should be allocating a commit index to it at that point (but we don't currently), at which point the commit_count and commit_state.count indices will no longer be in sync.
This only happens in a few places. Most calls will probably be via lookup_commit (which gets called when we parse_object such an unknown object), but there are others (e.g., peel_object may fill in the type). So we could fix it with something like:
diff --git a/commit.c b/commit.c index fb7897c..f4f4278 100644 --- a/commit.c +++ b/commit.c @@ -65,8 +65,11 @@ struct commit *lookup_commit(const unsigned char *sha1) struct commit *c = alloc_commit_node(); return create_object(sha1, OBJ_COMMIT, c); } - if (!obj->type) + if (!obj->type) { + struct commit *c = (struct commit *)obj; obj->type = OBJ_COMMIT; + c->index = alloc_commit_index(); + } return check_commit(obj, sha1, 0); } where alloc_commit_index() would be a thin wrapper around "return commit_count++". And then find and update any other callsites that need it. The downside is that it's hard to find those callsites. A safer alternative would be to speculatively allocate a commit index for all unknown objects. Then if any other code switches the type to commit, they already have an index. But it also means we'll have holes in our commit index space, which makes commit-slabs less efficient. We do not call lookup_unknown_object all that often, though, so it might be an OK tradeoff (and in at least one case, in diff_tree_stdin, I think the code can be converted not to use lookup_unknown_object in the first place). So worrying about the performance may not be worth it. -Peff