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

On Wed, Aug 12, 2020 at 01:49:29PM +0200, SZEDER Gábor wrote:

On Tue, Aug 11, 2020 at 04:52:14PM -0400, Taylor Blau wrote:

quoted

diff --git a/commit-graph.c b/commit-graph.c
index 6886f319a5..4aae5471e3 100644
--- a/commit-graph.c
+++ b/commit-graph.c

@@ -954,7 +954,8 @@ struct tree *get_commit_tree_in_graph(struct repository *r, const struct commit
 }

 static int get_bloom_filter_large_in_graph(struct commit_graph *g,
-					   const struct commit *c)
+					   const struct commit *c,
+					   uint32_t max_changed_paths)
 {
 	uint32_t graph_pos = commit_graph_position(c);
 	if (graph_pos == COMMIT_NOT_FROM_GRAPH)

@@ -965,6 +966,17 @@ static int get_bloom_filter_large_in_graph(struct commit_graph *g,

 	if (!(g && g->bloom_large))
 		return 0;
+	if (g->bloom_filter_settings->max_changed_paths != max_changed_paths) {
+		/*
+		 * Force all commits which are subject to a different
+		 * 'max_changed_paths' limit to be recomputed from scratch.
+		 *
+		 * Note that this could likely be improved, but is ignored since
+		 * all real-world graphs set the maximum number of changed paths
+		 * at 512.
+		 */
+		return 0;
+	}
 	return bitmap_get(g->bloom_large, graph_pos - g->num_commits_in_base);
 }

@@ -1470,6 +1482,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
 	int i;
 	struct progress *progress = NULL;
 	int *sorted_commits;
+	int max_new_filters;

 	init_bloom_filters();
 	ctx->bloom_large = bitmap_word_alloc(ctx->commits.nr / BITS_IN_EWORD + 1);

@@ -1486,10 +1499,15 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
 		ctx->order_by_pack ? commit_pos_cmp : commit_gen_cmp,
 		&ctx->commits);

+	max_new_filters = ctx->opts->max_new_filters >= 0 ?
+		ctx->opts->max_new_filters : ctx->commits.nr;

git_test_write_commit_graph_or_die() invokes
write_commit_graph_reachable() with opts=0x0, so 'ctx->opts' is NULL,
and we get segfault.  This breaks a lot of tests when run with
GIT_TEST_COMMIT_GRAPH=1 GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1.

Great catch, thanks. Fixing this is as simple as adding 'ctx->opts &&'
right before we dereference 'ctx->opts', since setting this variable
equal to 'ctx->commits.nr' is the right thing to do in that case.

Unrelated to this comment, I am hoping to send out a final version of
this series sometime next week so that we can keep moving forward with
Bloom filter improvements.

Have you had a chance to review the rest of the patches? I'll happily
wait until you have had a chance to do so before sending v5 so that we
can avoid a v6.

Thanks,
Taylor