On Fri, Sep 11, 2020 at 01:52:16PM -0400, Jeff King wrote: > On Wed, Sep 09, 2020 at 11:24:00AM -0400, Taylor Blau wrote: > > +With the `--max-new-filters=<n>` option, generate at most `n` new Bloom > > +filters (if `--changed-paths` is specified). If `n` is `-1`, no limit is > > +enforced. Commits whose filters are not calculated are stored as a > > +length zero Bloom filter, and their bit is marked in the `BFXL` chunk. > > +Overrides the `commitGraph.maxNewFilters` configuration. > > The BFXL chunk doesn't exist anymore in this iteration, right? Ack; I'll have to drop that. > I wondered about having a user-facing "-1" here. My gut feeling is that > we usually use "0" to mean "no limit" in other places, and it probably > make sense to be consistent. It does look like we use both, though, and > I'm having trouble formulating a grep pattern to find examples that > doesn't produce a lot of noise. > > These are "0 is no limit": > > pack.windowMemory > pack.deltaCacheSize > git-daemon --max-connections > > These are "-1 is no limit": > > git-grep --max-depth > rev-list --max-parents (I think?) > > So I dunno. It's a pretty minor thing, but I think it's good to aim for > consistency, and since this is user-facing we won't be able to change it > later. I think that we have to treat "-1" as the no-limit indicator, or otherwise we'd have to specify some other way to say we don't want to generate any filters. With this patch, users can write: $ git commit-graph write --changed-paths .. --max-new-filters=0 to generate a commit-graph without writing any new filters. This is important to be able to do since we also have a 'commitGraph.maxNewFilters' configuration, which callers may want to override. You may wonder why you wouldn't just write '--no-changed-paths' instead. Doing so would indeed generate no new filters, but it also wouldn't write any already existing filters into a new graph which is important when rolling up graph layers that already have incrementals, for example with '--split'. I'm happy to include all or none of this in a re-rolled commit message if you think it's relevant, too. > -Peff Thanks, Taylor
