Re: [PATCH 8/9] promisor-remote: keep advertised filter in memory

[Patrick Steinhardt <ps@pks.im>]

On Wed, Feb 04, 2026 at 11:57:42AM +0100, Christian Couder wrote:
> On Wed, Jan 7, 2026 at 11:05 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Tue, Dec 23, 2025 at 12:11:12PM +0100, Christian Couder wrote:
> > > diff --git a/promisor-remote.c b/promisor-remote.c
> > > index 8d6d2d7b76..d5f3223cd0 100644
> > > --- a/promisor-remote.c
> > > +++ b/promisor-remote.c
> > > @@ -837,6 +838,7 @@ static void filter_promisor_remote(struct repository *repo,
> > >       struct store_info *store_info = NULL;
> > >       struct string_list_item *item;
> > >       bool reload_config = false;
> > > +     struct string_list captured_filters = STRING_LIST_INIT_DUP;
> > >
> > >       if (!repo_config_get_string_tmp(the_repository, "promisor.acceptfromserver", &accept_str)) {
> > >               if (!*accept_str || !strcasecmp("None", accept_str))
> >
> > Nit: I found the "captured" terminology to be somewhat confusing. Can we
> > maybe rename this to `advertised_filters` to clarify?
> 
> Well "advertised_filter" is already used and I think it might be
> confusing to use a very similar name, so for now until we find a
> better name, I kept "captured" in v2 even if it's not the best.
> 
> What about using `server_filters`?

I think that'd work better than "captured". But we should probably not
call it "sever" but "remote" instead, so `remote_filters`. I would be
happy with such a rename.

Another alternative would be `accepted_filters` to stress the fact that
it's not the complete list of filters. I'd be happy with either though.

> > > @@ -935,3 +963,23 @@ void mark_promisor_remotes_as_accepted(struct repository *r, const char *remotes
> > >
> > >       string_list_clear(&accepted_remotes, 0);
> > >  }
> > > +
> > > +char *promisor_remote_construct_filter(struct repository *repo)
> > > +{
> > > +     struct string_list advertised_filters = STRING_LIST_INIT_NODUP;
> > > +     struct promisor_remote *r;
> > > +     char *result;
> > > +
> > > +     promisor_remote_init(repo);
> > > +
> > > +     for (r = repo->promisor_remote_config->promisors; r; r = r->next) {
> > > +             if (r->accepted && r->advertised_filter)
> > > +                     string_list_append(&advertised_filters, r->advertised_filter);
> >
> > Would we ever accept a promisor remote that _doesn't_ have an advertised
> > filter? If not, should we maybe `BUG()` in case the advertised filter
> > has not been set?
> 
> I think it should be fine to accept a promisor remote without an
> advertised filter. The server might prefer to not advertise filters
> because it thinks that the client should determine the best filter
> based on the client needs. That's how it works now.

Okay, fair.

Patrick