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

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

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