Re: [PATCH v3 3/4] revision: modify ref_exclusions to handle inclusions

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +static int ref_matched(const char *path,
>> +		       const struct string_list *l,
>> +		       const struct string_list *hidden_refs)
>>  {
>>  	const char *stripped_path = strip_namespace(path);
>>  	struct string_list_item *item;
>>
>> -	for_each_string_list_item(item, &exclusions->excluded_refs) {
>> +	for_each_string_list_item(item, l) {
>>  		if (!wildmatch(item->string, path, 0))
>>  			return 1;
>>  	}
>>
>> -	if (ref_is_hidden(stripped_path, path, &exclusions->hidden_refs))
>> +	if (ref_is_hidden(stripped_path, path, hidden_refs))
>>  		return 1;
>>
>>  	return 0;
>>  }
>>
>> -void init_ref_exclusions(struct ref_exclusions *exclusions)
>> +int ref_excluded(const struct ref_visibility *visibility, const char *path)
>>  {
>> -	struct ref_exclusions blank = REF_EXCLUSIONS_INIT;
>> -	memcpy(exclusions, &blank, sizeof(*exclusions));
>> +	return ref_matched(path, &visibility->excluded_refs, &visibility->hidden_refs);
>>  }
>>
>> -void clear_ref_exclusions(struct ref_exclusions *exclusions)
>> +int ref_included(const struct ref_visibility *visibility, const char *path)
>>  {
>> -	string_list_clear(&exclusions->excluded_refs, 0);
>> -	string_list_clear(&exclusions->hidden_refs, 0);
>> -	exclusions->hidden_refs_configured = 0;
>> +	return ref_matched(path, &visibility->included_refs, &visibility->hidden_refs);
>>  }
>
> It is unexpected that we do "hidden" inside ref_matched().  I would
> have expected that no matter what exclusion or inclusion patterns
> say, hidden things are to be kept hidden.  I.e.  I expected

>
>  - ref_matched(), which takes a path and a list of patterns, checks
>    if the path matches any of the given patterns;
>
>  - ref_excluded(), whcih takes a path and a visibility, asks
>    ref_matched() if the path matches visibility->excluded_refs and
>    relays its answer to the caller.
>
>  - ref_included(), which takes a path and a visibility, asks
>    ref_matched() if the path matches visibility->included_refs and
>    relays its answer to the caller.
>
>  - ref_visibility(), which takes a path and a visibility, goes
>    through the following sequence:
>
>    - if ref_is_hidden() says that the path is hidden, it is not
>      visible;
>
>    - if ref_excluded() says the path is excluded, it is not visible;
>
>    - if ref_included() says the path is included, it is visible;
>
>    - if none of the above triggers, the fate of the path is
>      determined by some default logic.

>
> or something along that line.  That would also allow us to avoid
> having to call ref_is_hidden() more than once when we need to check
> both inclusion and exclusion list.
>
> But perhaps I am missing some requirements to be taken into
> account, so let me read on.
>
> To be honest, I didn't expect the "exclusions can be extended",
> which I alluded to as a future, possible, follow-on work, to be
> included as a part of this topic.  I appreciate your going an extra
> mile, but I am not sure if it was a good change in the context of
> this series.  With this patch, it is not trivial to even validate
> that there is no behaviour change to any users of "struct
> ref_exclusions" when the included_refs list is empty, which is an
> absolute must to avoid regressions.