Re: [PATCH 1/3] Allow debugging unsafe directories' ownership

> Junio C Hamano <gitster@pobox.com> writes:
>
> > ... I am not sure about this part.  Do we have any other codepath to
> > show "to debug, run the program with this" suggestion?  Adding it in
> > the documentation is probably good, but this is an extra message
> > that is much larger than the "owned by X but you are Y" message that
> > would be shown.  With or without the environment set, the output
> > will become noisier with this patch.  I wonder if we are better off
> > giving the information that is given in the warning (in compat/ part
> > of the patch) _unconditionally_ in the message, which would make it
> > less noisy overall.
>
> I am wondering if passing a struct to allow is_path_owned_by*()
> helper(s) to report the detail of the failures they discover a
> cleaner way to do this.  To illustrate what I meant, along the
> lines of the following patch, with any additional code to actually
> stuff messages in the strbuf report, in the is_path_owned_by*()
> implementation.

>
> I am perfectly OK if we make it a debug-only option by hiding this
> behind an environment variable, but if we were to do so, I do not
> want to see us advertize the environment variable in the die()
> message (a debug-only option can be documented, but not worth
> surfacing in and contaminating the usual UI output).
>
> Comments?
>
>  compat/mingw.c    |  2 +-
>  git-compat-util.h |  3 ++-
>  setup.c           | 12 +++++++++---
>  3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git c/compat/mingw.c w/compat/mingw.c
> index 2607de93af..f12b7df16d 100644
> --- c/compat/mingw.c
> +++ w/compat/mingw.c
> @@ -2673,7 +2673,7 @@ static PSID get_current_user_sid(void)
>  	return result;
>  }
>
> -int is_path_owned_by_current_sid(const char *path)
> +int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  {
>  	WCHAR wpath[MAX_PATH];
>  	PSID sid = NULL;
> diff --git c/git-compat-util.h w/git-compat-util.h
> index 58d7708296..de34b0ea7e 100644
> --- c/git-compat-util.h
> +++ w/git-compat-util.h
> @@ -487,7 +487,8 @@ static inline void extract_id_from_env(const char *env, uid_t *id)
>  	}
>  }
>
> -static inline int is_path_owned_by_current_uid(const char *path)
> +struct strbuf;
> +static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)
>  {
>  	struct stat st;
>  	uid_t euid;
> diff --git c/setup.c w/setup.c
> index 09b6549ba9..ed823585f7 100644
> --- c/setup.c
> +++ w/setup.c
> @@ -1143,12 +1143,18 @@ static int ensure_valid_ownership(const char *gitfile,
>  	struct safe_directory_data data = {
>  		.path = worktree ? worktree : gitdir
>  	};
> +	struct strbuf report = STRBUF_INIT;
>
>  	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
> -	   (!gitfile || is_path_owned_by_current_user(gitfile)) &&
> -	   (!worktree || is_path_owned_by_current_user(worktree)) &&
> -	   (!gitdir || is_path_owned_by_current_user(gitdir)))
> +	    (!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))) {
> +		if (report.len) {
> +			fputs(report.buf, stderr);
> +			strbuf_release(&report);
> +		}
>  		return 1;
> +	}
>
>  	/*
>  	 * data.path is the "path" that identifies the repository and it is
>