Re: [PATCH v6 1/1] mingw: give more details about unsafe directory's ownership

[Johannes Sixt <j6t@kdbg.org>]

Am 06.01.24 um 12:29 schrieb Sören Krecker:
> Add domain/username in error message, if owner sid of repository and
> user sid are not equal on windows systems.
> 
> Old 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 error message:
> '''
> 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
> '''

I am not a fan of putting everything and the kitchen sink inside quotes.
In particular, the single-quotes around the user names are unnecessary,
IMO. Would you mind dropping them? (I do not mean to remove the quotes
around the path names because you do not touch this part of the code.)

> 
> 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*/

This comment line doesn't follow our style. Please either fix that (add
blanks after /* and before */ or (my preference) remove it altogether;
the code is clear without it.

> +	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)

We prefer to write this condition as

	if (!translate_sid_to_user)

> +		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;

This line grew a bit long now. Maybe break it to have lines not exceed
80 characters? While you do so, please insert blanks around = signs.

>  
> -			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);
>  		}
>  	}
>  

Aside from this, the patch works well for me. It is a real usability
improvement. Thank you for working on it.

-- Hannes