Re: [PATCH] commit-graph: increment progress indicator

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 12, 2024, 14:43 UTC
Message-ID: <Zmm0JcJRguLqxeif@nand.local>
In-Reply-To: <pull.1743.git.1718118555197.gitgitgadget@gmail.com>

Hi Johannes,

On Tue, Jun 11, 2024 at 03:09:15PM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 8 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
>
> This fixes a bug that was introduced by 368d19b0b7 (commit-graph:
> refactor compute_topological_levels(), 2023-03-20): Previously, the
> progress indicator was updated from `i + 1` where `i` is the loop
> variable of the enclosing `for` loop. After this patch, the update used
> `info->progress_cnt + 1` instead, however, unlike `i`, the
> `progress_cnt` attribute was not incremented. Let's increment it.

Nice find and fix. I remember vaguely working on what became upstream 368d19b0b7 with Stolee, and I'm glad to see the bug fix he wrote on top is also going upstream.

Show 10 quoted lines

> diff --git a/commit-graph.c b/commit-graph.c
> index e5dd3553dfe..41a2e1b4c6d 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1597,7 +1597,7 @@ static void compute_reachable_generation_numbers(
>  		timestamp_t gen;
>  		repo_parse_commit(info->r, c);
>  		gen = info->get_generation(c, info->data);
> -		display_progress(info->progress, info->progress_cnt + 1);
> +		display_progress(info->progress, ++info->progress_cnt);

It looks like this info->progress_cnt is only used in compute_reachable_generation_numbers() here, so I wonder if it may be worth it to do the following on top (ideally squashed into your patch here):

--- 8< ---

diff --git a/commit-graph.c b/commit-graph.c
index 41a2e1b4c6..0410f6a9c3 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1558,7 +1558,6 @@ struct compute_generation_info {
 	struct repository *r;
 	struct packed_commit_list *commits;
 	struct progress *progress;
-	int progress_cnt;

 	timestamp_t (*get_generation)(struct commit *c, void *data);
 	void (*set_generation)(struct commit *c, timestamp_t gen, void *data);
@@ -1597,7 +1596,7 @@ static void compute_reachable_generation_numbers(
 		timestamp_t gen;
 		repo_parse_commit(info->r, c);
 		gen = info->get_generation(c, info->data);
-		display_progress(info->progress, ++info->progress_cnt);
+		display_progress(info->progress, i + 1);

 		if (gen != GENERATION_NUMBER_ZERO && gen != GENERATION_NUMBER_INFINITY)
 			continue;
--- >8 ---

That would get rid of the info->progress_cnt field entirely, which seems
beneficial since it's only used by this single function, and we already
have 'i' which we can use as a replacement (as you note, effectively the
pre-image behavior of 368d19b0b7).

But I do not feel strongly either way, so no worries if you'd prefer to
keep this as-is.

Thanks,
Taylor