[PATCH v6 5/6] robocopy: Add basic copy logic and error handling
Zebediah Figura (she/her)
zfigura at codeweavers.com
Fri Oct 8 01:04:03 CDT 2021
On 9/21/21 09:54, Florian Eder wrote:
> 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 <others.meder at gmail.com>
> ---
> v6: Remove wine_todo from the additional syntax tests, as these tests pass now
> ---
> programs/robocopy/Makefile.in | 2 +-
> programs/robocopy/main.c | 177 ++++++++++++++++++++++++++++-
> programs/robocopy/robocopy.h | 17 +++
> programs/robocopy/robocopy.rc | 6 +
> programs/robocopy/tests/robocopy.c | 16 +--
> 5 files changed, 207 insertions(+), 11 deletions(-)
The biggest problem I see with this patch is that it implements
recursive copy, but the program by default isn't recursive, as the tests
from 6/6 show. It's a bit awkward to implement behaviour controlled by a
flag without implementing the flag, especially when recursiveness takes
*more* work.
I'd recommend starting with non-recursive tests, and implementing
non-recursive behaviour, and then adding recursive tests and recursive
behaviour in a separate patch.
> diff --git a/programs/robocopy/Makefile.in b/programs/robocopy/Makefile.in
> index 0f4f5c76119..1c640599b4b 100644
> --- a/programs/robocopy/Makefile.in
> +++ b/programs/robocopy/Makefile.in
> @@ -1,5 +1,5 @@
> MODULE = robocopy.exe
> -IMPORTS = kernelbase
> +IMPORTS = kernelbase shlwapi
>
> EXTRADLLFLAGS = -mconsole -municode -mno-cygwin
>
> diff --git a/programs/robocopy/main.c b/programs/robocopy/main.c
> index 6781fc17504..05546715718 100644
> --- a/programs/robocopy/main.c
> +++ b/programs/robocopy/main.c
> @@ -23,6 +23,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(robocopy);
> #include <windows.h>
> #include <stdlib.h>
> #include <pathcch.h>
> +#include <shlwapi.h>
> +#include <wine/list.h>
> #include "robocopy.h"
>
> struct robocopy_options options;
> @@ -71,6 +73,21 @@ static void output_message(const WCHAR *format_string, ...)
> LocalFree(string);
> }
>
> +static void output_error(UINT format_string_id, HRESULT error_code, WCHAR* path)
Inconsistent asterisk here.
> +{
> + WCHAR *error_string, error_code_long[64], error_code_short[64];
> +
> + FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS | FORMAT_MESSAGE_ALLOCATE_BUFFER,
> + NULL, error_code, 0, (LPWSTR)&error_string, 0, NULL);
Please try to avoid LP* typedefs in new code.
This line continuation is also weirdly spaced; I'd expect to align with
the previous line or be a constant 4 or 8 bytes.
> +
> + swprintf(error_code_long, ARRAY_SIZE(error_code_long), L"0x%08x", error_code);
> + swprintf(error_code_short, ARRAY_SIZE(error_code_short), L"%u", error_code);
Printing out the error number doesn't strike me as useful, and printing
it out in two bases definitely seems excessive.
> + output_message(format_string(format_string_id), L"", error_code_short, error_code_long, path, error_string);
> +
> + LocalFree(error_string);
> +}
> +
> static WCHAR *get_absolute_path(const WCHAR *path)
> {
> DWORD size;
> @@ -125,6 +142,146 @@ static void parse_arguments(int argc, WCHAR *argv[])
> }
> }
>
> +static BOOL path_in_array(WCHAR *name, struct path_array *names)
> +{
> + 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));
> + /* ignore the "\\?\" prefix, so that those backslashes are not matched */
I think this comment isn't relevant anymore?
> + pointer = wcschr(path, L'\\');
> + while (pointer != NULL)
> + {
> + if (!lstrcpynW(current_folder, path, pointer - path + 2)) return FALSE;
> + /* try to create the folder, ignoring any failure due to ERROR_ALREADY_EXISTS */
The code below tells me as much ;-)
> + if (!CreateDirectoryW(current_folder, NULL))
> + {
> + if (GetLastError() != ERROR_ALREADY_EXISTS)
> + {
> + output_error(STRING_ERROR_WRITE_DIRECTORY, GetLastError(), current_folder);
> + return FALSE;
> + }
> + }
> + else
> + output_message(format_string(STRING_CREATE_DIRECTORY), current_folder);
> + pointer = wcschr(pointer + 1, L'\\');
> + }
> + return TRUE;
> +}
> +
> +static void get_file_paths_in_folder(WCHAR *directory_path, struct list *paths)
> +{
Maybe "root" instead of "directory_path" would make things a bit clearer?
> + HANDLE temp_handle;
> + struct path *new_path, *current_path;
> + WIN32_FIND_DATAW entry_data;
> + WCHAR *parent_absolute_path, *current_relative_path, *current_absolute_path, *current_search_path;
> +
> + list_init(paths);
> +
> + /* initialize list with a empty relative path */
> + new_path = calloc(1, sizeof(struct path));
> + new_path->name = calloc(2, sizeof(WCHAR));
> + list_add_tail(paths, &new_path->entry);
This feels weird. Perhaps it would be better to move the
FindFirst/FindNext loop into a helper, and then call that for "." before
calling it for enumerated children?
> +
> + LIST_FOR_EACH_ENTRY(current_path, paths, struct path, entry)
> + {
> + /* append relative path to the (prefix) directory path */
> + PathAllocCombine(directory_path, current_path->name,
> + PATHCCH_ALLOW_LONG_PATHS | PATHCCH_FORCE_ENABLE_LONG_NAME_PROCESS,
> + &parent_absolute_path);
> +
> + /* append * to recieve every file / subdirectory in this directory */
> + PathAllocCombine(parent_absolute_path, L"*",
> + PATHCCH_ALLOW_LONG_PATHS | PATHCCH_FORCE_ENABLE_LONG_NAME_PROCESS,
> + ¤t_search_path);
This gets called frequently enough, and spans enough lines per call,
that I'd even suggest adding a helper to shorten it.
> + /* walk through all files / directories in this directory */
> + temp_handle = FindFirstFileExW(current_search_path, FindExInfoStandard, &entry_data, FindExSearchNameMatch, NULL, 0);
> + if (temp_handle != INVALID_HANDLE_VALUE)
> + {
> + do
> + {
> + /* Ignore . and .. entries */
> + if (!wcscmp(L".", entry_data.cFileName) || !wcscmp(L"..", entry_data.cFileName)) continue;
> +
> + PathAllocCombine(current_path->name, entry_data.cFileName,
> + PATHCCH_ALLOW_LONG_PATHS | PATHCCH_FORCE_ENABLE_LONG_NAME_PROCESS,
> + ¤t_relative_path);
> + PathAllocCombine(directory_path, current_relative_path,
> + PATHCCH_ALLOW_LONG_PATHS | PATHCCH_FORCE_ENABLE_LONG_NAME_PROCESS,
> + ¤t_absolute_path);
> +
> + /* If this entry is a matching file or empty directory, add it to the list of results */
I feel like all of the comments in this function are redundant.
> + if ((!PathIsDirectoryW(current_absolute_path) && path_in_array(entry_data.cFileName, options.files)) ||
> + (PathIsDirectoryW(current_absolute_path)))
This could be simplified to "PathIsDirectory || path_in_array(...)".
> + {
> + new_path = calloc(1, sizeof(struct path));
> + new_path->name = wcsdup(current_relative_path);
> + list_add_tail(paths, &new_path->entry);
> + }
> + }
> + while (FindNextFileW(temp_handle, &entry_data) != 0);
FindNextFileW() returns BOOL; comparing it to 0 strikes me as a bit weird.
> + }
> + }
> +}
> +
> +static BOOL perform_copy(void)
> +{
> + struct list paths_source;
> + struct path *current_path;
> + WCHAR *current_absolute_path, *target_path;
> +
> + list_init(&paths_source);
> +
> + if (!PathIsDirectoryW(options.source))
> + {
> + output_error(STRING_ERROR_READ_DIRECTORY, ERROR_FILE_NOT_FOUND, options.source);
> + return FALSE;
> + }
> +
> + create_directory_path(options.destination);
> +
> + /* get files in the source folder */
> + get_file_paths_in_folder(options.source, &paths_source);
> +
> + /* walk through files in the source folder */
> + LIST_FOR_EACH_ENTRY(current_path, &paths_source, struct path, entry)
> + {
> + PathAllocCombine(options.source, current_path->name,
> + PATHCCH_ALLOW_LONG_PATHS | PATHCCH_FORCE_ENABLE_LONG_NAME_PROCESS,
> + ¤t_absolute_path);
> +
> + /* append the relative source path to the destination to get the target path */
> + PathAllocCombine(options.destination, current_path->name,
> + PATHCCH_ALLOW_LONG_PATHS | PATHCCH_FORCE_ENABLE_LONG_NAME_PROCESS,
> + &target_path);
> +
> + if (PathIsDirectoryW(current_absolute_path))
> + {
> + if (!create_directory_path(target_path))
> + output_error(STRING_ERROR_WRITE_DIRECTORY, GetLastError(), target_path);
> + }
> + else
> + {
> + create_directory_path(target_path);
> + if (!CopyFileW(current_absolute_path, target_path, FALSE))
> + output_error(STRING_ERROR_WRITE_FILE, GetLastError(), target_path);
> + else
> + {
> + output_message(format_string(STRING_CREATE_FILE), target_path);
> + }
These braces are redundant, which bothers me mostly because this block
is inconsistent.
> + }
> + }
> + return TRUE;
> +}
> +
> static void print_header(void)
> {
> UINT i;
> @@ -146,8 +303,24 @@ int __cdecl wmain(int argc, WCHAR *argv[])
> {
> parse_arguments(argc, argv);
>
> + /* If no file filters are set, set *.* to include all files */
> + if (options.files->size == 0)
> + {
> + options.files->array[options.files->size] = wcsdup(L"*.*");
> + options.files->size++;
> + }
> +
> print_header();
>
> - WINE_FIXME("robocopy stub");
> - return 1;
> + /* Break if Source or Destination not set */
> + if (!options.destination || !options.source)
> + {
> + output_message(format_string(STRING_MISSING_DESTINATION_OR_SOURCE));
> + return ROBOCOPY_ERROR_NO_FILES_COPIED;
> + }
> +
> + if (!perform_copy())
> + return ROBOCOPY_ERROR_NO_FILES_COPIED;
> +
> + return ROBOCOPY_NO_ERROR_FILES_COPIED;
I did a double take when reading these two lines. How about "SUCCESS"
instead of "NO_ERROR"? Although it's not clear why "ROBOCOPY_SUCCESS"
and "ROBOCOPY_ERROR" aren't enough by themselves; are there other error
codes to worry about?
More information about the wine-devel
mailing list