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@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;}}elseoutput_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?