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

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

[Johannes Schindelin via GitGitGadget]

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 first two patches, which had made it already into Git
for Windows v2.35.2, and they are ready to be applied to core Git, too.

The third 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.

Johannes Schindelin (3):
  Allow debugging unsafe directories' ownership
  mingw: handle a file owned by the Administrators group correctly
  mingw: be more informative when ownership check fails on FAT32

 Documentation/config/safe.txt |  6 ++++
 compat/mingw.c                | 53 +++++++++++++++++++++++++++++++++++
 setup.c                       | 14 +++++++--
 3 files changed, 71 insertions(+), 2 deletions(-)


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

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

[Johannes Schindelin via GitGitGadget]

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`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config/safe.txt |  6 ++++++
 compat/mingw.c                | 21 +++++++++++++++++++++
 setup.c                       | 14 ++++++++++++--
 3 files changed, 39 insertions(+), 2 deletions(-)

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(&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;
-- 
gitgitgadget

[PATCH 2/3] mingw: handle a file owned by the Administrators group correctly

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When an Administrator creates a file or directory, the created
file/directory is owned not by the Administrator SID, but by the
_Administrators Group_ SID. The reason is that users with administrator
privileges usually run in unprivileged ("non-elevated") mode, and their
user SID does not change when running in elevated mode.

This is is relevant e.g. when running a GitHub workflow on a build
agent, which runs in elevated mode: cloning a Git repository in a script
step will cause the worktree to be owned by the Administrators Group
SID, for example.

Let's handle this case as following: if the current user is an
administrator, Git should consider a worktree owned by the
Administrators Group as if it were owned by said user.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 912444fb3ab..e0e020ee574 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2669,6 +2669,7 @@ int is_path_owned_by_current_sid(const char *path)
 	else if (sid && IsValidSid(sid)) {
 		/* Now, verify that the SID matches the current user's */
 		static PSID current_user_sid;
+		BOOL is_member;
 
 		if (!current_user_sid)
 			current_user_sid = get_current_user_sid();
@@ -2677,6 +2678,15 @@ int is_path_owned_by_current_sid(const char *path)
 		    IsValidSid(current_user_sid) &&
 		    EqualSid(sid, current_user_sid))
 			result = 1;
+		else if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) &&
+			 CheckTokenMembership(NULL, sid, &is_member) &&
+			 is_member)
+			/*
+			 * If owned by the Administrators group, and the
+			 * current user is an administrator, we consider that
+			 * okay, too.
+			 */
+			result = 1;
 		else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
 
-- 
gitgitgadget

[PATCH 3/3] mingw: be more informative when ownership check fails on FAT32

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The FAT file system has no concept of ACLs. Therefore, it cannot store
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`.

This addresses https://github.com/git-for-windows/git/issues/3886

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index e0e020ee574..4144d6247bd 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2630,6 +2630,21 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static int acls_supported(const char *path)
+{
+	size_t offset = offset_1st_component(path);
+	WCHAR wroot[MAX_PATH];
+	DWORD file_system_flags;
+
+	if (offset &&
+	    xutftowcsn(wroot, path, MAX_PATH, offset) > 0 &&
+	    GetVolumeInformationW(wroot, NULL, 0, NULL, NULL,
+				  &file_system_flags, NULL, 0))
+		return !!(file_system_flags & FILE_PERSISTENT_ACLS);
+
+	return 0;
+}
+
 int is_path_owned_by_current_sid(const char *path)
 {
 	WCHAR wpath[MAX_PATH];
@@ -2687,7 +2702,14 @@ 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)) {
+		else if (IsWellKnownSid(sid, WinWorldSid) &&
+			 git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0) &&
+			 !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)) {
 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
 
 			if (ConvertSidToStringSidA(sid, &str1))
-- 
gitgitgadget

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

[Junio C Hamano]

"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.

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

[Junio C Hamano]

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

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

[Johannes Schindelin]

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
>

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

[Johannes Schindelin via GitGitGadget]

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

[PATCH v2 1/5] setup: fix some formatting

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In preparation for touching code that was introduced in 3b0bf2704980
(setup: tighten ownership checks post CVE-2022-24765, 2022-05-10) and
that was formatted differently than preferred in the Git project, fix
the indentation before actually modifying the code.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 8c683e92b62..2f7b3e598f8 100644
--- a/setup.c
+++ b/setup.c
@@ -1142,7 +1142,7 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
  * added, for bare ones their git directory.
  */
 static int ensure_valid_ownership(const char *gitfile,
-				const char *worktree, const char *gitdir)
+				  const char *worktree, const char *gitdir)
 {
 	struct safe_directory_data data = {
 		.path = worktree ? worktree : gitdir
@@ -1316,10 +1316,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		strbuf_setlen(dir, offset);
 		if (gitdirenv) {
 			enum discovery_result ret;
+			const char *gitdir_candidate =
+				gitdir_path ? gitdir_path : gitdirenv;
 
-			if (ensure_valid_ownership(gitfile,
-						 dir->buf,
-				 (gitdir_path ? gitdir_path : gitdirenv))) {
+			if (ensure_valid_ownership(gitfile, dir->buf,
+						   gitdir_candidate)) {
 				strbuf_addstr(gitdir, gitdirenv);
 				ret = GIT_DIR_DISCOVERED;
 			} else
-- 
gitgitgadget

[PATCH v2 2/5] Prepare for more detailed "dubious ownership" messages

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When verifying the ownership of the Git directory, we sometimes would
like to say a bit more about it, e.g. when using a platform-dependent
code path (think: Windows and the permission model that is so different
from Unix'), but only when it is a appropriate to actually say
something.

To allow for that, collect that information and hand it back to the
caller (whose responsibility it is to show it or not).

Note: We do not actually fill in any platform-dependent information yet,
this commit just adds the infrastructure to be able to do so.

Based-on-an-idea-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c    |  2 +-
 compat/mingw.h    |  2 +-
 git-compat-util.h |  5 ++++-
 setup.c           | 25 +++++++++++++++----------
 4 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 2607de93af5..f12b7df16d9 100644
--- a/compat/mingw.c
+++ b/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 a/compat/mingw.h b/compat/mingw.h
index a74da68f313..209cf7cebad 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -463,7 +463,7 @@ char *mingw_query_user_email(void);
  * Verifies that the specified path is owned by the user running the
  * current process.
  */
-int is_path_owned_by_current_sid(const char *path);
+int is_path_owned_by_current_sid(const char *path, struct strbuf *report);
 #define is_path_owned_by_current_user is_path_owned_by_current_sid
 
 /**
diff --git a/git-compat-util.h b/git-compat-util.h
index 58d7708296b..36a25ae252f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -23,6 +23,9 @@
 #include <crtdbg.h>
 #endif
 
+struct strbuf;
+
+
 #define _FILE_OFFSET_BITS 64
 
 
@@ -487,7 +490,7 @@ 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)
+static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)
 {
 	struct stat st;
 	uid_t euid;
diff --git a/setup.c b/setup.c
index 2f7b3e598f8..cefd5f63c46 100644
--- a/setup.c
+++ b/setup.c
@@ -1142,16 +1142,17 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
  * added, for bare ones their git directory.
  */
 static int ensure_valid_ownership(const char *gitfile,
-				  const char *worktree, const char *gitdir)
+				  const char *worktree, const char *gitdir,
+				  struct strbuf *report)
 {
 	struct safe_directory_data data = {
 		.path = worktree ? worktree : gitdir
 	};
 
 	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)))
 		return 1;
 
 	/*
@@ -1232,6 +1233,7 @@ enum discovery_result {
  */
 static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 							  struct strbuf *gitdir,
+							  struct strbuf *report,
 							  int die_on_error)
 {
 	const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
@@ -1320,7 +1322,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 				gitdir_path ? gitdir_path : gitdirenv;
 
 			if (ensure_valid_ownership(gitfile, dir->buf,
-						   gitdir_candidate)) {
+						   gitdir_candidate, report)) {
 				strbuf_addstr(gitdir, gitdirenv);
 				ret = GIT_DIR_DISCOVERED;
 			} else
@@ -1345,7 +1347,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		if (is_git_directory(dir->buf)) {
 			if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
 				return GIT_DIR_DISALLOWED_BARE;
-			if (!ensure_valid_ownership(NULL, NULL, dir->buf))
+			if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
 				return GIT_DIR_INVALID_OWNERSHIP;
 			strbuf_addstr(gitdir, ".");
 			return GIT_DIR_BARE;
@@ -1378,7 +1380,7 @@ int discover_git_directory(struct strbuf *commondir,
 		return -1;
 
 	cwd_len = dir.len;
-	if (setup_git_directory_gently_1(&dir, gitdir, 0) <= 0) {
+	if (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {
 		strbuf_release(&dir);
 		return -1;
 	}
@@ -1425,7 +1427,7 @@ int discover_git_directory(struct strbuf *commondir,
 const char *setup_git_directory_gently(int *nongit_ok)
 {
 	static struct strbuf cwd = STRBUF_INIT;
-	struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;
+	struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT, report = STRBUF_INIT;
 	const char *prefix = NULL;
 	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
 
@@ -1450,7 +1452,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		die_errno(_("Unable to read current working directory"));
 	strbuf_addbuf(&dir, &cwd);
 
-	switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {
+	switch (setup_git_directory_gently_1(&dir, &gitdir, &report, 1)) {
 	case GIT_DIR_EXPLICIT:
 		prefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);
 		break;
@@ -1482,12 +1484,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		if (!nongit_ok) {
 			struct strbuf quoted = STRBUF_INIT;
 
+			strbuf_complete(&report, '\n');
 			sq_quote_buf_pretty(&quoted, dir.buf);
 			die(_("detected dubious ownership in repository at '%s'\n"
+			      "%s"
 			      "To add an exception for this directory, call:\n"
 			      "\n"
 			      "\tgit config --global --add safe.directory %s"),
-			    dir.buf, quoted.buf);
+			    dir.buf, report.buf, quoted.buf);
 		}
 		*nongit_ok = 1;
 		break;
@@ -1574,6 +1578,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	strbuf_release(&dir);
 	strbuf_release(&gitdir);
+	strbuf_release(&report);
 	clear_repository_format(&repo_fmt);
 
 	return prefix;
-- 
gitgitgadget

[PATCH v2 3/5] mingw: provide details about unsafe directories' ownership

[Johannes Schindelin via GitGitGadget]

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 providing more detailed information.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index f12b7df16d9..2c09c5bffee 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"
@@ -2720,6 +2721,29 @@ 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 (report) {
+			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)";
+			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);
+		}
 	}
 
 	/*
-- 
gitgitgadget

[PATCH v2 4/5] mingw: be more informative when ownership check fails on FAT32

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The FAT file system has no concept of ACLs. Therefore, it cannot store
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.

This addresses https://github.com/git-for-windows/git/issues/3886

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 2c09c5bffee..22f960c7e34 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2674,6 +2674,21 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static int acls_supported(const char *path)
+{
+	size_t offset = offset_1st_component(path);
+	WCHAR wroot[MAX_PATH];
+	DWORD file_system_flags;
+
+	if (offset &&
+	    xutftowcsn(wroot, path, MAX_PATH, offset) > 0 &&
+	    GetVolumeInformationW(wroot, NULL, 0, NULL, NULL,
+				  &file_system_flags, NULL, 0))
+		return !!(file_system_flags & FILE_PERSISTENT_ACLS);
+
+	return 0;
+}
+
 int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 {
 	WCHAR wpath[MAX_PATH];
@@ -2721,7 +2736,15 @@ 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 (report) {
+		else if (report &&
+			 IsWellKnownSid(sid, WinWorldSid) &&
+			 !acls_supported(path)) {
+			/*
+			 * On FAT32 volumes, ownership is not actually recorded.
+			 */
+			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))
-- 
gitgitgadget

[PATCH v2 5/5] mingw: handle a file owned by the Administrators group correctly

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When an Administrator creates a file or directory, the created
file/directory is owned not by the Administrator SID, but by the
_Administrators Group_ SID. The reason is that users with administrator
privileges usually run in unprivileged ("non-elevated") mode, and their
user SID does not change when running in elevated mode.

This is is relevant e.g. when running a GitHub workflow on a build
agent, which runs in elevated mode: cloning a Git repository in a script
step will cause the worktree to be owned by the Administrators Group
SID, for example.

Let's handle this case as following: if the current user is an
administrator, Git should consider a worktree owned by the
Administrators Group as if it were owned by said user.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 22f960c7e34..7aa9318db72 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2728,6 +2728,7 @@ 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;
+		BOOL is_member;
 
 		if (!current_user_sid)
 			current_user_sid = get_current_user_sid();
@@ -2736,6 +2737,15 @@ 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 (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) &&
+			 CheckTokenMembership(NULL, sid, &is_member) &&
+			 is_member)
+			/*
+			 * If owned by the Administrators group, and the
+			 * current user is an administrator, we consider that
+			 * okay, too.
+			 */
+			result = 1;
 		else if (report &&
 			 IsWellKnownSid(sid, WinWorldSid) &&
 			 !acls_supported(path)) {
-- 
gitgitgadget

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 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.

I actually had to rebase it back so that we could merge it to
'maint' for further 2.37.x releases.  I'll refer to the original
patches in this thread when I merge the result to 'seen', of course,
to make sure the results do match.  It would have been slightly less
convenient if you did not do this rebase, but it would have allowed
me to have much better confidence in the result that may eventually
go to 'maint'.  After all, mistakes in resolving merge conflicts on
'seen' can be corrected before the topic hits 'next'.

Thanks.  I do not know about the API calls mingw.c part of these
patches make, but the overall structure looks sensible to me.

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

[Johannes Schindelin]

Hi Junio,

On Mon, 8 Aug 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > 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.
>
> I actually had to rebase it back so that we could merge it to
> 'maint' for further 2.37.x releases.

I appreciate the effort you put into it, even if it is not your
responsibility to take care of Git for Windows' releases.

The range-diff shows that you snuck in a commit message change that I
would have either not made at all (I think the original was fine) or would
have made differently (because Access Control Lists are not specific to
Windows, even if Windows is the most obvious example for this permission
model):

-- snip --
1:  301d94f18f5 ! 1:  d51e1dff980 setup: fix some formatting
    @@ Commit message
         the indentation before actually modifying the code.

         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>

      ## setup.c ##
     @@ setup.c: static int safe_directory_cb(const char *key, const char *value, void *d)
2:  8cc45e4922a ! 2:  17d3883fe9c Prepare for more detailed "dubious ownership" messages
    @@ Metadata
     Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>

      ## Commit message ##
    -    Prepare for more detailed "dubious ownership" messages
    +    setup: prepare for more detailed "dubious ownership" messages

         When verifying the ownership of the Git directory, we sometimes would
         like to say a bit more about it, e.g. when using a platform-dependent
    -    code path (think: Windows and the permission model that is so different
    +    code path (think: Windows has the permission model that is so different
         from Unix'), but only when it is a appropriate to actually say
         something.

    @@ Commit message

         Based-on-an-idea-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>

      ## compat/mingw.c ##
     @@ compat/mingw.c: static PSID get_current_user_sid(void)
    @@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf
      				ret = GIT_DIR_DISCOVERED;
      			} else
     @@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
    + 		}
    +
      		if (is_git_directory(dir->buf)) {
    - 			if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
    - 				return GIT_DIR_DISALLOWED_BARE;
     -			if (!ensure_valid_ownership(NULL, NULL, dir->buf))
     +			if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
      				return GIT_DIR_INVALID_OWNERSHIP;
3:  63494818105 ! 3:  e883e04b68b mingw: provide details about unsafe directories' ownership
    @@ Commit message
         Let's help with that by providing more detailed information.

         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>

      ## compat/mingw.c ##
     @@
4:  7aaa6248dfe ! 4:  7c83470e64e mingw: be more informative when ownership check fails on FAT32
    @@ Commit message
         This addresses https://github.com/git-for-windows/git/issues/3886

         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>

      ## compat/mingw.c ##
     @@ compat/mingw.c: static PSID get_current_user_sid(void)
5:  fbfaff2ec21 ! 5:  3f7207e2ea9 mingw: handle a file owned by the Administrators group correctly
    @@ Commit message
         Administrators Group as if it were owned by said user.

         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>

      ## compat/mingw.c ##
     @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
-- snap --

Retitling the commit message was okay, of course, using the `setup:`
prefix.

> I'll refer to the original patches in this thread when I merge the
> result to 'seen', of course, to make sure the results do match.  It
> would have been slightly less convenient if you did not do this rebase,
> but it would have allowed me to have much better confidence in the
> result that may eventually go to 'maint'.  After all, mistakes in
> resolving merge conflicts on 'seen' can be corrected before the topic
> hits 'next'.

Yes, mistakes can happen, and the more people work together the easier it
is to avoid or fix them.

> Thanks.  I do not know about the API calls mingw.c part of these
> patches make, but the overall structure looks sensible to me.

I do not think that anybody expected you to know about Win32 API calls ;-)

Ciao,
Dscho