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 <abhishekkumar8222@xxxxxxxxx> > --- > > 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 > > 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) >
