Subject: Re: [PATCH v2 24/24] pack-bitmap-write: better reuse bitmaps

From: Taylor Blau <[email-masked]>
Date: Wed, 2 Dec 2020 11:35:34 -0500

On Wed, Dec 02, 2020 at 12:08:08AM -0800, Jonathan Tan wrote:
> > +			c_ent->maximal = 1;
> > +			p = NULL;
>
> Here, we're setting maximal without also setting a bit in this commit's
> commit_mask. This is fine because we're not propagating this commit's
> commit_mask to any parents (we're not continuing the walk from this
> commit), but it seems like a code smell. Suggested fix is below.
>
> > +		}
> > +
> >  		if (c_ent->maximal) {
> >  			num_maximal++;
> >  			ALLOC_GROW(bb->commits, bb->commits_nr + 1, bb->commits_alloc);
> >  			bb->commits[bb->commits_nr++] = commit;
> >  		}
>
> As far as I can tell, this means that this commit occupies a bit
> position in the commit mask that it doesn't need. Could this go into a
> separate list instead, to be appended to bb->commits at the very end?
>
> We could even skip the whole maximal stuff (for commits with existing
> bitmaps) and replace "c_ent->maximal = 1;" above with "add to list that
> we're going to append to bb->commits at the very end". That has the
> advantage of not having to redefine "maximal".

Hmm. I'd trust Stolee's opinion over mine here, so I'll be curious what
he has to say.

> >
> > +		if (!c_ent->commit_mask)
> > +			continue;
>
> I think this should be moved as far up as possible (right after
> the call to bb_data_at()) and commented, something like:
>
>   If there is no commit_mask, there is no reason to iterate over this
>   commit; it is not selected (if it were, it would not have a blank
>   commit mask) and all its children have existing bitmaps (see the
>   comment starting with "This commit has an existing bitmap" below), so
>   it does not contribute anything to the final bitmap file or its
>   descendants.

Good suggestion, thanks.

Thanks,
Taylor