Re: [PATCH 3/5] setup: refactor `ensure_safe_repository()` testing priorities

[Junio C Hamano <gitster@pobox.com>]

Michael Lohmann <git@lohmann.sh> writes:

> The implicit ownership test takes precedence over the explicit
> allow-listing of a path by "safe.directory" config. Sort by "priority"
> (explicitness). This also allows to more easily integrate additional
> checks.
>
> Make the explicit safe.directory check take precedence over owner check.

I do not think the above argument makes much sense, with the code
with or without this patch.

It would be a very different story if the explicit specification
allowed users to configure a set of directories to be rejected, in
which case a user can mark a directory as unsafe even the
ownership-based rules would allow it, and explicit rules may have
higher "priority".

But that is not what safe_directory_cb() does.

In other words, there is no "priority" among the rules considered by
ensure_safe_repository() helper.  At least, with the shape of the
helper function at this step in the series, all rules are equally
capable of declaring a directory "safe".

If you are in later steps (I haven't read them) introducing ways to
say "this and that directories are explicitly forbidden", perhaps
reordering like this should be done at that point.

Alternatively, you can leave the change here in the middle of the
series, but explain the rationale differently, e.g.,

    With the current code, this change does not make any difference
    because there is no explicit rule that lets you reject a
    directory that the ownership-based rule may accept.  In a later
    step in this series, however, we will introduce a mechanism to
    allow such an explicit rule, at which point the order of checks,
    i.e. seeing the explicit rule reject a directory and failing the
    operation before consulting the ownership-based rule, will start
    to matter.  As a preliminary change, reorder the existing
    checks.

or something like that, perhaps.

> Signed-off-by: Michael Lohmann <git@lohmann.sh>
> ---
>  setup.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 69f6d1b36c..41a12a85ab 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1307,12 +1307,6 @@ static int ensure_safe_repository(const char *gitfile,
>  {
>  	struct safe_directory_data data = { 0 };
>  
> -	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
> -	    (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&
> -	    (!worktree || is_path_owned_by_current_user(worktree, report)) &&
> -	    (!gitdir || is_path_owned_by_current_user(gitdir, report)))
> -		return 1;
> -
>  	/*
>  	 * normalize the data.path for comparison with normalized paths
>  	 * that come from the configuration file.  The path is unsafe
> @@ -1330,7 +1324,16 @@ static int ensure_safe_repository(const char *gitfile,
>  	git_protected_config(safe_directory_cb, &data);
>  
>  	free(data.path);
> -	return data.is_safe;
> +	if (data.is_safe)
> +		return 1;
> +
> +	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
> +	    (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&
> +	    (!worktree || is_path_owned_by_current_user(worktree, report)) &&
> +	    (!gitdir || is_path_owned_by_current_user(gitdir, report)))
> +		return 1;
> +
> +	return 0;
>  }
>  
>  void die_upon_assumed_unsafe_repo(const char *gitfile, const char *worktree,