On 12/7/2020 1:24 PM, Jonathan Tan wrote:
>> 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.
OK, I see the value. The value is bounded, since the number of
these "0" gaps is bounded by the number of selected commits _and_
reduces the possible number of maximal commits.
However, that seems like enough justification to create the second
list.
Thanks,
-Stolee
