Re: [PATCH 1/3] Allow debugging unsafe directories' ownership
From: Johannes Schindelin <hidden>
Date: 2022-07-15 14:34:03
Hi Junio, On Thu, 14 Jul 2022, Junio C Hamano wrote:
Junio C Hamano [off-list ref] writes:quoted
... 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 like it! Let me play with this, after this coming week (during which I plan to be mostly offline). Thanks, Dscho
quoted hunk ↗ jump to hunk
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