Re: [PATCH v3 1/4] alloc: introduce parsed_commits_count
From: Jakub Narębski <hidden>
Date: 2020-06-12 23:16:58
On 12.06.2020, Abhishek Kumar wrote:
Commit slab relies on uniqueness of commit->index to access data. As submodules are repositories on their own, alloc_commit_index() (which depends on repository->parsed_objects->commit_count) no longer returns unique values. This would break tests once we move `generation` and `graph_pos` into a commit slab, as commits of supermodule and submodule can have the same index but must have different graph positions.
First, commits of supermodule and of submodule are in different graphs, so I don't see why they have to be in the same serialized commit-graph file. Second, Git stores many different types of information on slab already. How comes that we have not had any problems till now? There is contains_cache, commit_seen, indegree_slab, author_date_slab, commit_base, commit_pos, bloom_filter_slab, buffer_slab, commit_rev_name, commit_names, commit_name_slab, saved_parents, blame_suspects, commit_todo_item.
Let's introduce a counter variable, `parsed_commits_count` to keep track of parsed commits so far.
All right, thought it might be worth mentioning that it is a global variable, or rather a static variable in a function.
Signed-off-by: Abhishek Kumar <redacted> --- CI Build for the failing tests: https://travis-ci.com/github/abhishekkumar2718/git/jobs/345413840
The failed tests are, from what I see: - t4060-diff-submodule-option-diff-format.sh - t4041-diff-submodule-option.sh - t4059-diff-submodule-not-initialized.sh
quoted hunk ↗ jump to hunk
alloc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)diff --git a/alloc.c b/alloc.c index 1c64c4dd16..29f0e3aa80 100644 --- a/alloc.c +++ b/alloc.c@@ -101,7 +101,9 @@ void *alloc_object_node(struct repository *r) static unsigned int alloc_commit_index(struct repository *r) { - return r->parsed_objects->commit_count++; + static unsigned int parsed_commits_count = 0; + r->parsed_objects->commit_count++;
Do we use r->parsed_objects->commit_count anywhere?
+ return parsed_commits_count++;
Does it matter that it is not thread safe, because it is not atomic? Shouldn't it be + static _Atomic unsigned int parsed_commits_count = 0; or + static _Atomic unsigned int parsed_commits_count = ATOMIC_VAR_INIT(0); (If it is allowed).
} void init_commit_node(struct repository *r, struct commit *c)