Trying to use os.rename to rename a dir fails if it's not empty on overlay mounts:
$ t="$(mktemp -d)"
$ monty -m "${t}::/mnt::overlay" -c "import os; os.mkdir('/mnt/foo'); open('/mnt/foo/x','w').close(); os.rename('/mnt/foo','/mnt/foo')"
error: Traceback (most recent call last):
File "<string>", line 1, in <module>
import os; os.mkdir('/mnt/foo'); open('/mnt/foo/x','w').close(); os.rename('/mnt/foo','/mnt/foo')
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
OSError: [Errno 39] Directory not empty: '/mnt/foo'
$ echo $?
1CPython doesn't fail in the same case (at least on Linux):
$ t="$(mktemp -d)"
$ python -c "import os; os.mkdir('${t}/foo'); open('${t}/foo/x','w').close(); os.rename('${t}/foo','${t}/foo')"
$ echo $?
0This is not documented in Python, but is documented in the POSIX definition of rename: https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html
If the old argument and the new argument resolve to either the same existing directory entry or different directory entries for the same existing file, rename() shall return successfully and perform no other action.
I believe the fix is as simple as:
diff --git a/crates/monty-fs/src/overlay.rs b/crates/monty-fs/src/overlay.rs
index dd3375e6..5961ee7f 100644
--- a/crates/monty-fs/src/overlay.rs
+++ b/crates/monty-fs/src/overlay.rs
@@ -1082,6 +1082,11 @@ fn rename(
None => host_is_dir(ctx.mount_dir, resolve_real(src_vpath, ctx)?.for_dir_op()),
};
+ // A self-rename is a no-op on every POSIX system regardless of contents
+ if src_rel == dst_rel {
+ return Ok(MontyObject::none());
+ }
+
reject_rename_type_mismatch(state, &dst_rel, src_is_dir, ctx, dst_vpath)?;
// Renaming a directory onto an existing non-empty directory must fail,This should be the correct place, as it must come after the reject_mount_root checks to keep the deliberate mount root errors, and it must be after the src_is_dir check to still disallow a symlinked source.
Changing it here would also fix os.replace and pathlib.Path.rename AFAICT.
Happy to draft a PR with that change.
Activity
2Hey @jaheba! Sorry for the dealy.
I just submitted https://github.com/pydantic/monty/pull/1109 to fix this.
Thanks for the issue and the proposed fix. It would be great, if you open a PR for this.
They should be all the same, but we can just add tests?