On 3/15/2023 6:49 PM, Jonathan Tan wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
It turns out the progress is a bit redundant here, but not entirely in
the case of ensure_generations_valid() (if progress was enabled).
Let's break down the iteration, which has nested loops:
1. for all commits in the initial list.
2. perform DFS until generation can be computed. (while loop)
When writing a commit-graph file, that initial list is _every commit
in the commit-graph_, so having a display_progress() in the for loop
is sufficient to get the exact number.
In the case of ensure_generations_valid(), the number of assignments
in the while loop can be much larger than the initial input list.
However, ensure_generations_valid() does not use progress _and_ even
if it did, it would make sense to signal progress based on the number
of tips that need to be computed. I'll remove these progress counts
inside the mutators.
Thanks,
-Stolee
