Re: [PATCH v2 3/5] setup: add 'allow_dot' arg to path_allowlist_apply()

Christian Couder <[email protected]> writes:

> A previous commit created path_allowlist_apply() with the goal of later
> reusing that function. But when it will be reused in a following commit
> this function will need to reject non-absolute paths including those
> with a single dot that are currently accepted.
>
> To prepare for reusing path_allowlist_apply(), let's add a
> `bool allow_dot` argument to it, and let's export this function.
>
> While at it let's document it properly in "setup.h".

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

Thanks.