Re: [PATCH v3 3/8] commit-graph: combine generation computations

[Jonathan Tan <jonathantanmy@google.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +static void compute_reachable_generation_numbers_1(
> +			struct compute_generation_info *info,
> +			int generation_version)
>  {
>  	int i;
>  	struct commit_list *list = NULL;
>  
> -	if (ctx->report_progress)
> -		ctx->progress = start_delayed_progress(
> -					_("Computing commit graph topological levels"),
> -					ctx->commits.nr);
> -	for (i = 0; i < ctx->commits.nr; i++) {
> -		struct commit *c = ctx->commits.list[i];
> -		uint32_t level;
> +	for (i = 0; i < info->commits->nr; i++) {
> +		struct commit *c = info->commits->list[i];
> +		timestamp_t gen;
> +		repo_parse_commit(info->r, c);
> +		gen = info->get_generation(c, info->data);
>  
> -		repo_parse_commit(ctx->r, c);
> -		level = *topo_level_slab_at(ctx->topo_levels, c);
> +		display_progress(info->progress, info->progress_cnt + 1);
>  
> -		display_progress(ctx->progress, i + 1);
> -		if (level != GENERATION_NUMBER_ZERO)
> +		if (gen != GENERATION_NUMBER_ZERO && gen != GENERATION_NUMBER_INFINITY)
>  			continue;
>  
>  		commit_list_insert(c, &list);

So this replaces a call to display_progress with another...

>  			if (all_parents_computed) {
>  				pop_commit(&list);
> -
> -				if (max_level > GENERATION_NUMBER_V1_MAX - 1)
> -					max_level = GENERATION_NUMBER_V1_MAX - 1;
> -				*topo_level_slab_at(ctx->topo_levels, current) = max_level + 1;
> +				gen = compute_generation_from_max(
> +						current, max_gen,
> +						generation_version);
> +				info->set_generation(current, gen, info->data);
>  			}

...here is where set_generation is called...

> +static void set_topo_level(struct commit *c, timestamp_t t, void *data)
> +{
> +	struct write_commit_graph_context *ctx = data;
> +	*topo_level_slab_at(ctx->topo_levels, c) = (uint32_t)t;
> +	display_progress(ctx->progress, ctx->progress_cnt + 1);
> +}

...is this display_progress() redundant? (set_topo_level() is one of the
possibilities that set_generation could be assigned to.) There already
seems to be one at the top. Further supporting my query is the fact that
in the hunk containing set_generation, there is no progress report on
the LHS of the diff.

> +static void set_generation_v2(struct commit *c, timestamp_t t, void *data)
> +{
> +	struct write_commit_graph_context *ctx = data;
> +	struct commit_graph_data *g = commit_graph_data_at(c);
> +	g->generation = (uint32_t)t;
> +	display_progress(ctx->progress, ctx->progress_cnt + 1);
> +}

Likewise for this function.

Everything else up to and including this patch looks good.