Re: [PATCH v4 6/6] last-modified: keep per-path Bloom filters for wildcard pathspecs
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 10, 2026, 07:04 UTC
- Message-ID
- <aqJWihcFmX7tPio5@pks.im>
- In-Reply-To
- <20260901-toon-speed-up-last-modified-v4-6-a09949800404@iotcl.com>
On Tue, Sep 01, 2026 at 11:10:26AM +0200, Toon Claes wrote:
Show 19 quoted lines
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c > index 8ab7944314..bedccb3ace 100644 > --- a/builtin/last-modified.c > +++ b/builtin/last-modified.c > @@ -370,6 +375,14 @@ static int last_modified_run(struct last_modified *lm) > > prepare_revision_walk(&lm->rev); > > + /* > + * 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); > + > max_count = lm->rev.max_count; > > init_active_paths_for_commit(&lm->active_paths);
So the revision subsystem is unhappy, but we basically force the bloom filter settings in there anyway? That feels a bit fragile to me. Is there a reason why the revision machinery itself specifically needs to have the bloom filters populated, or do we basically just have to set up the bloom filters so that we can access them ourselves?
If the latter, can't we instead store the bloom filter settings in `struct last_modified` instead of forcing them into the revision machinery? Something like the below patch on top of tihs, which still passes all of our tests.
There might be good reasons though why we can't do it this way.
Patrick
diff --git a/builtin/last-modified.c b/builtin/last-modified.c index bedccb3ace..dabd0b7c34 100644 --- a/builtin/last-modified.c +++ b/builtin/last-modified.c @@ -58,6 +58,8 @@ struct last_modified { bool nul_termination; int max_depth; + struct bloom_filter_settings *bloom_filter_settings; + const char **all_paths; size_t all_paths_nr; struct active_paths_for_commit active_paths; @@ -117,9 +119,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); } @@ -265,7 +267,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) @@ -294,7 +296,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; @@ -375,14 +377,6 @@ static int last_modified_run(struct last_modified *lm) prepare_revision_walk(&lm->rev); - /* - * 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); - max_count = lm->rev.max_count; init_active_paths_for_commit(&lm->active_paths); @@ -530,7 +524,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;