* Re: [PATCH] commit-graph: increment progress indicator
2024-06-11 15:09 [PATCH] commit-graph: increment progress indicator Johannes Schindelin via GitGitGadget
@ 2024-06-12 7:14 ` Patrick Steinhardt
2024-06-12 8:26 ` Johannes Schindelin
2024-06-12 14:43 ` Taylor Blau
1 sibling, 1 reply; 4+ messages in thread
From: Patrick Steinhardt @ 2024-06-12 7:14 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Derrick Stolee, Johannes Schindelin
[-- Attachment #1: Type: text/plain, Size: 3031 bytes --]
On Tue, Jun 11, 2024 at 03:09:15PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> commit-graph: fix a progress indicator bug
>
> Stolee noticed this bug when integrating the for-each-ref --ahead-behind
> patches into GitHub's internal fork of Git, and fixed it. For a variety
> of reasons, upstreaming this fix fell between the cracks. Until now.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1743%2Fdscho%2Fincrement-progress-in-commit-graph-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1743/dscho/increment-progress-in-commit-graph-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1743
>
> commit-graph.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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);
>
> if (gen != GENERATION_NUMBER_ZERO && gen != GENERATION_NUMBER_INFINITY)
> continue;
The fix looks obviously correct. Do we also want to amend tests? We have
e.g. "t6500-gc.sh", "gc --no-quiet", where we already grep for the
progress report without verifying numbers. The output there is:
Computing commit graph topological levels: 25% (1/4), done.
Computing commit graph generation numbers: 25% (1/4), done.
, which clearly demonstrates the bug for both callsites of the buggy
function.
The following change would thus detect such regressions in the future:
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 43d40175f8..1b5909d1b7 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -158,7 +158,7 @@ test_expect_success TTY 'with TTY: gc --no-quiet' '
git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
test_must_be_empty stdout &&
test_grep "Enumerating objects" stderr &&
- test_grep "Computing commit graph generation numbers" stderr
+ test_grep "Computing commit graph generation numbers: 100% (4/4), done." stderr
'
test_expect_success 'gc --quiet' '
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] commit-graph: increment progress indicator
2024-06-11 15:09 [PATCH] commit-graph: increment progress indicator Johannes Schindelin via GitGitGadget
2024-06-12 7:14 ` Patrick Steinhardt
@ 2024-06-12 14:43 ` Taylor Blau
1 sibling, 0 replies; 4+ messages in thread
From: Taylor Blau @ 2024-06-12 14:43 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Derrick Stolee, Johannes Schindelin
Hi Johannes,
On Tue, Jun 11, 2024 at 03:09:15PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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.
> 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
^ permalink raw reply related [flat|nested] 4+ messages in thread