Re: [PATCH 1/3] revision: support tracking uninteresting commits

[Derrick Stolee <derrickstolee@github.com>]

On 5/3/2023 5:48 PM, Taylor Blau wrote:
> On Tue, Apr 25, 2023 at 02:15:49PM -0400, Derrick Stolee wrote:
>> On 4/24/2023 8:00 PM, Taylor Blau wrote:
>>> The boundary-based bitmap walk will want to know which commits were
>>> marked UNINTERESTING in the walk used to discover the boundary.
>>>
>>> Track which commits are marked as such during list limitation as well as
>>> the topo walk step, though the latter is not used.
>>
>> I was surprised to see this as an addition to revision.c, and
>> only specific to limit_list() (and its iterative copy). I would
>> expect this to work for non-topo-order, too. I suppose we couldn't
>> completely trust that the first visit to a commit includes the
>> UNINTERESTING flag if there is clock skew in the commit history.
> 
> Yeah, the distinction between limit_list() and the --topo-order code
> makes things a little funky here. But I think that's OK, since
> `--topo-order` is not likely to be used here, since this is all
> bitmap-based traversal. IOW, I think that it would be reasonable to ban
> the revision args combination of --use-bitmap-index with --topo-order.
 
I think there is a miscommunication here, since the limit_list()
code will only be run if there is a need to "walk to the end" before
outputting results. --topo-order is an example of something that
triggers this need, but it is disabled by default.

It seems that revs->collect_uninteresting would be a condition that
should signal to enable revs->limited so this code is actually
executed.

Taking a look at how this works with your current patch, the only
thing I can think is that since your initial commits include some
UNINTERESTING commits, that actually triggers revs->limited = 1
in handle_commit(). It's an indirect use, and without such a
commit the boundary would be empty, anyway.

Thanks,
-Stolee