On Fri, Jul 11, 2014 at 12:59:48AM +0100, Ramsay Jones wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html