Re: [PATCH v5 1/1] Adds domain/username to error message

[Junio C Hamano <gitster@pobox.com>]

Sören Krecker <soekkle@freenet.de> writes:

> Subject: Re: [PATCH v5 1/1] Adds domain/username to error message

Looking at past commits that worked on the area this patch touches,
namely, 7c83470e (mingw: be more informative when ownership check
fails on FAT32, 2022-08-08) and e883e04b (mingw: provide details
about unsafe directories' ownership, 2022-08-08), I would retitle
the commit perhaps like so:

    Subject: [PATCH v5] mingw: give more details about unsafe directory's ownership

if I were doing this patch.

> Adds domain/username in error message, if owner sid of repository and

"Adds" -> "Add".

> user sid are not equal on windows systems.
>
> Old Prompted error message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 	'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
> 	'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
>
> 	git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> New prompted error massage:

"massage" -> "message".

I probably would drop two "prompted" from the above, too, if I were
doing this patch.

Thanks for working on making this error message more readable.  I'll
queue it when I see an Ack from Dscho.





> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
>         'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
> but the current user is:
>         'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
> To add an exception for this directory, call:
>
>         git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>  compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 51 insertions(+), 13 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 42053c1f65..6240387205 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
>  	return result;
>  }
>  
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> +	SID_NAME_USE pe_use;
> +	DWORD len_user = 0, len_domain = 0;
> +	BOOL translate_sid_to_user;
> +
> +	/* returns only FALSE, because the string pointers are NULL*/
> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> +			  &pe_use); 
> +	/*Alloc needed space of the strings*/
> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> +	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> +				   *str, &len_domain, &pe_use);
> +	if (translate_sid_to_user == FALSE)
> +		FREE_AND_NULL(*str);
> +	else
> +		(*str)[len_domain] = '/';
> +	return translate_sid_to_user;
> +}
> +
>  static int acls_supported(const char *path)
>  {
>  	size_t offset = offset_1st_component(path);
> @@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  			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;
> +			LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;
>  
> -			if (ConvertSidToStringSidA(sid, &str1))
> +			if (user_sid_to_user_name(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;
> +			if (ConvertSidToStringSidA(sid, &str2))
> +				to_local_free2 = str2;
>  			else
>  				str2 = "(inconvertible)";
> +
> +			if (!current_user_sid) {
> +				str3 = "(none)";
> +				str4 = "(none)";
> +			}
> +			else if (!IsValidSid(current_user_sid)) {
> +				str3 = "(invalid)";
> +				str4 = "(invalid)";
> +			} else {
> +				if (user_sid_to_user_name(current_user_sid,
> +							  &str3))
> +					to_free3 = str3;
> +				else
> +					str3 = "(inconvertible)";
> +				if (ConvertSidToStringSidA(current_user_sid,
> +							   &str4))
> +					to_local_free4 = str4;
> +				else
> +					str4 = "(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);
> +				    "\t'%s' (%s)\nbut the current user is:\n"
> +				    "\t'%s' (%s)\n",
> +				    path, str1, str2, str3, str4);
> +			free(to_free1);
> +			LocalFree(to_local_free2);
> +			free(to_free3);
> +			LocalFree(to_local_free4);
>  		}
>  	}