From: Christian Couder <christian.couder@gmail.com>
Date: Tue, 08 Sep 2026 16:55:07 GMT
Subject: Re: [PATCH v2 3/5] setup: add 'allow_dot' arg to path_allowlist_apply()
Message-ID: <CAP8UFD1T_+EKRu7BcdNn_ga=vPz27xZb+yeVWVCnKrnU3zFRpQ@mail.gmail.com>
In-Reply-To: <xmqqy0e8mv0k.fsf@gitster.g>

On Fri, Aug 14, 2026 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:

> If this is just "I want to add an extra caller that has specific
> need and do not care about others in the future", this may be OK but
> as a public function, this is a bit disappointing API design.

I thought that flags might be enough at least for some time, but I
agree that it could soon make the code difficult to reason about,
which is not a good thing for this kind of code.

> I expected, as a generally useful function, you would instead add a
> callback function to allow replacing the use of is_absoute_path()
> plus the warning there, i.e.
>
> void path_allowlist_apply(const char *key, const char *value,
>                           const char *target_path, bool *matches,
>                           bool (*allow_path)(const char *path))
> {
>         ...
>
>         if (!allow_path(allowed))
>                 goto end;
>
> Also to avoid limiting this to configuration callback, I might
> recommend to have it be more like this:
>
> void path_allowlist_apply(const char *allowed, const char *target_path,
>                           bool *matches,
>                           bool (*allow_path)(const char *path, void *cbdata),
>                           void *allow_path_cbdata)
>
> where the original safe-directory thing may call
> git_config_pathname() to compute allowed before calling this helper,
> and pass the address of something like:
>
>         struct { const char *key, *value } cbdata = {
>                 .key = key, .value = value;
>         };
>
> as the cbdata, and pass something like this
>
>         static bool allow_safe_dir(const char *path, void *cbdata_)
>         {
>                 struct { const char *key, *value } *cbdata = _cbdata;
>                 if (is_absoute_path(path) || !strcmp(path, ".")
>                         return true; /* ok */
>
>                 warning(_("%s '%s' not absolute"), cbdata->key, path);
>                 return false;
>         }
>
> as the allow_path callback function.  IOW warning, or insisting on
> it being absolute, etc., does not have to be carved in stone.

I have tried to implement it like you suggest in the v3 I just sent.

Thanks.