Re: [PATCH] pack-objects: update "nr_seen" progress based on pack-reused count

From: Derrick Stolee <stolee@gmail.com>
Date: Apr 13, 2021, 00:48 UTC
Message-ID: <06e42cf3-188c-d314-b3b9-bdb2f43a37e6@gmail.com>
In-Reply-To: <YHPBXu9R/7i1jZhy@coredump.intra.peff.net>

On 4/11/2021 11:41 PM, Jeff King wrote:> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c

Show 10 quoted lines

> index 525c2d8552..faee5a5c76 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3527,7 +3527,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
>  			&reuse_packfile_bitmap)) {
>  		assert(reuse_packfile_objects);
>  		nr_result += reuse_packfile_objects;
> -		display_progress(progress_state, nr_result);
> +		nr_seen += reuse_packfile_objects;
> +		display_progress(progress_state, nr_seen);

nr_seen and nr_result are defined in the same line with nr_written, as static globals. I can understand how this mistake happened.

There are no other places where nr_result is used for display_progress() while nr_seen _is_ used a couple more times.

Show 22 quoted lines

> +test_expect_success 'enumerating progress counts pack-reused objects' '
> +	count=$(git rev-list --objects --all --count) &&
> +	git repack -adb &&
> +
> +	# check first with only reused objects; confirm that our progress
> +	# showed the right number, and also that we did pack-reuse as expected.
> +	# Check only the final "done" line of the meter (there may be an
> +	# arbitrary number of intermediate lines ending with CR).
> +	GIT_PROGRESS_DELAY=0 \
> +		git pack-objects --all --stdout --progress \
> +		</dev/null >/dev/null 2>stderr &&
> +	grep "Enumerating objects: $count, done" stderr &&
> +	grep "pack-reused $count" stderr &&
> +
> +	# now the same but with one non-reused object
> +	git commit --allow-empty -m "an extra commit object" &&
> +	GIT_PROGRESS_DELAY=0 \
> +		git pack-objects --all --stdout --progress \
> +		</dev/null >/dev/null 2>stderr &&
> +	grep "Enumerating objects: $((count+1)), done" stderr &&
> +	grep "pack-reused $count" stderr
> +'

Good test. LGTM.

Thanks, -Stolee