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

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 12, 2024, 07:14 UTC
Message-ID: <ZmlK59HYWY_aXgv5@tanuki>
In-Reply-To: <pull.1743.git.1718118555197.gitgitgadget@gmail.com>

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

Show 38 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.
> 
> 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