Re: [PATCH 3/3] commit-graph: respect 'core.useBloomFilters'

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 13, 2020, 19:22 UTC
Message-ID: <20200713192250.GB77607@syl.lan>
In-Reply-To: <CAL3xRKdZyE+9r-bPTDo_Fiz=nT_Y7uve+rvBqQZxjL-DYMGYpw@mail.gmail.com>

On Wed, Jul 01, 2020 at 11:58:53AM +0200, Son Luong Ngoc wrote:

Show 23 quoted lines

> Hi folks,
>
> On Tue, 30 Jun 2020 15:33:40 -0400, Jeff King wrote:
>
> > > > It might even be worth considering whether "changed paths" needs more
> > > > context (or would if we add new features in the future). On a "git
> > > > commit-graph write" command-line it is perfectly clear, but would
> > > > core.commitGraphChangedPaths be worth it? It's definitely more specific,
> > > > but it's also way more ugly. ;)
> > >
> > > Here's a third option what about 'graph.readChangedPaths'. I think that
> > > Stolee and I discussed a new top-level 'graph' section, since we now
> > > have a few commit-graph-related configuration variables in 'core'.
> >
> > Yes, I like that even better. Probably "graph" is sufficiently specific
> > within Git's context, though I guess it _could_ bring to mind "git log
> > --graph". So many overloaded terms. :)
>
> I would suggest using 'commitgraph.readChangedPaths' as I was planning on
> implementing the same config in [1] but never got around to it.
>
> From an end-user perspective, not server admin, 'graph' is very much
> correlated to 'git log --graph'.

Do users really correlate the top-level 'graph.*' configuration with options *just* related to 'git log --graph'? I find this unlikely, but I would welcome the opinions of others, too.

Show 19 quoted lines

> Using 'commitgraph' instead of core could also help us enabling more config
> down the line that equate to the current options in 'git commit-graph write'.
>
> I.e. something like 'commitgraph.writeSplit' might be desirable to tune the
> behavior of 'gc.writeCommitGraph' to use '--split=replace' strategy.
>
> ---
>
> @Taylor: Thanks a lot for implementing this.
>
> On Tue, 30 Jun 2020 13:17:36 -0400, Taylor Blau wrote:
>
> > We're planning on using these patches as part of a two-phase roll-out of
> > changed-path Bloom filters, where the first phase conditions whether or
> > not repositories *write* Bloom filters, and the second phase (controlled
> > via the new 'core.useBloomFilters') controls whether repositories *read*
> > their Bloom filters.
>
> Could you elaborate a bit more on the 'two-phase roll-out' mentioned here?

Sure. What I am referring to is the ability to control independently which repositories write Bloom filters during commit-graph generation (via some background jobs, the details of which are unimportant), and which repositories read Bloom filters when, for eg. running 'git log' or similar.

This allows us to quickly recover from any sort of bug in, say, 'git log's use of Bloom filters without having to drop the (otherwise correct) Bloom filters from disk. In other words, it allows us to pretend that they are not there.

> I was looking for a way to verify whether a commit-graph chain has been
> written with Bloom filter (and force it to rewrite if not) but there seems
> to be no straightforward way?

No, 'git commit-graph verify' does not deal with Bloom filters for now. It may be worthwhile to add that functionality, though.

> Do we need to implement a flag in 'git commit-graph verify' to check
> for Bloom filter existence?

Checking for existence would be one thing. More helpful would be regenerating those Bloom filters and checking that we get the same result. Allowing the caller to specify which one would be helpful, too.

Thanks.

> [1]: https://github.com/gitgitgadget/git/pull/633
>
> Regards,
> Son Luong.

Thanks, Taylor