I'd expect it to match POSIX's mv: https://linux.die.net/man/1/mv

i.e. if destination is a directory, source is always moved into it.

But the existing base::Move documentation is very vague around what happens to directories (i.e. never mentions moving into):

// Moves the given path, whether it's a file or a directory.
// If a simple rename is not possible, such as in the case where the paths are
// on different volumes, this will attempt to copy and delete. Returns
// true for success.
// This function fails if either path contains traversal components ('..').
BASE_EXPORT bool Move(const FilePath& from_path, const FilePath& to_path);


Given that, I'd assume callers in Chromium always expect to overwrite the destination when moving a directory.

Looks like there are 49 (35 non-test) callers [1], worth doing a survey and clarifying the API?

[1] https://cs.chromium.org/chromium/src/base/files/file_util.h?q=base::Move&sq=package:chromium&l=86&dr=Ss


If we need to support the "move into" use case I think we should have an enum to have callers explicitly specify intent (like other ambiguous base APIs do, e.g. SplitString) -- akin to -t/-T options in POSIX's mv.

An empty dir shouldn't behave differently than a non-empty dir O_O. I agree with everything gab said but it sounds like the only outlier here is posix moving into an empty dir.. behaving unlike posix. I'd be surprised but interested to know if any non-test callers rely on that behaviour. I suspect the easiest path forward would be to eliminate that case and always move the directory into the named target dir if it exists.

Having it match shell mv/move seems logical when I first think about it.

But in an installer or other non-interactive use it seems like it'd be best to have case1 separated from case2/3, so that the caller is explicit about whether they expect a rename of the tail component, or a move. Then homogenizing case2 with case3 seems clearly right.

What if source is a file? What if destination exists and is a file?

I actually don’t think that any of these cases should behave differently. move(source, /path/to/destination) should always move source to /path/to/destination, precisely, or fail. If /path/to/destination already exists, then I think it’s fine to fail. If the engineer really wanted /path/to/destination/basename(source), then they should have specified that as the destination instead.

This is the most predictable and the least subject to racing external filesystem activity.

That's a really good idea, mark. Then things can't end up in accidental places.

Agreed, I don't think callers in Chromium ever intend to not overwrite destination (all existing ones I've seen first delete dest, then move, maybe we should just have an explicit overwrite flag -- which only influences whether we fail or overwrite dest when it exists, never moving into dest?).

But like I said in #1 there are 35 callers and we should probably survey existing use cases?

I mostly agree with Mark as well. rename() is destructive when called with two files, so I think it makes sense for base::Move to be destructive as well. I suggest taking things one step further and making it unconditionally destructive (i.e., get rid of the failure mode where |from_path| names a directory and |to_path| names a file as a result of someone squatting on the filesystem).

How does this sound:

// Moves |from_path| to |to_path|, replacing |to_path| if it exists. Beyond
// ordinary access rights restrictions (e.g., the process must have permission
// to modify the files and directories operated on), this function return false
// if |from_path| does not exist. |from_path| is copied to |to_path| and then
// deleted if it cannot be moved (e.g., the two paths name items on different
// volumes). This function returns true as long as |to_path| names the former
// contents of |from_path|, even if some or all of |from_path| could not be
// deleted.
//
// Special considerations when |from_path| names a file:
// - The operation fails if |to_path| names a file that cannot be deleted or
//   names a directory that cannot be entirely deleted.
//
// Special considerations when |from_path| names a directory:
// - The operation fails if |to_path| names a file that cannot be deleted or
//   names a directory for which none of its contents can be deleted.
//   Specifically, the contents of |from_path| will be copied into |to_path| if
//   any portion of |to_path| could be deleted. Callers requiring that |to_path|
//   be entirely deleted prior to moving must do so explicitly.
//
// On failure, no guarantees are made regarding any prior contents of |to_path|
// (some or all may have been deleted). The prior contents of |from_path| are
// left in-tact on failure.

I like it but this section is weird to me.

// - The operation fails if |to_path| names a file that cannot be deleted or
//   names a directory for which none of its contents can be deleted.
//   Specifically, the contents of |from_path| will be copied into |to_path| if
//   any portion of |to_path| could be deleted.

Why wouldn't we required that all of |to_path| can be fully deleted? Otherwise we allow cases where |to_path| grows without bounds (ref.  issue 179576 ).

Agreed; growing a target directory without bounds is bad.

I had that there because there's a failure mode on Windows where you think you've deleted everything in a directory but you still can't delete the directory itself until any open handles on deleted files in the directory are closed. I think that my robust delete in https://codereview.chromium.org/2545283002/ will drastically mitigate this, so maybe that part isn't needed. If we go with a destructive move strategy that fails if the target can't be deleted, then it becomes impossible for a caller who wants that to proceeed.

So, back to some options:
- Move is never destructive -- if the target exists the move fails, so the caller must Delete ahead of time if they desire.
- Move is strictly destructive -- the move fails if the delete is incomplete.
- Move is lazily destructive -- the move proceeds if the delete is incomplete.

I can see merit to each. I've started to survey all callers of base::Move to see which makes everyone's lives easier.

You started fixing this bug over two years ago. Are you still working on it? You can update the status to "archived", "wontfix", or "closed". You can remove yourself as owner and change status to "untriaged", but if this is still a real bug, please do not sit on it.

I'm not actively working on this, though it is still an issue.