Re: [PATCH v5 1/1] Adds domain/username to error message
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 4, 2024, 20:09 UTC
- Message-ID
- <xmqqbka07te6.fsf@gitster.g>
- In-Reply-To
- <20240104192202.2124-2-soekkle@freenet.de>
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".
Show 15 quoted lines
> 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.
Show 105 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' (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);
> }
> }