Re: [PATCH 4/5] commit-graph: be extra careful about mixed generations

>>  	struct topo_level_slab *topo_levels;
>>  	const struct commit_graph_opts *opts;
>> @@ -1452,6 +1453,15 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
>>  		ctx->progress = start_delayed_progress(
>>  					_("Computing commit graph generation numbers"),
>>  					ctx->commits.nr);
>> +
>> +	if (ctx->write_generation_data && !ctx->trust_generation_numbers) {
>> +		for (i = 0; i < ctx->commits.nr; i++) {
>> +			struct commit *c = ctx->commits.list[i];
>> +			repo_parse_commit(ctx->r, c);
>> +			commit_graph_data_at(c)->generation = GENERATION_NUMBER_ZERO;
>> +		}
>> +	}
>> +
> 
> This took me a while to figure out since I spent quite a lot of time
> thinking that you were setting the topological level to zero, _not_ the
> corrected committer date.
> 
> Now that I understand which is which, I agree that this is the right way
> to go forward.
> 
> That said, I do find it unnecessarily complex that we compute both the
> generation number and the topological level in the same loops in
> compute_generation_numbers()...
> 
>>  	for (i = 0; i < ctx->commits.nr; i++) {
>>  		struct commit *c = ctx->commits.list[i];
>>  		uint32_t level;
>> @@ -1480,7 +1490,8 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
>>  				corrected_commit_date = commit_graph_data_at(parent->item)->generation;
>>
>>  				if (level == GENERATION_NUMBER_ZERO ||
>> -				    corrected_commit_date == GENERATION_NUMBER_ZERO) {
>> +				    (ctx->write_generation_data &&
>> +				     corrected_commit_date == GENERATION_NUMBER_ZERO)) {
> 
> ...for exactly reasons like this. It does make sense that they could be
> computed together since their computation is indeed quite similar. But
> in practice I think you end up spending a lot of time reasoning around
> complex conditionals like these.
> 
> So, I feel a little bit like we should spend some effort to split these
> up. I'm OK with a little bit of code duplication (though if we can
> factor out some common routine, that would also be nice). But I think
> there's a tradeoff between DRY-ness and understandability, and that we
> might be on the wrong side of it here.

>>  					all_parents_computed = 0;
>>  					commit_list_insert(parent->item, &list);
>>  					break;
>> @@ -1500,12 +1511,15 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
>>  					max_level = GENERATION_NUMBER_V1_MAX - 1;
>>  				*topo_level_slab_at(ctx->topo_levels, current) = max_level + 1;
>>
>> -				if (current->date && current->date > max_corrected_commit_date)
>> -					max_corrected_commit_date = current->date - 1;
>> -				commit_graph_data_at(current)->generation = max_corrected_commit_date + 1;
>> -
>> -				if (commit_graph_data_at(current)->generation - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)
>> -					ctx->num_generation_data_overflows++;
>> +				if (ctx->write_generation_data) {
>> +					timestamp_t cur_g;
>> +					if (current->date && current->date > max_corrected_commit_date)
>> +						max_corrected_commit_date = current->date - 1;
>> +					cur_g = commit_graph_data_at(current)->generation
>> +					      = max_corrected_commit_date + 1;
>> +					if (cur_g - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)
>> +						ctx->num_generation_data_overflows++;
>> +				}
> 
> Looks like two things happened here:
> 
>   - A new local variable was introduced to store the value of
>     'commit_graph_data_at(current)->generation' (now called 'cur_g'),
>     and
> 
>   - All of this was guarded by a conditional on
>     'ctx->write_generation_data'.
> 
> The first one is a readability improvement, and the second is the
> substantive one, no?