Re: [PATCH 4/4] last-modified: keep per-path Bloom filters for wildcard pathspecs

[Jeff King <peff@peff.net>]

On Fri, Jul 17, 2026 at 09:16:34PM +0200, Toon Claes wrote:

> > +	/*
> > +	 * prepare_revision_walk() clears bloom_filter_settings for pathspecs
> > +	 * without a Bloom key. Restore it so the per-path check keeps working.
> > +	 */
> > +	if (!lm->rev.bloom_filter_settings)
> > +		lm->rev.bloom_filter_settings =
> > +			get_bloom_filter_settings(lm->rev.repo);
> > +
> 
> @Peff, as far I could tell:
> 
> * This change was not needed to be able to use the Bloom filters with
>   the pathspec.

Ah, right. In my earlier attempt I came at it from the bottom up: I
found the bloom_keyvec, saw how it was populated, and then worked my way
back to prepare_to_use_bloom_filter() without going further.

But it is much nicer if we can rely on prepare_revision_walk() here, as
we don't need to make an additional function public.

> * Only restoring bloom_filter_settings was needed. In your patch you're
>   calling prepare_to_use_bloom_filter(), but that is being called by
>   prepare_revision_walk(). Thus the restoring of the filter settings
>   I've added after that function.

Hmm, OK. The "clearing" done by prepare_revision_walk() is kind of
weird. The bloom settings are a const pointer, not a resource we own, so
there is really no need to clear them.

But accepting for a moment that we do clear them, is this maybe an
indication that we are abusing rev_info.bloom_filter_settings? It is
really an internal implementation detail that revision.c uses for its
own bloom filters. Wouldn't it be cleaner for last-modified to keep its
own, like this:

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index fe012b0c2e..5e176bbeed 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -61,6 +61,8 @@ struct last_modified {
 	size_t all_paths_nr;
 	struct active_paths_for_commit active_paths;
 
+	struct bloom_filter_settings *bloom_filter_settings;
+
 	/* 'scratch' to avoid allocating a bitmap every process_parent() */
 	struct bitmap *scratch;
 };
@@ -114,9 +116,9 @@ static void add_path_from_diff(struct diff_queue_struct *q,
 
 		FLEX_ALLOC_STR(ent, path, path);
 		oidcpy(&ent->oid, &p->two->oid);
-		if (lm->rev.bloom_filter_settings)
+		if (lm->bloom_filter_settings)
 			bloom_key_fill(&ent->key, path, strlen(path),
-				       lm->rev.bloom_filter_settings);
+				       lm->bloom_filter_settings);
 		hashmap_entry_init(&ent->hashent, strhash(ent->path));
 		hashmap_add(&lm->paths, &ent->hashent);
 	}
@@ -262,7 +264,7 @@ static bool maybe_changed_path(struct last_modified *lm,
 	struct last_modified_entry *ent;
 	struct hashmap_iter iter;
 
-	if (!lm->rev.bloom_filter_settings)
+	if (!lm->bloom_filter_settings)
 		return true;
 
 	if (commit_graph_generation(origin) == GENERATION_NUMBER_INFINITY)
@@ -277,7 +279,7 @@ static bool maybe_changed_path(struct last_modified *lm,
 			continue;
 
 		if (bloom_filter_contains(filter, &ent->key,
-					  lm->rev.bloom_filter_settings))
+					  lm->bloom_filter_settings))
 			return true;
 	}
 	return false;
@@ -502,7 +504,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
 		return argc;
 	}
 
-	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
+	lm->bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
 
 	if (populate_paths_from_revs(lm) < 0)
 		return -1;

It's mostly academic, as both of the pointers (if not NULL) would always
point to the same setting that ultimately come from the repository
object. But it feels cleaner for them to keep their own pointers,
because that pointer may also signal "do we have usable bloom filters".
We are a little lucky in dodging a bug here: last-modified uses the
pointer for that purpose, but if revision.c did so also, they'd
conflict.

  Side note: this is really a repository property, so it would be nice
  if we could just do:

    repo_bloom_filter_contains(filter, &ent->key);

  without managing the settings pointer ourselves at all. But the cost
  to fetch it from the graph linked list is not totally trivial, so we'd
  probably end up having to cache it somewhere. I don't know if that's
  worth it (plus last-modified would still have to keep a boolean
  somewhere to decide whether it is using bloom filters or not).

-Peff