From: "Zebediah Figura (she/her)" <zfigura@codeweavers.com>
Subject: Re: [PATCH v2 3/6] robocopy: Add source / destination / file argument parser
Message-Id: <a9fe3832-503f-8b86-9c3b-a89ba75663e1@codeweavers.com>
Date: Fri, 17 Sep 2021 00:38:01 -0500
In-Reply-To: <20210915215700.424685-3-others.meder@gmail.com>
References: <20210915215700.424685-1-others.meder@gmail.com> <20210915215700.424685-3-others.meder@gmail.com>

Just a couple more nitpicks here; sorry I missed them the first time...

On 9/15/21 16:56, Florian Eder wrote:
> diff --git a/programs/robocopy/main.c b/programs/robocopy/main.c
> index 00dacc8ab6c..e5400952ec5 100644
> --- a/programs/robocopy/main.c
> +++ b/programs/robocopy/main.c
> @@ -21,9 +21,83 @@ WINE_DEFAULT_DEBUG_CHANNEL(robocopy);
>   
>   #define WIN32_LEAN_AND_MEAN
>   #include <windows.h>
> +#include <stdlib.h>
> +#include <pathcch.h>
> +#include "robocopy.h"
> +
> +struct robocopy_options options;
> +
> +WCHAR *get_absolute_path(WCHAR *path)

Missing "static". The argument could also be const.

> +{
> +    DWORD size;
> +    WCHAR *absolute_path;
> +
> +    /* allocate absolute path + potential backslash + null WCHAR */
> +    size = GetFullPathNameW(path, 0, NULL, NULL) + 2;
> +    if (!wcsnicmp(path, L"\\\\?\\", 4))
> +    {
> +        /* already prefixed with \\?\ */
> +        absolute_path = calloc(size, sizeof(WCHAR));
> +        GetFullPathNameW(path, size, absolute_path, NULL);
> +        PathCchAddBackslashEx(absolute_path, size, NULL, NULL);
> +    }
> +    else
> +    {
> +        /* not prefixed with \\?\, we must add it in front of the path */
> +        absolute_path = calloc(size + 4, sizeof(WCHAR));
> +        wcscpy(absolute_path, L"\\\\?\\");
> +        GetFullPathNameW(path, size, &(absolute_path[4]), NULL);
> +        PathCchAddBackslashEx(absolute_path, size + 4, NULL, NULL);
> +    }
> +    return absolute_path;
> +}
> +
> +static void parse_arguments(int argc, WCHAR *argv[])
> +{
> +    int i;
> +
> +    memset(&options, 0, sizeof(options));
> +    options.files = calloc(1, offsetof(struct path_array, array[argc]));
> +
> +    for (i = 1; i < argc; i++)
> +    {
> +        /*
> +         * Robocopy switches contain one (and only one) backslash at the start
> +         * /xf => valid flag
> +         * /r:1 => valid flag
> +         * /r:1aö => valid flag
> +         * /r:1aö/ => not a valid flag, is interpreted as a filename
> +         */
> +        if ((argv[i][0] == L'/') && (wcschr(argv[i] + 1, L'/') == NULL))
> +            WINE_FIXME("encountered an unknown robocopy flag: %S\n", argv[i]);

I think %S makes sense in the tests, but in the code it's probably 
better to use debugstr_w(), so that we can clearly see non-ASCII characters.

> +        else
> +        {
> +            /*
> +            *(Probably) not a flag, we can parse it as the source / the destination / a filename
> +            * Priority: Source > Destination > (more than one) File
> +            */
> +            if (!options.source)
> +            {
> +                options.source = get_absolute_path(argv[i]);
> +            }
> +            else if (!options.destination)
> +            {
> +                options.destination = get_absolute_path(argv[i]);
> +            }
> +            else
> +            {
> +                options.files->array[options.files->size] = calloc(wcslen(argv[i]) + 1, sizeof(WCHAR));
> +                wcscpy(options.files->array[options.files->size], argv[i]);

This looks like wcsdup().

> +                options.files->size++;
> +            }
> +        }
> +    }
> +}
>   
>   int __cdecl wmain(int argc, WCHAR *argv[])
>   {
> +    parse_arguments(argc, argv);
> +
>       WINE_FIXME("robocopy stub");
>       return 0;
>   }