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

From: Jeff King <peff@peff.net>
Date: Apr 13, 2021, 07:39 UTC
Message-ID: <YHVKuFaR/AjYhf9b@coredump.intra.peff.net>
In-Reply-To: <06e42cf3-188c-d314-b3b9-bdb2f43a37e6@gmail.com>

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

Show 14 quoted lines

> 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