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

>> On 12/2/2020 11:35 AM, Taylor Blau wrote:
>>> 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?
>>
>> I don't see any value in having a second list here. That only makes
>> things more complicated.
> 
> It does make things more complicated, but it could help shrink commit
> bitmasks (which seem to be a concern, according to patch 23).
> 
> Suppose num_maximal was 3 and we encountered such a commit (not
> selected, but has an old bitmap). So we increment num_maximal. Then, we
> encounter a selected commit. That commit would then have a bitmask of
> ???01. If we had not incremented num_maximal (which would require a
> second list), then the bitmask would be ???1.