Thanks for putting this up! I have two questions reading this that I think we need to answer before merging this.

1. Can we think of any other cases where the rename call fails that we need to take into account?

2. What does GNU do? We can't look at the source code, but we can use strace on GNU mv to see what syscalls it performs. I'd like to know whether they also give up after a failed rename.

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@tertsdiepraam

1. maybe EIO should be considered (such as the file system does not support move operation i gussed)?
2. yes, the gnu mv give up copying files after EACCES fired:

   renameat2(AT_FDCWD, "docs", AT_FDCWD, "../some-docs", RENAME_NOREPLACE) = -1 EACCES (Permission denied)
   newfstatat(AT_FDCWD, "../some-docs", 0x7ffd6d9a2cd0, 0) = -1 ENOENT (No such file or directory)
   newfstatat(AT_FDCWD, "docs", {st_mode=S_IFDIR|0755, st_size=132, ...}, AT_SYMLINK_NOFOLLOW) = 0
   newfstatat(AT_FDCWD, "../some-docs", 0x7ffd6d9a2830, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
   ....
   write(2, ": Permission denied", 19: Permission denied)
   

   it fallback to copy if we copy files across device (mv docs /tmp/some-docs):

   renameat2(AT_FDCWD, "docs", AT_FDCWD, "/tmp/some-docs", RENAME_NOREPLACE) = -1 EXDEV (Invalid cross-device link)
   newfstatat(AT_FDCWD, "/tmp/some-docs", 0x7ffc6b18c1b0, 0) = -1 ENOENT (No such file or directory)
   newfstatat(AT_FDCWD, "docs", {st_mode=S_IFDIR|0755, st_size=132, ...}, AT_SYMLINK_NOFOLLOW) = 0
   newfstatat(AT_FDCWD, "/tmp/some-docs", 0x7ffc6b18bd10, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
   rmdir("/tmp/some-docs")                 = -1 ENOENT (No such file or directory)
   lgetxattr("docs", "security.selinux", "unconfined_u:object_r:user_tmp_t"..., 255) = 36
   access("/var/run/setrans/.setrans-unix", F_OK) = -1 ENOENT (No such file or directory)
   futex(0x7fd465c55520, FUTEX_WAKE_PRIVATE, 2147483647) = 0
   futex(0x7fd465c56644, FUTEX_WAKE_PRIVATE, 2147483647) = 0
   openat(AT_FDCWD, "/proc/thread-self/attr/fscreate", O_RDWR|O_CLOEXEC) = 3
   write(3, "unconfined_u:object_r:user_tmp_t"..., 36) = 36
   close(3)                                = 0
   mkdir("/tmp/some-docs", 0700)           = 0
   newfstatat(AT_FDCWD, "/tmp/some-docs", {st_mode=S_IFDIR|0700, st_size=6, ...}, AT_SYMLINK_NOFOLLOW) = 0
   openat(AT_FDCWD, "docs", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 3
   fstat(3, {st_mode=S_IFDIR|0755, st_size=132, ...}) = 0
   getdents64(3, 0x561b1e9f5fc0 /* 9 entries */, 32768) = 280
   getdents64(3, 0x561b1e9f5fc0 /* 0 entries */, 32768) = 0
   close(3)                                = 0
   newfstatat(AT_FDCWD, "docs/src", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
   newfstatat(AT_FDCWD, "/tmp/some-docs/src", 0x7ffc6b18b7c0, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
   rmdir("/tmp/some-docs/src")             = -1 ENOENT (No such file or directory)
   lgetxattr("docs/src", "security.selinux", "unconfined_u:object_r:user_tmp_t"..., 255) = 36
   mkdir("/tmp/some-docs/src", 0700)       = 0
   newfstatat(AT_FDCWD, "/tmp/some-docs/src", {st_mode=S_IFDIR|0700, st_size=6, ...}, AT_SYMLINK_NOFOLLOW) = 0
   openat(AT_FDCWD, "docs/src", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 3
   fstat(3, {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
   getdents64(3, 0x561b1e9fe1d0 /* 15 entries */, 32768) = 512
   getdents64(3, 0x561b1e9fe1d0 /* 0 entries */, 32768) = 0
   close(3)                                = 0
   ...
   

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/chown/preserve-root is no longer failing!

Two jobs are failing. Minor stuff :)

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?

@sylvestre i have fixed cSpell and Cargo.toml formatting issues.

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?

but it does not seem to be caused my changes.

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?

sorry but it is very very likely your change here

@sylvestre Thanks for your patience. I have found the problem, the call to std::fs::rename("E/", "E/.~1~") reported an error: EINVAL. This is not an EXDEV error, so I made it return immediately.

But I don't think it's my fault, because the GNU tests running mv -T --backup=numbered C E/ || fail=1 will move E to E.~1~, but uu_mv will move E to E/.~1~. This is clearly incorrect.

Before my changes, the GNU tests passed, but the result is incorrect. see the outputs below:

hamflx@hamflx-workstation:~/coreutils/test$ git fetch origin
hamflx@hamflx-workstation:~/coreutils/test$ git reset --hard origin/main
HEAD is now at dcb53b6c9 head: add missing features
hamflx@hamflx-workstation:~/coreutils/test$ cargo build -p uu_mv
    Finished dev [unoptimized + debuginfo] target(s) in 0.04s
hamflx@hamflx-workstation:~/coreutils/test$ find . -delete && mkdir C D E && touch C/c D/d E/e
hamflx@hamflx-workstation:~/coreutils/test$ mv -T --backup=numbered C E/
hamflx@hamflx-workstation:~/coreutils/test$ tree -a
.
├── D
│   └── d
├── E
│   └── c
└── E.~1~
    └── e

3 directories, 3 files
hamflx@hamflx-workstation:~/coreutils/test$ find . -delete && mkdir C D E && touch C/c D/d E/e
hamflx@hamflx-workstation:~/coreutils/test$ ../target/debug/mv -T --backup=numbered C E/
hamflx@hamflx-workstation:~/coreutils/test$ tree -a
.
├── D
│   └── d
└── E
    └── c

2 directories, 2 files

I have created a new branch to try to fix this problem: #6119. The outputs after this change:

➜  test git:(fix/invalid-backup-numbered-path) find . -delete && mkdir C D E && touch C/c D/d E/e
➜  test git:(fix/invalid-backup-numbered-path) ../target/debug/mv -T --backup=numbered C E/
➜  test git:(fix/invalid-backup-numbered-path) tree -a
.
├── D
│   └── d
├── E
│   └── c
└── E.~1~
    └── e

3 directories, 3 files

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?

GNU testsuite comparison:

GNU test failed: tests/mv/backup-dir. tests/mv/backup-dir is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

What needs to be done to get this PR merged?

@sylvestre

I think @jfinkels has been upgrading your PR :)
it should land soon. sorry about the latency

@hamflx the best thing to do is resolve the merge conflicts and build failures in this merge request. If it helps, you can look at my attempt to update your branch #7152. (I don't have Windows so it's very hard for me to debug the Windows CI failures.)

This change adds a Windows-specific check that explicitly removes the destination file before attempting to rename/move the source file. This is needed because on Windows, if the source file is opened (even with FILE_SHARE_DELETE) and the destination file exists, the MoveFileExW operation will fail with "Access Denied". By removing the destination file first, we ensure the move operation can succeed.

fn rename_with_fallback(
    from: &Path,
    to: &Path,
    multi_progress: Option<&MultiProgress>,
) -> io::Result<()> {
    #[cfg(windows)]
    if to.is_file() {
        // On Windows, if the source file is opened (even with FILE_SHARE_DELETE) and
        // the destination file exists, MoveFileExW will fail with "Access Denied".
        // We need to explicitly remove the destination file first to make the move operation succeed.
        fs::remove_file(to)?;
    }
    // ...
}

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)

GNU testsuite comparison:

Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)

I adjusted the solution by removing the deletion logic before rename (which could lead to scenarios where the file move fails but the target file is deleted).

Instead, it now directly falls back to the copy logic. However, to avoid situations where copy operations might fail due to lacking delete permissions on the source file, we added the can_delete_file function to check whether the source file can be deleted.

@sylvestre ready for review

Thanks and well done :)