Hi Junio,
On Thu, 14 Jul 2022, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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 like it! Let me play with this, after this coming week (during which I
plan to be mostly offline).
Thanks,
Dscho
>
> 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
>
