Re: [GSOC Patch v2 2/4] commit: move members graph_pos, generation to a slab

From: Derrick Stolee <stolee@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Abhishek Kumar" <abhishekkumar8222@gmail.com>
Cc: git@vger.kernel.org, jnareb@gmail.com
Subject: Re: [GSOC Patch v2 2/4] commit: move members graph_pos, generation to a slab
Date: Mon, 8 Jun 2020 08:35:35 -0400	[thread overview]
Message-ID: <c9333a2d-a0d7-0fe4-e485-7d28b703506a@gmail.com> (raw)
In-Reply-To: <20200608082636.GC8232@szeder.dev>

On 6/8/2020 4:26 AM, SZEDER Gábor wrote:
> 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