On Fri, May 05, 2023 at 01:59:31PM -0400, Derrick Stolee wrote: > On 5/5/2023 1:27 PM, Taylor Blau wrote: > > Here is a reroll of my series to implement a boundary-based bitmap > > traversal algorithm that I worked on towards the end of 2021 with Peff. > > > > The algorithm is unchanged from the last version, but the implementation > > has been made much more straightforward, thanks to a very helpful > > suggestion from Stolee. > > > > Instead of hackily trying to write down all of the UNINTERESTING commits > > between the tips and boundary within limit_list(), we can just perform a > > commit-only walk, which will give us the set of commits that we need. > > Something that didn't seem to get attention in this version was buried > deep in the commentary of my high-level review [1]: Oops, sorry, I definitely missed this unintentionally and did not mean to ignore it. > > For these reasons, I'm surprised that this patch completely replaces > > the old algorithm for the new one. I would prefer to see a config > > option that enables this new algorithm. It would be safer to deploy > > that way, too. > > I still think it would be nice to keep the two algorithms for at least > a little while instead of completely removing the old one. Let's gather > some more evidence and get more reps in with the new algorithm before > making it the new default. I could imagine a scenario where someone > really wants to spend the extra time to make sure none of the objects > reachable from the UNINTERESTING commits are included in the output of > this diff. > > [1] https://lore.kernel.org/git/a143150d-7cf5-c697-0e48-0f7af1c6de8f@xxxxxxxxxx/ Hmm. I'm not opposed to keeping the old algorithm around, though I wonder what the configuration option would look like here. I imagine that we don't want to support the old algorithm indefinitely, though. Perhaps something like `pack.useBoundaryBitmapTraversal` (implied by `feature.experimental`), defaulting to "false", and then eventually "true"? Thanks, Taylor
