Re: [GSOC Patch v2 2/4] commit: move members graph_pos, generation to a slab
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jun 8, 2020, 12:35 UTC
- Message-ID
- <c9333a2d-a0d7-0fe4-e485-7d28b703506a@gmail.com>
- In-Reply-To
- <20200608082636.GC8232@szeder.dev>
On 6/8/2020 4:26 AM, SZEDER Gábor wrote:
Show 51 quoted lines
> On Mon, Jun 08, 2020 at 01:02:35AM +0530, Abhishek Kumar wrote:
>> diff --git a/commit-graph.c b/commit-graph.c
>> index 7d887a6a2c..f7cca4def4 100644
>> --- a/commit-graph.c
>> +++ b/commit-graph.c
>
>> @@ -1302,8 +1302,8 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
>> ctx->commits.nr);
>> for (i = 0; i < ctx->commits.nr; i++) {
>> display_progress(ctx->progress, i + 1);
>> - if (ctx->commits.list[i]->generation != GENERATION_NUMBER_INFINITY &&
>> - ctx->commits.list[i]->generation != GENERATION_NUMBER_ZERO)
>> + if (commit_graph_generation(ctx->commits.list[i]) != GENERATION_NUMBER_INFINITY &&
>> + commit_graph_generation(ctx->commits.list[i]) != GENERATION_NUMBER_ZERO)
>> continue;
>>
>> commit_list_insert(ctx->commits.list[i], &list);
>> @@ -1314,22 +1314,22 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
>> uint32_t max_generation = 0;
>>
>> for (parent = current->parents; parent; parent = parent->next) {
>> - if (parent->item->generation == GENERATION_NUMBER_INFINITY ||
>> - parent->item->generation == GENERATION_NUMBER_ZERO) {
>> + if (commit_graph_generation(parent->item) == GENERATION_NUMBER_INFINITY ||
>> + commit_graph_generation(parent->item) == GENERATION_NUMBER_ZERO) {
>> all_parents_computed = 0;
>> commit_list_insert(parent->item, &list);
>> break;
>> - } else if (parent->item->generation > max_generation) {
>> - max_generation = parent->item->generation;
>> + } else if (commit_graph_generation(parent->item) > max_generation) {
>> + max_generation = commit_graph_generation(parent->item);
>> }
>> }
>>
>> if (all_parents_computed) {
>> - current->generation = max_generation + 1;
>> + commit_graph_data_at(current)->generation = max_generation + 1;
>> pop_commit(&list);
>>
>> - if (current->generation > GENERATION_NUMBER_MAX)
>> - current->generation = GENERATION_NUMBER_MAX;
>> + if (commit_graph_generation(current) > GENERATION_NUMBER_MAX)
>> + commit_graph_data_at(current)->generation = GENERATION_NUMBER_MAX;
>> }
>> }
>> }
>
> Something about these conversions is not right, as they send
> compute_generation_numbers() into an endless loop, and
> 't5318-commit-graph.sh' hangs because of this.Abhishek responded off-list, but it's worth having the discussion here, too.
While the next patch fixes the bug introduced here, we strive to have every patch compile and pass all tests on all platforms. It can be hard to verify that last "all platforms" condition, but we can run (most) tests on each of our patches using the following:
$ git rebase -x "make -j12 DEVELOPER=1 && (cd t && prove -j8 t[0-8]*.sh)" <base>
Thanks, Szeder, for finding this issue in the patch.
Looking at this patch and patch 3, I think you should just squash that patch into this one, since the code you are removing in patch 3 was added by this one. Add a paragraph in your commit message that details why we need to use commit_graph_data_at() directly in write_graph_chunk_data() and compute_generation_numbers().
Thanks, -Stolee