Re: [PATCH 10/16] promisor-remote: pass config entry to all_fields_match() directly

[Patrick Steinhardt <ps@pks.im>]

On Mon, Mar 23, 2026 at 09:05:13AM +0100, Christian Couder wrote:
> The `in_list == 0` path of all_fields_match() re-looks up the

This reads a bit weird. How about "looks up the remote in config_info by
advertised->name repeatedly" instead?

> diff --git a/promisor-remote.c b/promisor-remote.c
> index da347fa2dc..8f2c1280c3 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -619,7 +627,11 @@ static int all_fields_match(struct promisor_info *advertised,
>  		if (!value)
>  			return 0;
>  
> -		if (in_list) {
> +		if (config_entry) {
> +			match = match_field_against_config(field, value,
> +							   config_entry);
> +		} else {
> +			struct string_list_item *item;
>  			for_each_string_list_item(item, config_info) {
>  				struct promisor_info *p = item->util;
>  				if (match_field_against_config(field, value, p)) {
> @@ -627,12 +639,6 @@ static int all_fields_match(struct promisor_info *advertised,
>  					break;
>  				}
>  			}
> -		} else {
> -			item = string_list_lookup(config_info, advertised->name);
> -			if (item) {
> -				struct promisor_info *p = item->util;
> -				match = match_field_against_config(field, value, p);
> -			}
>  		}
>  
>  		if (!match)

Okay, the logic is reversed now, which makes sense as we now pass `NULL`
instead of `1`, and the promisor info instead of `0`.

The change itself makes sense, but other than that I have a very hard
time understanding these two functions. I think they would strongly
benefit from some comments explaining what's going on, what the input is
and what we're trying to do. Of course that doesn't have to be part of
this commit here, but I would appreciate a preparatory commit that helps
guide the reader a bit.

Patrick