Subject: Re: [PATCH v2 0/2] pack-bitmap: boundary-based bitmap traversalt

From: Taylor Blau <[email-masked]>
Date: Fri, 5 May 2023 14:46:14 -0400

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/[email-masked]/

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