[PATCH v6 3/6] robocopy: Add source / destination / file argument parser

[Zebediah Figura (she/her)]

On 9/21/21 09:54, Florian Eder wrote:
> Parses (relative / absolute) path arguments as source, destination
> and files to include
> 
> Signed-off-by: Florian Eder <others.meder at gmail.com>
> ---
> v6: Identical to patch in v5
> ---
>   programs/robocopy/Makefile.in |  1 +
>   programs/robocopy/main.c      | 61 +++++++++++++++++++++++++++++++++++
>   programs/robocopy/robocopy.h  | 33 +++++++++++++++++++
>   3 files changed, 95 insertions(+)
>   create mode 100644 programs/robocopy/robocopy.h
> 

Although splitting is nice, This patch doesn't exactly do anything, 
which makes it hard to judge. (For one thing, why are we transforming 
everything to an absolute path? I guess 4/6 is the reason, although I 
kind of wonder if 4/6 should even exist.)

I would suggest trying to reorder patches as follows:

(1) stub
(2) syntax tests
(3) tests for basic functionality
(4) argument parsing
(5) basic functionality
(6) print console output?

And then it might even be reasonable to merge patches 4 and 5.

> diff --git a/programs/robocopy/Makefile.in b/programs/robocopy/Makefile.in
> index 5a48be3725b..0f4f5c76119 100644
> --- a/programs/robocopy/Makefile.in
> +++ b/programs/robocopy/Makefile.in
> @@ -1,4 +1,5 @@
>   MODULE    = robocopy.exe
> +IMPORTS   = kernelbase
>   
>   EXTRADLLFLAGS = -mconsole -municode -mno-cygwin
>   
> diff --git a/programs/robocopy/main.c b/programs/robocopy/main.c
> index bbda15f573a..e1800fbd74e 100644
> --- a/programs/robocopy/main.c
> +++ b/programs/robocopy/main.c
> @@ -21,9 +21,70 @@ 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 WCHAR *get_absolute_path(const WCHAR *path)
> +{
> +    DWORD size;
> +    WCHAR *absolute_path;
> +
> +    /* allocate absolute path + potential backslash + null WCHAR */

This is the sort of comment that is described not only by the function 
name but by reading its code.

> +    size = GetFullPathNameW(path, 0, NULL, NULL) + 2;
> +    absolute_path = calloc(size, sizeof(WCHAR));
> +    GetFullPathNameW(path, size, absolute_path, NULL);
> +    PathCchAddBackslashEx(absolute_path, size, 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
> +         */

Should we have tests for these? It gets a bit awkward because Wine 
interprets such paths (rightly) as Unix paths, but they could still be 
useful to demonstrate the Windows behaviour.

> +        if ((argv[i][0] == L'/') && (wcschr(argv[i] + 1, L'/') == NULL))

Nitpick, but the L prefix is unnecessary. There's a few other such cases 
in these patches.

I also find "!wcschr" more idiomatic than "== NULL", although others 
seem to disagree.

> +            WINE_FIXME("encountered an unknown robocopy flag: %s\n", debugstr_w(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
> +            */

This is also kind of a redundant comment; the code below tells us as much.

> +            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] = wcsdup(argv[i]);
> +                options.files->size++;
> +            }
> +        }
> +    }
> +}
>   
>   int __cdecl wmain(int argc, WCHAR *argv[])
>   {
> +    parse_arguments(argc, argv);
> +
>       WINE_FIXME("robocopy stub");
>       return 1;
>   }
> diff --git a/programs/robocopy/robocopy.h b/programs/robocopy/robocopy.h
> new file mode 100644
> index 00000000000..f62eb92c663
> --- /dev/null
> +++ b/programs/robocopy/robocopy.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright 2021 Florian Eder
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + */
> +
> +#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;
> +};
> 

Do we gain a lot by having a separate header file?