Re: [PATCH v2 2/5] setup: extract path_allowlist_apply()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 14, 2026, 17:56 UTC
- Message-ID
- <xmqqecg0oabe.fsf@gitster.g>
- In-Reply-To
- <20260813154748.2378747-3-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
Show 40 quoted lines
> 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 <christian.couder@gmail.com>
> ---
> 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.
Show 38 quoted lines
> + /*
> + * 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?