{"thread":{"id":"60668","subject":"[PATCH 0/1] Replace SID with domain/username on Windows","startedAt":"2023-12-28T13:28:56Z","lastAt":"2023-12-28T19:27:55Z","messageCount":3,"participants":["Sören Krecker","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"486118","messageId":"20231228132844.4240-1-soekkle@freenet.de","threadId":"60668","inReplyTo":null,"subject":"[PATCH 0/1] Replace SID with domain/username on Windows","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2023-12-28T13:28:43Z","receivedAt":"2023-12-28T13:28:56Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"*** BLURB HERE ***\n\nsoekkle (1):\n Replace SID with domain/username\n\n compat/mingw.c | 27 +++++++++++++++++++++++----\n 1 file changed, 23 insertions(+), 4 deletions(-)\n\n\nbase-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa\n-- \n2.39.2\n\n"},{"id":"486119","messageId":"20231228132844.4240-2-soekkle@freenet.de","threadId":"60668","inReplyTo":"20231228132844.4240-1-soekkle@freenet.de","subject":"[PATCH 1/1] Replace SID with domain/username","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2023-12-28T13:28:44Z","receivedAt":"2023-12-28T13:28:59Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"From: soekkle \n\nReplace SID with domain/username in erromessage, if owner of repository\nand user are not equal on windows systems.\n\nSigned-off-by: Sören Krecker \n---\n compat/mingw.c | 27 +++++++++++++++++++++++----\n 1 file changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 42053c1f65..7318f0e20e 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2684,6 +2684,25 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n+BOOL user_sid_to_string(PSID sid, LPSTR* str)\n+{\n+\tSID_NAME_USE peUse;\n+\tDWORD lenName = { 0 }, lenDomain = { 0 };\n+\tLookupAccountSidA(NULL, sid, NULL, &lenName, NULL,\n+\t\t\t\t\t&lenDomain, &peUse); // returns only FALSE, because the string pointers are NULL\n+\tALLOC_ARRAY((*str), (size_t)lenDomain + (size_t)lenName); // Alloc neded Space of the strings\n+\tBOOL retVal = LookupAccountSidA(NULL, sid, (*str) + lenDomain, &lenName,\n+\t\t\t\t *str,\n+\t\t\t\t\t&lenDomain, &peUse);\n+\t*(*str + lenDomain) = '/';\n+\tif (retVal == FALSE)\n+\t{\n+\t\tfree(*str);\n+\t\t*str = NULL;\n+\t}\n+\treturn retVal;\n+}\n+\n static int acls_supported(const char *path)\n {\n \tsize_t offset = offset_1st_component(path);\n@@ -2767,7 +2786,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t} else if (report) {\n \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n \n-\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n+\t\t\tif (user_sid_to_string(sid, &str1))\n \t\t\t\tto_free1 = str1;\n \t\t\telse\n \t\t\t\tstr1 = \"(inconvertible)\";\n@@ -2776,7 +2795,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\t\tstr2 = \"(none)\";\n \t\t\telse if (!IsValidSid(current_user_sid))\n \t\t\t\tstr2 = \"(invalid)\";\n-\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n+\t\t\telse if (user_sid_to_string(current_user_sid, &str2))\n \t\t\t\tto_free2 = str2;\n \t\t\telse\n \t\t\t\tstr2 = \"(inconvertible)\";\n@@ -2784,8 +2803,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\t\t \"'%s' is owned by:\\n\"\n \t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n \t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n-\t\t\tLocalFree(to_free1);\n-\t\t\tLocalFree(to_free2);\n+\t\t\tfree(to_free1);\n+\t\t\tfree(to_free2);\n \t\t}\n \t}\n \n-- \n2.39.2\n\n"},{"id":"486136","messageId":"CAPig+cRK5PVBd6Lvztptk1td1S46rQ8KtA6S6+jTsuuCA6rnnw@mail.gmail.com","threadId":"60668","inReplyTo":"20231228132844.4240-2-soekkle@freenet.de","subject":"Re: [PATCH 1/1] Replace SID with domain/username","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-12-28T19:27:43Z","receivedAt":"2023-12-28T19:27:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 28, 2023 at 8:29 AM Sören Krecker wrote:\n> From: soekkle \n>\n> Replace SID with domain/username in erromessage, if owner of repository\n> and user are not equal on windows systems.\n>\n> Signed-off-by: Sören Krecker \n> ---\n\nI don't do Windows (anymore), thus I'm not qualified to comment on the\nsubstance of this patch, so I'll just make some general, hopefully\nhelpful, observations.\n\nTypo: \"erromessage\" should be \"error message\"\n\nYour name in the \"From:\" header and Signed-off-by: should be the same.\n\nPerhaps Widows folks can understand the purpose of this patch without\nfurther explanation, but for other readers, it's not clear what\nproblem the patch is trying to solve. The commit message is a good\nplace to explain _why_ this change is desirable.\n\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> @@ -2684,6 +2684,25 @@ static PSID get_current_user_sid(void)\n> +BOOL user_sid_to_string(PSID sid, LPSTR* str)\n\nIn this codebase, '*' sticks to the variable name, not the type, so:\n\n BOOL user_sid_to_string(PSID sid, LPSTR *str)\n\n> +{\n> + SID_NAME_USE peUse;\n> + DWORD lenName = { 0 }, lenDomain = { 0 };\n\nLooking through compat/mingw.c, it appears that (as with the rest of\nthe project), variable names tend to use underscores rather than\ncamel-case, so for consistency these might be better expressed as\n\"pe_use\" (whatever that means), \"name_len\", and \"domain_len\".\n\nI was curious about the `{ 0 }` initializer. It seems we have a mix of\nboth `{0}` and `{ 0 }` in the codebase, so what you have here is\nlikely fine.\n\n> + LookupAccountSidA(NULL, sid, NULL, &lenName, NULL,\n> + &lenDomain, &peUse); // returns only FALSE, because the string pointers are NULL\n\nAs with the rest of the project, compat/mingw.c still shuns \"//\"\ncomments. Use /*...*/ comments instead.\n\n> + ALLOC_ARRAY((*str), (size_t)lenDomain + (size_t)lenName); // Alloc neded Space of the strings\n\nType: \"neded\" -> \"needed\"\n\n(and \"Space\" -> \"space\")\n\n> + BOOL retVal = LookupAccountSidA(NULL, sid, (*str) + lenDomain, &lenName,\n> + *str,\n> + &lenDomain, &peUse);\n> + *(*str + lenDomain) = '/';\n> + if (retVal == FALSE)\n> + {\n> + free(*str);\n> + *str = NULL;\n\nThe FREE_AND_NULL() macro from git-compat-util.h is a good companion\nto the ALLOC_ARRAY() macro used above, so freeing and nullifying could\nbe done in one line:\n\n FREE_AND_NULL(*str);\n\n> + }\n> + return retVal;\n> +}\n\nPerhaps a variable name such as `ok` would convey more to the reader\nthan the generic `retVal`?\n"}]}