| Message ID | 3480381b8b99142bcc0213957a43d68a962c52d9.1657700238.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Some improvements to safe.directory on Windows | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > When Git refuses to use an existing repository because it is owned by > someone else than the current user, it can be a bit tricky on Windows to > figure out what is going on. > > Let's help with that by offering some more information via the > environment variable `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`. I can see how the change to compat/ part is useful, but ... > diff --git a/setup.c b/setup.c > index 9dcecda65b0..3ba42ffcb27 100644 > --- a/setup.c > +++ b/setup.c > @@ -1353,13 +1353,23 @@ const char *setup_git_directory_gently(int *nongit_ok) > case GIT_DIR_INVALID_OWNERSHIP: > if (!nongit_ok) { > struct strbuf quoted = STRBUF_INIT; > + struct strbuf hint = STRBUF_INIT; > + > +#ifdef __MINGW32__ > + if (!git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) > + strbuf_addstr(&hint, > + _("\n\nSet the environment variable " > + "GIT_TEST_DEBUG_UNSAFE_DIRECTORIES=true " > + "and run\n" > + "again for more information.")); > +#endif ... 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. Also what's our vision for non-Windows platform for "further debugging aid" in this area? Would we perhaps want a similar "warning" that says "owned by X but you are Y"? Thanks.
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
Hi Junio, On Thu, 14 Jul 2022, Junio C Hamano wrote: > 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 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 >
Hi Junio, On Fri, 15 Jul 2022, Johannes Schindelin wrote: > On Thu, 14 Jul 2022, Junio C Hamano wrote: > > > 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 like it! Let me play with this, after this coming week (during which I > plan to be mostly offline). I had to play with it for a while until I could make it work, but eventually I managed to do it. The second iteration was just sent, and implements this route. Thank you, Dscho
diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt index 74627c5e7c6..18fac9cb7f3 100644 --- a/Documentation/config/safe.txt +++ b/Documentation/config/safe.txt @@ -40,3 +40,9 @@ which id the original user has. If that is not what you would prefer and want git to only trust repositories that are owned by root instead, then you can remove the `SUDO_UID` variable from root's environment before invoking git. ++ +Due to the permission model on Windows where ACLs are used instead of +Unix' simpler permission model, it can be a bit tricky to figure out why +a directory is considered unsafe. To help with this, Git will provide +more detailed information when the environment variable +`GIT_TEST_DEBUG_UNSAFE_DIRECTORIES` is set to `true`. diff --git a/compat/mingw.c b/compat/mingw.c index 38ac35913df..912444fb3ab 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1,6 +1,7 @@ #include "../git-compat-util.h" #include "win32.h" #include <aclapi.h> +#include <sddl.h> #include <conio.h> #include <wchar.h> #include "../strbuf.h" @@ -2676,6 +2677,26 @@ int is_path_owned_by_current_sid(const char *path) IsValidSid(current_user_sid) && EqualSid(sid, current_user_sid)) result = 1; + else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) { + LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; + + if (ConvertSidToStringSidA(sid, &str1)) + to_free1 = str1; + else + str1 = "(inconvertible)"; + + if (!current_user_sid) + str2 = "(none)"; + else if (!IsValidSid(current_user_sid)) + str2 = "(invalid)"; + else if (ConvertSidToStringSidA(current_user_sid, &str2)) + to_free2 = str2; + else + str2 = "(inconvertible)"; + warning("'%s' is owned by:\n\t'%s'\nbut the current user is:\n\t'%s'", path, str1, str2); + LocalFree(to_free1); + LocalFree(to_free2); + } } /* diff --git a/setup.c b/setup.c index 9dcecda65b0..3ba42ffcb27 100644 --- a/setup.c +++ b/setup.c @@ -1353,13 +1353,23 @@ const char *setup_git_directory_gently(int *nongit_ok) case GIT_DIR_INVALID_OWNERSHIP: if (!nongit_ok) { struct strbuf quoted = STRBUF_INIT; + struct strbuf hint = STRBUF_INIT; + +#ifdef __MINGW32__ + if (!git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) + strbuf_addstr(&hint, + _("\n\nSet the environment variable " + "GIT_TEST_DEBUG_UNSAFE_DIRECTORIES=true " + "and run\n" + "again for more information.")); +#endif sq_quote_buf_pretty("ed, dir.buf); die(_("detected dubious ownership in repository at '%s'\n" "To add an exception for this directory, call:\n" "\n" - "\tgit config --global --add safe.directory %s"), - dir.buf, quoted.buf); + "\tgit config --global --add safe.directory %s%s"), + dir.buf, quoted.buf, hint.buf); } *nongit_ok = 1; break;