Re: [PATCH v2 2/5] setup: extract path_allowlist_apply()

Christian Couder <[email protected]> writes:

> In a following commit we are going to check whether a repository is
> part of an allowlist specified in a config variable.
>
> To prepare for that let's extract existing code from
> safe_directory_cb() into a new path_allowlist_apply() helper that will
> help with such checks.
>
> While at it let's make the helper's code simpler and more generic.
>
> Signed-off-by: Christian Couder <[email protected]>
> ---
>  setup.c | 107 +++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 59 insertions(+), 48 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 95909e9603..39dfa1cc5f 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1339,6 +1339,64 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
>  	}
>  }
>  
> +static void path_allowlist_apply(const char *key, const char *value,
> +				 const char *target_path, int *is_match)
> +{
> +	char *allowed = NULL;
> +	char *normalized = NULL;
> +
> +	if (!value || !*value) {
> +		*is_match = 0;
> +		return;
> +	}
> +
> +	if (!strcmp(value, "*")) {
> +		*is_match = 1;
> +		return;
> +	}
> +
> +	if (git_config_pathname(&allowed, key, value) || !allowed)
> +		return;

The inversion of the polarity from the original here is a nice
touch.  We no longer have to look at deeply indented block to tell
immediately that nothing will happen when the configuration variable
is not set.

> +	/*
> +	 * Setting the config variable to a non-absolute path makes
> +	 * little sense---it won't be relative to the configuration
> +	 * file the item is defined in.  Except for ".", which means
> +	 * "if we are at the top level of a repository, then it is
> +	 * OK", which is slightly tighter than "*" that allows
> +	 * discovery.
> +	 */
> +	if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
> +		warning(_("%s '%s' not absolute"), key, allowed);
> +		goto end;
> +	}
> +
> +	/*
> +	 * A .gitconfig in $HOME may be shared across different
> +	 * machines and the config variable entries may or may not
> +	 * exist as paths on all of these machines.  In other words,
> +	 * it is not a warning worthy event when there is no such path
> +	 * on this machine---the entry may be useful elsewhere.
> +	 */
> +	normalized = real_pathdup(allowed, 0);
> +	if (!normalized)
> +		goto end;
> +
> +	if (ends_with(normalized, "/*")) {
> +		size_t len = strlen(normalized);
> +		if (!fspathncmp(normalized, target_path, len - 1))
> +			*is_match = 1;
> +		goto end;
> +	}
> +
> +	if (!fspathcmp(target_path, normalized))
> +		*is_match = 1;
> +
> +end:
> +	free(normalized);
> +	free(allowed);
> +}

The name "is_match" somehow feels a bit awkward.  How about calling
it 

    *matches = true/false;

instead?