| Message ID | pull.1743.git.1718118555197.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: increment progress indicator | expand |
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
Hi Patrick, On Wed, 12 Jun 2024, Patrick Steinhardt wrote: > 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. > > > > [...] > > 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' ' Good idea! Thank you, Johannes
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
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;