Re: [PATCH 03/41] robocopy: add source / destination / file argument parser

[Zebediah Figura]

On 9/6/21 9:54 AM, Florian Eder wrote:
> Parses (relative / absolute) path arguments as source, destination
> and files to include
> Signed-off-by: Florian Eder
> Splitted from the next patch to enhance readabilty a bit :-)
>
> programs/robocopy/Makefile.in |  2 +-
>   programs/robocopy/main.c      | 80 +++++++++++++++++++++++++++++++++++
>   programs/robocopy/robocopy.h  | 14 +++++-
>   3 files changed, 94 insertions(+), 2 deletions(-)
>
> diff --git a/programs/robocopy/Makefile.in b/programs/robocopy/Makefile.in
> index 5463edcb8b3..3f16d00c0a8 100644
> --- a/programs/robocopy/Makefile.in
> +++ b/programs/robocopy/Makefile.in
> @@ -1,5 +1,5 @@
>   MODULE    = robocopy.exe
> -IMPORTS   =
> +IMPORTS   = kernelbase
>
> EXTRADLLFLAGS = -mconsole -municode -mno-cygwin
>
> diff --git a/programs/robocopy/main.c b/programs/robocopy/main.c
> index a184235e488..8f8974b3528 100644
> --- a/programs/robocopy/main.c
> +++ b/programs/robocopy/main.c
> @@ -21,9 +21,89 @@ 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;
> -
>
> +static BOOL is_valid_robocopy_flag(WCHAR *string)

You could probably shorten this name to "is_valid_flag" or even 
"is_flag". For that matter, it seems not unreasonable to just open-code 
it where it's used.
> +{
> - /*
> -
> * 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 (string[0] != L'/') return FALSE;
> - if (wcschr(string + 1, L'/') != NULL) return FALSE;
> - return TRUE;
>
> +}
> -
>
> +WCHAR *get_absolute_path_with_trailing_backslash(WCHAR *path)

This is an awfully long name; it seems reasonable to me to just call it 
get_absolute_path().
> +{
> - 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
> - {
> -
>    absolute_path = calloc(size + 4, sizeof(WCHAR));
> -
>    wcscpy(absolute_path, L"\\\\?\\");

What's the purpose of this part?
> -
>    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 * sizeof(WCHAR*)));

This can be expressed as "offsetof(struct path_array, array[argc])".
> -
> - for (i = 1; i < argc; i++)
> - {
> -
>    if (is_valid_robocopy_flag(argv[i]))
> -
>        WINE_FIXME("encountered an unknown robocopy flag: %S\n", argv[i]);
> -
>    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_with_trailing_backslash(argv[i]);
> -
>        }
> -
>        else if (!options.destination)
> -
>        {
> -
>            options.destination = get_absolute_path_with_trailing_backslash(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]);
> -
>            options.files->size++;
> -
>        }
> -
>    }
> - }
>
> +}
>
> int __cdecl wmain(int argc, WCHAR *argv[])
>   {
> - parse_arguments(argc, argv);
> -
> WINE_FIXME("robocopy stub");
> return 0;
> }
>
> \ No newline at end of file
> diff --git a/programs/robocopy/robocopy.h b/programs/robocopy/robocopy.h
> index 33e84b82ea0..aa857cd8c35 100644
> --- a/programs/robocopy/robocopy.h
> +++ b/programs/robocopy/robocopy.h
> @@ -16,4 +16,16 @@
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>
> */
>
> -#define WIN32_LEAN_AND_MEAN
> \ No newline at end of file
> +#define WIN32_LEAN_AND_MEAN
> +#include <windows.h>
> -
>
> +struct path_array {
> - UINT size;
> - WCHAR *array[1];
>
> +};
> -
>
> +struct robocopy_options {
> - WCHAR *destination;
> - WCHAR *source;
> - struct path_array* files;
>
> +};

A couple style nitpicks here: inconsistent asterisk style, and 
inconsistent brace style.
> \ No newline at end of file