Re: [PATCH v3 1/1] Replace SID with domain/username

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 2, 2024, 17:43 UTC
Message-ID: <xmqqsf3feilq.fsf@gitster.g>
In-Reply-To: <20231231091245.2853-2-soekkle@freenet.de>

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

Show 16 quoted lines

> Replace SID with domain/username in error message, if owner of repository
> and user are not equal on windows systems.
>
> Old 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 massage:

"massage"???

Show 10 quoted lines

> '''
> 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'
> but the current user is:
>         'DESKTOP-L78JVA6/test'
> To add an exception for this directory, call:
>
>         git config --global --add safe.directory C:/Users/test/source/repos/git
> '''

The same "does this lose information?" comment applies to this one as well, as the fundamental approach is unchanged.

Show 17 quoted lines

> +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);
> +	*(*str + len_domain) = '/';
> +	if (translate_sid_to_user == FALSE) {
> +		FREE_AND_NULL(*str);
> +	}

Is this "FREE_AND_NULL" about clearing after you see an error? If so, shouldn't "overwrite the byte after the domain part with a slash" be done only when you have no error, i.e., perhaps

	translate_sid_to_user = LookupAccountSidA(...);
	if (!translate_sid_to_user)
		FREE_AND_NULL(*str);
	else
        	(*str)[len_domain] = '/';

or something along the line?

> +	return translate_sid_to_user;
> +}