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

On Tue, Jun 30, 2020 at 03:27:18PM -0400, Taylor Blau wrote:

So, I'm certainly open to changing it, although I'm not sure that I'm as
worried about exposing the implementation detail as I am about squatting
on Bloom filters within Git in general. I don't think that this
configuration will end up getting used by folks other than server
administrators and for debugging purposes, so those populations are
already likely to be aware of changed-path Bloom filters beforehand.

Yeah, the squatting thing is definitely my bigger concern (having been
through the "bitmaps" version of the same thing).

quoted

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. :)

That's a little shorter, and it adds the verb 'read', which is more
descriptive than 'use' (I touch on this in the third patch, where I say
that this configuration variable _doesn't_ affect the '--changed-path'
option when writing).

Yeah, saying "read" specifically is much nicer.

quoted

quoted

+	prepare_repo_settings(the_repository);
+
 	graph = read_commit_graph_one(the_repository, odb);

I wondered why we would need this prepare_repo_settings() now, when it
should have been needed already to cover core.commitGraph already. I
strongly suspect the answer is: "test-tool read-graph" never properly
respected core.commitGraph in the first place.

Yep. Could probably be broken out into a separate patch (or mentioned as
an aside in this one), but you're right: this helper did not respect
any configuration that 'prepare_repo_settings' picks up.

I'd probably just note it in the commit message, but I'd be fine with
that or with a separate patch.

-Peff