Re: [PATCH v8 2/3] robocopy: Add argument parser and basic copy logic

[Zebediah Figura (she/her)]

On 10/31/21 15:34, 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
>
> v8: Made some arguments constant, added helper function to replace / remove duplicates of very long flags for PathAllocCombine and
> simplified logic in append_matching_directory_content
>
> This patch still converts both source and destination path to absolute paths, as doing otherwise would break the already existing
> support for paths > MAX_PATH. It would also not really save many LoCs / much logic, as it would still be required to make sure the
> paths both end with a backslash, to simplify the copy logic (as we then can just append the relative paths).

Right, thanks, that makes sense.

Just a few more nitpicks below...
> +struct robocopy_options options;

Missing "static".
> +static void combine_path(const WCHAR *path1, const WCHAR *path2, WCHAR **out)
> +{
> - PathAllocCombine(path1, path2,
> -
>                 PATHCCH_ALLOW_LONG_PATHS | PATHCCH_FORCE_ENABLE_LONG_NAME_PROCESS,
> -
>                 out);
>
> +}

You might consider making the combined path the return value. Food for 
thought.
> -
>
> +static void append_matching_directory_content(struct list *paths, const WCHAR *search_path, const WCHAR *root, struct path_array *file_names)

"file_names" could be const. This line is also kind of long—we don't 
really have a hard limit, but 120 characters is pushing it.
> +{
> - 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) return;
> - do
> - {
> -
>    if (!wcscmp(L".", entry_data.cFileName) || !wcscmp(L"..", entry_data.cFileName)) continue;
> -
> -
>    combine_path(root, entry_data.cFileName, &current_absolute_path);
> -
> -
>    if (PathIsDirectoryW(current_absolute_path) || path_in_array(entry_data.cFileName, file_names))
> -
>    {
> -
>        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));
>
> +}
> -
>
> +static void get_file_paths_in_folder(const WCHAR *root, struct list *paths)
> +{
> - struct path *new_path;
> - WCHAR *current_search_path;
> -
> - list_init(paths);

This list is initialized both here and in the caller. Not sure which 
makes more sense given following patches, but once is enough ;-)
> - new_path = calloc(1, sizeof(struct path));
> - new_path->name = calloc(2, sizeof(WCHAR));
> - list_add_tail(paths, &new_path->entry);
> -
> - combine_path(root, L"*", &current_search_path);
> - append_matching_directory_content(paths, current_search_path, root, options.files);
>
> +}