commit-graph: increment progress indicator
PATCH 1/1 commit-graph: increment progress indicator
From: Derrick Stolee <[email protected]>
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.
---
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(-)
commit-graph.c +1 -1
@@ -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 ;
base-commit: 7b0defb3915eaa0bd118f0996e8c00b4eb2dc1ca
Signature · 1 line
gitgitgadget
Quoted 30 lines · Johannes Schindelin via GitGitGadget
From: Derrick Stolee <[email protected]>
This fixes a bug that was introduced by
368d19b0b7(commit-graph:
refactorcompute_topological_levels(), 2023-03-20): Previously, the progress indicator was updated fromi + 1whereiis the loop variable of the enclosingforloop. After this patch, the update usedinfo->progress_cnt + 1instead, however, unlikei, theprogress_cntattribute was not incremented. Let's increment it.Signed-off-by Derrick Stolee <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>---
commit-graph: fix a progress indicator bugStolee 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/1743commit-graph.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)commit-graph.c +0
-0
commit-graph.c static void compute_reachable_generation_numbers( · line 1603
Quoted 8 lines
commit-graph.c +1
-1@@ -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
Hi Patrick,
Quoted 24 lines · Patrick Steinhardt
Quoted 10 lines · Johannes Schindelin via GitGitGadget
From: Derrick Stolee <[email protected]>
This fixes a bug that was introduced by
368d19b0b7(commit-graph:
refactorcompute_topological_levels(), 2023-03-20): Previously, the progress indicator was updated fromi + 1whereiis the loop variable of the enclosingforloop. After this patch, the update usedinfo->progress_cnt + 1instead, however, unlikei, theprogress_cntattribute was not incremented. Let's increment it.[...]
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:
Quoted 13 lines
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' 'Good idea!
Thank you,
Johannes
Hi Johannes,
Quoted 8 lines · Johannes Schindelin via GitGitGadget
From: Derrick Stolee <[email protected]>
This fixes a bug that was introduced by
368d19b0b7(commit-graph:
refactorcompute_topological_levels(), 2023-03-20): Previously, the progress indicator was updated fromi + 1whereiis the loop variable of the enclosingforloop. After this patch, the update usedinfo->progress_cnt + 1instead, however, unlikei, theprogress_cntattribute 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.
commit-graph.c static void compute_reachable_generation_numbers( · line 1600
Quoted 10 lines
commit-graph.c +1
-1@@ -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< ---
commit-graph.c +1 -2
@@ -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