Re: [PATCH v6 4/5] promisor-remote: allow a client to check fields

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> diff --git a/promisor-remote.c b/promisor-remote.c
> index ae2c49a0a0..501cb92391 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -388,6 +388,20 @@ static struct string_list *fields_sent(void)
>  	return &fields_list;
>  }
>  
> +static struct string_list *fields_checked(void)
> +{
> +	static struct string_list fields_list = STRING_LIST_INIT_NODUP;
> +	static int initialized = 0;

No need to explicitly 0 initialize "static int"; let BSS take care
of it.  Perhaps we should add an entry to CodingGuidelines if we do
not have one (#leftoverbits).

> +	if (!initialized) {
> +		fields_list.cmp = strcasecmp;
> +		fields_from_config(&fields_list, "promisor.checkFields");
> +		initialized = 1;
> +	}
> +
> +	return &fields_list;
> +}

OK.

> @@ -533,6 +547,61 @@ enum accept_promisor {
>  	ACCEPT_ALL
>  };
>  
> +static int match_field_against_config(const char *field, const char *value,
> +				      struct promisor_info *config_info)
> +{
> +	if (config_info->filter && !strcasecmp(field, promisor_field_filter))
> +		return !strcmp(config_info->filter, value);
> +	else if (config_info->token && !strcasecmp(field, promisor_field_token))
> +		return !strcmp(config_info->token, value);
> +
> +	return 0;
> +}
> +
> +static int all_fields_match(struct promisor_info *advertised,
> +			    struct string_list *config_info,
> +			    int in_list)
> +{
> +	struct string_list* fields = fields_checked();

Asterisk sticks to a variable, not type.  I.e.

	struct string_list *fields = ...;


> +	struct string_list_item *item_checked;
> +
> +	for_each_string_list_item(item_checked, fields) {
> +		int match = 0;
> +		const char *field = item_checked->string;
> +		const char *value = NULL;
> +		struct string_list_item *item;
> +
> +		if (!strcasecmp(field, promisor_field_filter))
> +			value = advertised->filter;
> +		else if (!strcasecmp(field, promisor_field_token))
> +			value = advertised->token;

Hmph, together with match_field_against_config(), do we really need
to have this case insensitive to begin with?  I would suggest making
it a habit to design the interface minimally, and not making case
insensitive comparison as default counts one of them.  If the
comparison were not case insensitive, we do not even need to have
this loop; rather we can just look up from the list of the fields
for an exact string (i.e. promisor_field_filter).  I do not know
offhand what other code will become easier to read and simpler by
such a change.

Thanks.