commit-graph: fix a progress indicator bug

[dscho]

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.

Changes since v1:

• Adjusted Stolee's email address to a non-bouncing one
• Added test coverage helpfully provided by Patrick

Cc: Derrick Stolee stolee@gmail.com cc: Patrick Steinhardt ps@pks.im cc: Taylor Blau me@ttaylorr.com

[dscho]

/submit

[gitgitgadget[bot]]

Submitted as pull.1743.git.1718118555197.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1743/dscho/increment-progress-in-commit-graph-v1

To fetch this version to local tag pr-1743/dscho/increment-progress-in-commit-graph-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1743/dscho/increment-progress-in-commit-graph-v1

[gitgitgadget[bot]]

This branch is now known as ds/ahead-behind-fix.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/7770cac9f493f468ca8a65f4a2825363f8e868a4.

[gitgitgadget[bot]]

There was a status update in the "New Topics" section about the branch ds/ahead-behind-fix on the Git mailing list:

Fix for a progress bar.

Will merge to 'next'.
source: <pull.1743.git.1718118555197.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

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

[gitgitgadget[bot]]

User Patrick Steinhardt <ps@pks.im> has been added to the cc: list.

[gitgitgadget[bot]]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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

[gitgitgadget[bot]]

On the Git mailing list, Taylor Blau wrote (reply to this):

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

[gitgitgadget[bot]]

User Taylor Blau <me@ttaylorr.com> has been added to the cc: list.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/9ee4e17336f0ac1a06db3df4a87e3fbbab938b07.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/ahead-behind-fix on the Git mailing list:

Fix for a progress bar.

Will merge to 'next'.
source: <pull.1743.git.1718118555197.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/6886d9d007b0527dc0c0479715a18da381a1ae05.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/df378ec81eecd97ac303243025221914894b0ec3.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/2d750b213e49cedec2a12e3e540951da76603b55.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/0138afbc349cc20f6243fe3b3f439781c88bddf9.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/ahead-behind-fix on the Git mailing list:

Fix for a progress bar.

Will merge to 'master'.
source: <pull.1743.git.1718118555197.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/e1ba4aa52872c2a04ec505e8287f0d0b7e0f74e9.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch ds/ahead-behind-fix on the Git mailing list:

Fix for a progress bar.

Will merge to 'master'.
source: <pull.1743.git.1718118555197.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/e4ecba994c731fb23786b5ae0b6cddc13694fce1.

[gitgitgadget[bot]]

This patch series was integrated into master via https://github.com/git/git/commit/e4ecba994c731fb23786b5ae0b6cddc13694fce1.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/e4ecba994c731fb23786b5ae0b6cddc13694fce1.

[gitgitgadget[bot]]

Closed via e4ecba994c731fb23786b5ae0b6cddc13694fce1.