Re: [PATCH v7 3/4] robocopy: Add argument parser and basic copy logic

[Zebediah Figura]

On 10/16/21 6:13 AM, Florian Eder wrote:
> Parses path arguments as source, destination and files to include,
> reads all files in the source folder that match any of the files to include
> and copies them to the destination, creating necessary folders in the process
> Signed-off-by: Florian Eder
>
> v7: Merged patches 3 and 5, removed all messaging / error outputs and
> made copy function non-recursive for now, until the /S switch is implemented
>
> programs/robocopy/Makefile.in      |   1 +
>   programs/robocopy/main.c           | 196 ++++++++++++++++++++++++++++-
>   programs/robocopy/tests/robocopy.c |  38 +++---
>   3 files changed, 214 insertions(+), 21 deletions(-)
>
> diff --git a/programs/robocopy/Makefile.in b/programs/robocopy/Makefile.in
> index f5d9dff6e4d..d1f5db665df 100644
> --- a/programs/robocopy/Makefile.in
> +++ b/programs/robocopy/Makefile.in
> @@ -1,6 +1,7 @@
>   MODULE    = robocopy.exe
>
> EXTRADLLFLAGS = -mconsole -municode
> +IMPORTS   = kernelbase shlwapi
>
> C_SRCS = \
>    main.c
> diff --git a/programs/robocopy/main.c b/programs/robocopy/main.c
> index 320de52f2d5..6e19170712d 100644
> --- a/programs/robocopy/main.c
> +++ b/programs/robocopy/main.c
> @@ -21,9 +21,201 @@ WINE_DEFAULT_DEBUG_CHANNEL(robocopy);
>
> #define WIN32_LEAN_AND_MEAN
>   #include <windows.h>
> +#include <stdlib.h>
> +#include <pathcch.h>
> +#include <shlwapi.h>
> +#include <wine/list.h>
> -
>
> +struct path_array
> +{
> - UINT size;
> - WCHAR *array[1];
>
> +};
> -
>
> +struct path
> +{
> - struct list entry;
> - WCHAR *name;
>
> +};
> -
>
> +struct robocopy_options
> +{
> - WCHAR *destination;
> - WCHAR *source;
> - struct path_array *files;
>
> +};
> -
>
> +#define ROBOCOPY_SUCCESS_FILES_COPIED        1
> +#define ROBOCOPY_ERROR_NO_FILES_COPIED        16
> -
>
> +struct robocopy_options options;
> -
>
> +static WCHAR *get_absolute_path(const WCHAR *path)
> +{
> - DWORD size;
> - WCHAR *absolute_path;
> -
> - 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;
>
> +}

Do we need to convert paths to absolute before copying them?

(Do we even need to convert paths to absolute before writing output, and 
if so, should that happen in this patch?)
> -
>
> +static BOOL path_in_array(WCHAR *name, struct path_array *names)

Both of these arguments could be const. There's a few other cases below.
> +{
> - int i;
> - for (i = 0; i < names->size; i++)
> - {
> -
>    if (PathMatchSpecW(name, names->array[i])) return TRUE;
> - }
> - return FALSE;
>
> +}
> -
>
> +static BOOL create_directory_path(WCHAR *path)
> +{
> - WCHAR *pointer, *current_folder;
> - current_folder = calloc(wcslen(path) + 1, sizeof(WCHAR));
> - pointer = wcschr(path, L'\');
> - while (pointer != NULL)
> - {
> -
>    if (!lstrcpynW(current_folder, path, pointer - path + 2)) return FALSE;
> -
>    if (!CreateDirectoryW(current_folder, NULL) && GetLastError() != ERROR_ALREADY_EXISTS) return FALSE;
> -
>    pointer = wcschr(pointer + 1, L'\\');
> - }
> - return TRUE;
>
> +}
> -
>
> +static void append_matching_directory_content(struct list *paths, WCHAR *search_path, WCHAR *root, struct path_array *file_names)
> +{
> - WIN32_FIND_DATAW entry_data;
> - HANDLE handle;
> - WCHAR *current_absolute_path;
> - struct path *new_path;
> -
> - handle = FindFirstFileExW(search_path, FindExInfoStandard, &entry_data, FindExSearchNameMatch, NULL, 0);
> - if (handle != INVALID_HANDLE_VALUE)

You could return early here and save a level of indentation.
> - {
> -
>    do
> -
>    {
> -
>        if (!wcscmp(L".", entry_data.cFileName) || !wcscmp(L"..", entry_data.cFileName)) continue;
> -
> -
>        PathAllocCombine(root, entry_data.cFileName,
> -
>                         PATHCCH_ALLOW_LONG_PATHS | PATHCCH_FORCE_ENABLE_LONG_NAME_PROCESS,
> -
>                         &current_absolute_path);

Have you considered adding a helper for this PathAllocCombine() call? 
It's only one function, granted, but it's done many times here, and it 
does take up several lines.
> -
> -
>        if ((!PathIsDirectoryW(current_absolute_path) && path_in_array(entry_data.cFileName, file_names)) ||
> -
>            (PathIsDirectoryW(current_absolute_path)))

This could be simplified to "PathIsDirectory || path_in_array(...)".
> -
>        {
> -
>            new_path = calloc(1, sizeof(struct path));
> -
>            new_path->name = wcsdup(entry_data.cFileName);
> -
>            list_add_tail(paths, &new_path->entry);
> -
>        }
> -
>    }
> -
>    while (FindNextFileW(handle, &entry_data));
> - }
>
> +}