Common/FileUtil: Migrate CopyDir() to a more clear interface.

[AdmiralCurtiss]

Cleaning up some of the nonsense. @shuffle2 please check this. I moved the equivalent() check into the error case of Copy() because std::filesystem::copy() is defined to fail instead of destroying files in this situation and it's pretty unlikely in the first place.

There's one behavior of Copy() that I don't quite like, which is that if the source is a file and the destination is a folder it copies the file into the folder instead of either failing or overwriting the folder, it kinda feels like a situation that could surprise. But that's just what std::filesystem::copy() does, so it's probably okay to keep as-is...?

[AdmiralCurtiss]

Oh, and I'd actually like to call CopyAny() just Copy() but that function already exists and only copies files, so we'd have to rename that first -- see #11595.

[shuffle2]

idk, the callers of lots of stuff into FileUtil could still use a big refactoring (ideally it should just be an exception-swallowing wrapper around std::filesystem, and we can expose the fs::path type, i think - eventually)

i was actually thinking you'd do the inverse - replace the places where CopyDir was called with destructive=true, because that's where the regression is, isn't it (and those could just be replaced with Rename, i think)? the logic for what's currently in this PR seems fine, but I don't think that regression is covered.

[AdmiralCurtiss]

Yes, those still need to be replaced with something (I think we need an explicit Move() function because Rename() only works if source and destination are on the same logical volume, which we've run into before, see 0efff51), but I ran out of time yesterday and figured I'd at least PR this part of it. This is done now.

[AdmiralCurtiss]

Okay, here's the rest.

[AdmiralCurtiss]

I'm not sure if this part is actually necessary -- I originally thought it would be if source and destination are on different file systems, but std::filesystem::rename() seems to handle that just fine by itself. So I dunno, maybe this is unnecessary?

[shuffle2]

practically, how can rename fail in such a way that the following code is useful? the failure cases listed on https://en.cppreference.com/w/cpp/filesystem/rename seem to be actual errors which the below wouldn't help with.

[AdmiralCurtiss]

Like I said, I'm not sure if the copy_file part is useful, but the directory iteration can definitely happen if you try to merge into an existing directory.

[AdmiralCurtiss]

Like, to be clear: You have /d1/a.txt and /d2/b.txt. You then try to fs::rename("/d1", "/d2"). This fails; /d2 is non-empty so the exception does not apply. But the expected behavior of a move from /d1 to /d2 is that all contents from /d1 end up in /d2 -- you want to end up with a /d2 containing both a.txt and b.txt -- so you have to iterate through the contents and move them one-by-one using the same algorithm.

[shuffle2]

Ah...well, sorry to be annoying, but maybe the name should be something like MoveWithOverwrite or something? Since it deviates from the posix behavior.

[AdmiralCurtiss]

Sure.

[shuffle2]

I've interpreted it as:
if new_p exists and is empty, it is deleted (on posix-respecting systems) or an error is returned.
if there was no error then the rename of old_p to new_p occurs, in such a way that external observers don't notice the delete of empty directory happened.

i.e. on any system there will be an error if new_p is non-empty, but on some systems there may not be an error if new_p is empty.

[shuffle2]

i think the posix way to think about it is that only the fs node of the directory itself is being moved. So if you moved old_p to a new_p which has children, the child nodes of new_p may not be reachable anymore. So the posix way of dealing with this is forcing the caller to deal with the children of new_p in some way.

[AdmiralCurtiss]

Hm... well whatever the case, it's written in a very confusing way.

I'll try this on a Linux system in a bit just to be sure.

[AdmiralCurtiss]

Okay yeah your reading is correct, if the destination is a non-empty directory it fails on Linux too.

#include <filesystem>
#include <fstream>
#include <iostream>

int main(int argc, char** argv) {
  namespace fs = std::filesystem;

  fs::path source_dir("./test_source_dir");
  fs::path dest_dir("./test_dest_dir");

  fs::remove_all(source_dir);
  fs::remove_all(dest_dir);
  fs::create_directory(source_dir);
  fs::create_directory(dest_dir);
  std::ofstream((source_dir/"1.txt").c_str()) << "source file 1";
  std::ofstream((source_dir/"2.txt").c_str()) << "source file 2";
  std::ofstream((dest_dir/"1.txt").c_str()) << "dest file 1";
  std::ofstream((dest_dir/"3.txt").c_str()) << "dest file 3";

  std::error_code error;
  fs::rename(source_dir, dest_dir, error);
  if (error) {
    std::cout << "error: " << error.message() << std::endl;
  } else {
    std::cout << "success" << std::endl;
  }

  return 0;
}

prints

error: Directory not empty

So do you want anything else changed or is this good as-is then?

[shuffle2]

lgtm, thanks for checking!