Re: [PATCH 12/12] builtin/commit-graph.c: introduce '--max-new-filters=<n>'

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:

quoted

+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