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

From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, dstolee@microsoft.com, gitster@pobox.com,
	szeder.dev@gmail.com
Subject: Re: [PATCH 12/12] builtin/commit-graph.c: introduce '--max-new-filters=<n>'
Date: Fri, 11 Sep 2020 14:59:34 -0400	[thread overview]
Message-ID: <20200911185934.GA2871@xor.lan> (raw)
In-Reply-To: <20200911175216.GA2693949@coredump.intra.peff.net>

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