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.
>> 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.
It would be equivalent to add it to the list and then continuing the
loop instead of piggy-backing on the if (c_ent->maximal) block, followed
by a trivial loop over the (nullified) parents.
>>>
>>> + 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.
Yeah, makes sense to me.
Thanks,
-Stolee
