Re: [PATCH v2] commit-reach: remove get_reachable_subset()

[Derrick Stolee <stolee@gmail.com>]

On 6/11/2026 9:52 AM, Kristofer Karlsson wrote:
> On Thu, 11 Jun 2026 at 14:57, Derrick Stolee <stolee@gmail.com> wrote:
>> Finally, a commentary: You seem to have a habit of responding to
>> review feedback only through new patch versions, but I'd rather see
>> some thoughts in the discussion thread as direct replies to the review,
>> especially if you think you will change direction like this. Saying
>> something like "Maybe I should update the method to have two walk modes"
>> in a reply would have given me an opportunity to respond and perhaps
>> avoided a new version that went in this direction.
> 
> That's fair, I apologize both for jumping ahead too quickly with a new
> patch and also for evolving it into multiple logical changes
> (both code removal and complex refactoring).
> 
> I will be more mindful going forward about letting the discussion
> settle more before submitting followup patches.
> 
> I have no strong opinion on how to proceed - either park/abandon this
> or go with v1 as-is. I think you're right that having two modes within
> tips_reachable_from_bases is reducing the win here unless the mode is
> truly seamless but the abstraction does leak through a bit.
I think that my biggest issue is that callers are needing to know
something about the performance characteristics of each solution. It
may be better to keep the behavior completely internal: have the
method decide which approach is better based on the information it
has. For instance: is the minimum generation number "infinite"? Then
the BFS is going to be better than the DFS approach. Such an approach
would make it clear why there is the complexity _and_ would improve
both callers in the right scenarios.

You were correct to find two methods that claimed to do the same
thing, but we need to take time to consider the solutions based on
all factors.

Thanks,
-Stolee