[PATCH v2 0/5] Some improvements to safe.directory on Windows

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

Due to the semantics being substantially different from Unix, the
safe.directory feature presents its own set of problems on Windows. One
particular issue would have prevented it from working in GitHub Actions'
build agents, which we definitely rely on in the Git project itself. This
was addressed via the fifth patch, which had made it (in a slightly
different form) already into Git for Windows v2.35.2, and they are ready to
be applied to core Git, too.

The FAT32 patch came in later, and was released as part of Git for Windows
v2.37.0, so I also have confidence that it is stable and ready to be
integrated into core Git, too.

Changes since v1:

 * Restructured the patch series.
 * Instead of an environment variable to turn on debugging, we now always
   show the platform-dependent information together with the error message
   about the dubious ownership (iff it is shown, that is), based on an idea
   by Junio.
 * Rebased onto gc/bare-repo-discovery to avoid a merge conflict.

Johannes Schindelin (5):
  setup: fix some formatting
  Prepare for more detailed "dubious ownership" messages
  mingw: provide details about unsafe directories' ownership
  mingw: be more informative when ownership check fails on FAT32
  mingw: handle a file owned by the Administrators group correctly

 compat/mingw.c    | 59 ++++++++++++++++++++++++++++++++++++++++++++++-
 compat/mingw.h    |  2 +-
 git-compat-util.h |  5 +++-
 setup.c           | 30 ++++++++++++++----------
 4 files changed, 81 insertions(+), 15 deletions(-)


base-commit: 776f184893d2861a729aa4b91d69931036e03e4b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1286%2Fdscho%2Fsafe.directory-and-windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1286/dscho/safe.directory-and-windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1286

Range-diff vs v1:

 -:  ----------- > 1:  301d94f18f5 setup: fix some formatting
 -:  ----------- > 2:  8cc45e4922a Prepare for more detailed "dubious ownership" messages
 1:  3480381b8b9 ! 3:  63494818105 Allow debugging unsafe directories' ownership
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    Allow debugging unsafe directories' ownership
     +    mingw: provide details about unsafe directories' ownership
      
          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`.
     +    Let's help with that by providing more detailed information.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     - ## Documentation/config/safe.txt ##
     -@@ Documentation/config/safe.txt: 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`.
     -
       ## compat/mingw.c ##
      @@
       #include "../git-compat-util.h"
     @@ compat/mingw.c
       #include <conio.h>
       #include <wchar.h>
       #include "../strbuf.h"
     -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
     +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
       		    IsValidSid(current_user_sid) &&
       		    EqualSid(sid, current_user_sid))
       			result = 1;
     -+		else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
     ++		else if (report) {
      +			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
      +
      +			if (ConvertSidToStringSidA(sid, &str1))
     @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
      +				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);
     ++			strbuf_addf(report,
     ++				    "'%s' is owned by:\n"
     ++				    "\t'%s'\nbut the current user is:\n"
     ++				    "\t'%s'\n", path, str1, str2);
      +			LocalFree(to_free1);
      +			LocalFree(to_free2);
      +		}
       	}
       
       	/*
     -
     - ## setup.c ##
     -@@ setup.c: 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(&quoted, 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;
 3:  dae03f1b204 ! 4:  7aaa6248dfe mingw: be more informative when ownership check fails on FAT32
     @@ Commit message
          any ownership information anyway, and the `GetNamedSecurityInfoW()` call
          pretends that everything is owned "by the world".
      
     -    Let's special-case that scenario and tell the user what's going on, at
     -    least when they set `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`.
     +    Let's special-case that scenario and tell the user what's going on.
      
          This addresses https://github.com/git-for-windows/git/issues/3886
      
     @@ compat/mingw.c: static PSID get_current_user_sid(void)
      +	return 0;
      +}
      +
     - 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];
     -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
     - 			 * okay, too.
     - 			 */
     +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
     + 		    IsValidSid(current_user_sid) &&
     + 		    EqualSid(sid, current_user_sid))
       			result = 1;
     --		else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
     -+		else if (IsWellKnownSid(sid, WinWorldSid) &&
     -+			 git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0) &&
     +-		else if (report) {
     ++		else if (report &&
     ++			 IsWellKnownSid(sid, WinWorldSid) &&
      +			 !acls_supported(path)) {
      +			/*
      +			 * On FAT32 volumes, ownership is not actually recorded.
      +			 */
     -+			warning("'%s' is on a file system that does not record ownership", path);
     -+		} else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
     ++			strbuf_addf(report, "'%s' is on a file system that does"
     ++				    "not record ownership\n", path);
     ++		} else if (report) {
       			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
       
       			if (ConvertSidToStringSidA(sid, &str1))
 2:  be06d711a13 ! 5:  fbfaff2ec21 mingw: handle a file owned by the Administrators group correctly
     @@ Commit message
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## compat/mingw.c ##
     -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
     +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
       	else if (sid && IsValidSid(sid)) {
       		/* Now, verify that the SID matches the current user's */
       		static PSID current_user_sid;
     @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
       
       		if (!current_user_sid)
       			current_user_sid = get_current_user_sid();
     -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
     +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
       		    IsValidSid(current_user_sid) &&
       		    EqualSid(sid, current_user_sid))
       			result = 1;
     @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
      +			 * okay, too.
      +			 */
      +			result = 1;
     - 		else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
     - 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
     - 
     + 		else if (report &&
     + 			 IsWellKnownSid(sid, WinWorldSid) &&
     + 			 !acls_supported(path)) {

-- 
gitgitgadget