Re: [PATCH 08/10] bloom: split 'get_bloom_filter()' in two

On Tue, Aug 04, 2020 at 09:00:31AM -0400, Derrick Stolee wrote:

On 8/3/2020 2:57 PM, Taylor Blau wrote:

quoted

'get_bloom_filter' takes a flag to control whether it will compute a
Bloom filter if the requested one is missing. In the next patch, we'll
add yet another flag to this method, which would force all but one
caller to specify an extra 'NULL' parameter at the end.

Instead of doing this, split 'get_bloom_filter' into two functions:
'get_bloom_filter' and 'get_or_compute_bloom_filter'. The former only
looks up a Bloom filter (and does not compute one if it's missing,
thus dropping the 'compute_if_not_present' flag). The latter does
compute missing Bloom filters, with an additional parameter to store
whether or not it needed to do so.

This simplifies many call-sites, since the majority of existing callers
to 'get_bloom_filter' do not want missing Bloom filters to be computed
(so they can drop the parameter entirely and use the simpler version of
the function).

quoted

+struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
+						 struct commit *c,
+						 int compute_if_not_present,
+						 int *computed)

Could we further simplify this by letting "computed" be the indicator
for whether we should compute the filter? If "computed" is NULL, then
we won't compute it directly. This allows us to reduce the "1, NULL)"
to "NULL)" in these callers:

I like what you're getting at--that is, that we shouldn't make calling
this more complicated than necessary, and right now lots of callers
always pass "1, NULL)" as the last two arguments--but I'm not sure that
I like this suggestion.

I could imagine a future caller would want to compute the Bloom filters
if missing, but not care about whether or not they were computed from
scratch. In that case, they'd need a dummy variable. Not the worst thing
in the world, but I think it's less clear.

By the way, I think that this suggestion only helps for "0, NULL" into
just "NULL", not "1, NULL" (which requires a dummy variable with your
suggestion).

quoted

+			struct bloom_filter *filter = get_or_compute_bloom_filter(ctx->r, c, 1, NULL);

quoted

+	filter = get_or_compute_bloom_filter(the_repository, c, 1, NULL);

Thanks,
-Stolee

Thanks,
Taylor