Modifications to os::copy_file()

[hatahet]

• Now returns false if source and destination are the same.
• Returns false if source is a directory.

Fixes #9947

[catamorphism]

Can you rebase this so there's no merge node? See https://github.com/mozilla/rust/wiki/Note-git-workflow

Thanks!

[hatahet]

Done

[alexcrichton]

I don't think that explicitly checking for a directory is the right thing to do. Testing for equality is also a little tricky I think. We currently don't have a lot of symlink support, but I would imagine that if you copy a symlink to the file that it points to you would run into similar trouble (and this doesn't handle that case).

For the directory case, I would be much happier about attempting the copy, and then if the copy fails we remove the file we just created (the destination). There would be no special logic for whether we were copying a directory or not (what happens if you copy a unix pipe into a destination, I presume that creates an empty file as well).

[hatahet]

@alexcrichton The problem is, copy_file() still returns true if a directory was given as a source. However, it will actually create a file as you pointed out, not a directory.

[alexcrichton]

I agree that that shouldn't happen, but the checks that were implemented in this patch aren't sufficient in catching all cases. If you copy a directory to a file, it will return false, but what happens if you copy a unix pipe to a file?

The logic should either be "do it and cleanup on failure", or "don't do it unless the source is a file" and not explicitly check for a directory at all.

[hatahet]

Gotcha, that makes sense.

[hatahet]

So, for the time being, making it only work if from.is_file is sufficient? Then we can open another issue for the other cases.

[alexcrichton]

I think that we can pretty much all of the edge cases with logic like:

fn copy_file(src: Path, dst: Path) -> bool {
  if dst.exists() { return false; }
  if !src.is_file() { return false; }
  // ... copy file
}

Then the documentation can be explicit about when an error is generated and when an error occurs. It may also be worth mentioning that this does not protect against concurrent systems which may create the destination before the file is copied, but after it's checked to not exist.

[alexcrichton]

This may be causing bors to get stuck. Closing to see if it un-sticks bors

[alexcrichton]

Hm it appears that this is causing bors to go haywire, would you mind re-opening as a new pull request? Thanks!

[hatahet]

Ugh, sorry about that, I deleted my GitHub fork and made a new one. I think things are ok for the time being?

[alexcrichton]

Would you mind reopening just to make sure? Just be sure to link it with a cc to this pull request so we know what's what.

[hatahet]

First reference was a mistake, I removed it from the parent issue.