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

[Jeff King <peff@peff.net>]

On Mon, Apr 12, 2021 at 08:48:24PM -0400, Derrick Stolee wrote:

> On 4/11/2021 11:41 PM, Jeff King wrote:> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > 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.

I think it is even more subtle than that. Both topics (the one to
convert callers to use nr_seen as progress, and the one adding this call
to use nr_result for progress) were "in flight" at the same time, but in
a funny way. The latter was written much earlier in a fork of Git, but
not sent upstream immediately. So the nr_seen conversion didn't know
about it, and later the other topic was "merged" (actually,
cherry-picked) into upstream.

So it really is a semantic conflict when merging two branches that
happened simultaneously. It's a little odd in that the branches were
running simultaneously for years, but the same thing could happen even
with two topics much closer together.

Anyway, that's all just an interesting sidenote.

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

Thanks for double-checking. That make sense, since we added only this
one call, and the others were all converted to nr_seen when it was
introduced.

-Peff