On Thu, Dec 28, 2023 at 8:29 AM Sören Krecker <soekkle@xxxxxxxxxx> wrote:
> From: soekkle <soekkle@xxxxxxxxxx>
>
> Replace SID with domain/username in erromessage, if owner of repository
> and user are not equal on windows systems.
>
> Signed-off-by: Sören Krecker <soekkle@xxxxxxxxxx>
> ---
I don't do Windows (anymore), thus I'm not qualified to comment on the
substance of this patch, so I'll just make some general, hopefully
helpful, observations.
Typo: "erromessage" should be "error message"
Your name in the "From:" header and Signed-off-by: should be the same.
Perhaps Widows folks can understand the purpose of this patch without
further explanation, but for other readers, it's not clear what
problem the patch is trying to solve. The commit message is a good
place to explain _why_ this change is desirable.
> diff --git a/compat/mingw.c b/compat/mingw.c
> @@ -2684,6 +2684,25 @@ static PSID get_current_user_sid(void)
> +BOOL user_sid_to_string(PSID sid, LPSTR* str)
In this codebase, '*' sticks to the variable name, not the type, so:
BOOL user_sid_to_string(PSID sid, LPSTR *str)
> +{
> + SID_NAME_USE peUse;
> + DWORD lenName = { 0 }, lenDomain = { 0 };
Looking through compat/mingw.c, it appears that (as with the rest of
the project), variable names tend to use underscores rather than
camel-case, so for consistency these might be better expressed as
"pe_use" (whatever that means), "name_len", and "domain_len".
I was curious about the `{ 0 }` initializer. It seems we have a mix of
both `{0}` and `{ 0 }` in the codebase, so what you have here is
likely fine.
> + LookupAccountSidA(NULL, sid, NULL, &lenName, NULL,
> + &lenDomain, &peUse); // returns only FALSE, because the string pointers are NULL
As with the rest of the project, compat/mingw.c still shuns "//"
comments. Use /*...*/ comments instead.
> + ALLOC_ARRAY((*str), (size_t)lenDomain + (size_t)lenName); // Alloc neded Space of the strings
Type: "neded" -> "needed"
(and "Space" -> "space")
> + BOOL retVal = LookupAccountSidA(NULL, sid, (*str) + lenDomain, &lenName,
> + *str,
> + &lenDomain, &peUse);
> + *(*str + lenDomain) = '/';
> + if (retVal == FALSE)
> + {
> + free(*str);
> + *str = NULL;
The FREE_AND_NULL() macro from git-compat-util.h is a good companion
to the ALLOC_ARRAY() macro used above, so freeing and nullifying could
be done in one line:
FREE_AND_NULL(*str);
> + }
> + return retVal;
> +}
Perhaps a variable name such as `ok` would convey more to the reader
than the generic `retVal`?
