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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 28, 2023, 19:27 UTC
Message-ID: <CAPig+cRK5PVBd6Lvztptk1td1S46rQ8KtA6S6+jTsuuCA6rnnw@mail.gmail.com>
In-Reply-To: <20231228132844.4240-2-soekkle@freenet.de>

On Thu, Dec 28, 2023 at 8:29 AM Sören Krecker <soekkle@freenet.de> wrote:

Show 7 quoted lines

> From: soekkle <soekkle@freenet.de>
>
> 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@freenet.de>
> ---

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

Show 8 quoted lines

> +       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`?