Re: [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
> 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?