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

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2023, 19:54 UTC
Message-ID: <xmqqfs82ve34.fsf@gitster.g>
In-Reply-To: <0a0693ad612755e675861dfa5a660baf8d325ed0.1683828635.git.gitgitgadget@gmail.com>

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 36 quoted lines

> +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.