Issue #15154: Add "dir_fd" parameter to os.rmdir, remove "rmdir"

parameter from os.remove / os.unlink.
Patch written by Georg Brandl.  (I'm really looking forward to George
getting commit privileges so I don't have to keep doing checkins on his
behalf.)

diff --git a/Doc/library/os.rst b/Doc/library/os.rst
index 3b3bd3dd332e224f66d4f456aeec749d6b3df7a6..65a0ac1b0d14dfc539cd1c5dd66494856e6273cf 100644 (file)
--- a/Doc/library/os.rst
+++ b/Doc/library/os.rst
@@ -1750,14 +1750,10 @@ Files and Directories
       The *dir_fd* argument.
 
 
-.. function:: remove(path, *, dir_fd=None, rmdir=False)
+.. function:: remove(path, *, dir_fd=None)
 
-   Remove (delete) the file *path*.  This function is identical to
-   :func:`os.unlink`.
-
-   Specify ``rmdir=True`` if *path* is a directory.  Failing to do so
-   will raise an exception; likewise, specifying ``rmdir=True`` when
-   *path* is not a directory will also raise an exception.
+   Remove (delete) the file *path*.  If *path* is a directory, :exc:`OSError`
+   is raised.  Use :func:`rmdir` to remove directories.
 
    If *dir_fd* is not ``None``, it should be a file descriptor referring to a
    directory, and *path* should be relative; path will then be relative to
@@ -1771,10 +1767,12 @@ Files and Directories
    be raised; on Unix, the directory entry is removed but the storage allocated
    to the file is not made available until the original file is no longer in use.
 
+   This function is identical to :func:`unlink`.
+
    Availability: Unix, Windows.
 
    .. versionadded:: 3.3
-      The *dir_fd* and *rmdir* arguments.
+      The *dir_fd* argument.
 
 
 .. function:: removedirs(path)
@@ -1872,14 +1870,25 @@ Files and Directories
    .. versionadded:: 3.3
 
 
-.. function:: rmdir(path)
+.. function:: rmdir(path, *, dir_fd=None)
 
    Remove (delete) the directory *path*.  Only works when the directory is
    empty, otherwise, :exc:`OSError` is raised.  In order to remove whole
    directory trees, :func:`shutil.rmtree` can be used.
 
+   If *dir_fd* is not ``None``, it should be a file descriptor referring to a
+   directory, and *path* should be relative; path will then be relative to
+   that directory.  (If *path* is absolute, *dir_fd* is ignored.)
+   *dir_fd* may not be supported on your platform;
+   you can check whether or not it is available using
+   :data:`os.supports_dir_fd`.  If it is unavailable, using it will raise
+   a :exc:`NotImplementedError`.
+
    Availability: Unix, Windows.
 
+   .. versionadded:: 3.3
+      The *dir_fd* parameter.
+
 
 .. data:: XATTR_SIZE_MAX
 
@@ -2235,9 +2244,9 @@ Files and Directories
    .. versionadded:: 3.3
 
 
-.. function:: unlink(path, *, dir_fd=None, rmdir=False)
+.. function:: unlink(path, *, dir_fd=None)
 
-   Remove (delete) the file *path*.  This is the same function as
+   Remove (delete) the file *path*.  This function is identical to
    :func:`remove`; the :func:`unlink` name is its traditional Unix
    name.  Please see the documentation for :func:`remove` for
    further information.
@@ -2245,7 +2254,7 @@ Files and Directories
    Availability: Unix, Windows.
 
    .. versionadded:: 3.3
-      The *dir_fd* and *rmdir* parameters.
+      The *dir_fd* parameter.
 
 
 .. function:: utime(path, times=None, *, ns=None, dir_fd=None, follow_symlinks=True)

diff --git a/Lib/os.py b/Lib/os.py
index b5ad1b515dd60bd2ea8740a446419f5b882ef973..4a40cfed6b502d27ea9ec2d7bdaf4c18aa7f65dc 100644 (file)
--- a/Lib/os.py
+++ b/Lib/os.py
@@ -157,6 +157,7 @@ if _exists("_have_functions"):
     _add("HAVE_RENAMEAT",   "rename")
     _add("HAVE_SYMLINKAT",  "symlink")
     _add("HAVE_UNLINKAT",   "unlink")
+    _add("HAVE_UNLINKAT",   "rmdir")
     _add("HAVE_UTIMENSAT",  "utime")
     supports_dir_fd = _set
 
@@ -214,10 +215,6 @@ if _exists("_have_functions"):
     _add("MS_WINDOWS",      "stat")
     supports_follow_symlinks = _set
 
-    _set = set()
-    _add("HAVE_UNLINKAT",   "unlink")
-    supports_remove_directory = _set
-
     del _set
     del _have_functions
     del _globals

diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
index cc1120f70c3cf44bd02d324e5b59e4a5d79a8e9c..654dd23a5cf8e604d2d0e570718d00872e2d22d7 100644 (file)
--- a/Lib/test/test_os.py
+++ b/Lib/test/test_os.py
@@ -785,7 +785,10 @@ class FwalkTests(WalkTests):
                 os.unlink(name, dir_fd=rootfd)
             for name in dirs:
                 st = os.stat(name, dir_fd=rootfd, follow_symlinks=False)
-                os.unlink(name, dir_fd=rootfd, rmdir=stat.S_ISDIR(st.st_mode))
+                if stat.S_ISDIR(st.st_mode):
+                    os.rmdir(name, dir_fd=rootfd)
+                else:
+                    os.unlink(name, dir_fd=rootfd)
         os.rmdir(support.TESTFN)
 
 

diff --git a/Misc/NEWS b/Misc/NEWS
index 031550ef10f1af535109ecaf64eec60f9e16272a..fadf214289d1daca752d2dd9d2a142ae9f4bf13f 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -47,6 +47,9 @@ Core and Builtins
 Library
 -------
 
+- Issue #15154: Add "dir_fd" parameter to os.rmdir, remove "rmdir"
+  parameter from os.remove / os.unlink.
+
 - Issue #4489: Add a shutil.rmtree that isn't susceptible to symlink attacks.
   It is used automatically on platforms supporting the necessary os.openat()
   and os.unlinkat() functions. Main code by Martin von Löwis.

diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index 30b797c2ab1525e2b639be65827cfa720ba790dc..bfe32b81544699f50eff7dc52295318f9f7dcfb7 100644 (file)
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -4084,17 +4084,62 @@ posix_replace(PyObject *self, PyObject *args, PyObject *kwargs)
 }
 
 PyDoc_STRVAR(posix_rmdir__doc__,
-"rmdir(path)\n\n\
-Remove a directory.");
+"rmdir(path, *, dir_fd=None)\n\n\
+Remove a directory.\n\
+\n\
+If dir_fd is not None, it should be a file descriptor open to a directory,\n\
+  and path should be relative; path will then be relative to that directory.\n\
+dir_fd may not be implemented on your platform.\n\
+  If it is unavailable, using it will raise a NotImplementedError.");
 
 static PyObject *
-posix_rmdir(PyObject *self, PyObject *args)
+posix_rmdir(PyObject *self, PyObject *args, PyObject *kwargs)
 {
+    path_t path;
+    int dir_fd = DEFAULT_DIR_FD;
+    static char *keywords[] = {"path", "dir_fd", NULL};
+    int result;
+    PyObject *return_value = NULL;
+
+    memset(&path, 0, sizeof(path));
+    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O&|$O&:rmdir", keywords,
+            path_converter, &path,
+#ifdef HAVE_UNLINKAT
+            dir_fd_converter, &dir_fd
+#else
+            dir_fd_unavailable, &dir_fd
+#endif
+            ))
+        return NULL;
+
+    Py_BEGIN_ALLOW_THREADS
 #ifdef MS_WINDOWS
-    return win32_1str(args, "rmdir", "y:rmdir", RemoveDirectoryA, "U:rmdir", RemoveDirectoryW);
+    if (path.wide)
+        result = RemoveDirectoryW(path.wide);
+    else
+        result = RemoveDirectoryA(path.narrow);
+    result = !result; /* Windows, success=1, UNIX, success=0 */
 #else
-    return posix_1str(args, "O&:rmdir", rmdir);
+#ifdef HAVE_UNLINKAT
+    if (dir_fd != DEFAULT_DIR_FD)
+        result = unlinkat(dir_fd, path.narrow, AT_REMOVEDIR);
+    else
 #endif
+        result = rmdir(path.narrow);
+#endif
+    Py_END_ALLOW_THREADS
+
+    if (result) {
+        return_value = path_error("rmdir", &path);
+        goto exit;
+    }
+
+    return_value = Py_None;
+    Py_INCREF(Py_None);
+
+exit:
+    path_cleanup(&path);
+    return return_value;
 }
 
 
@@ -4186,68 +4231,54 @@ BOOL WINAPI Py_DeleteFileW(LPCWSTR lpFileName)
 #endif /* MS_WINDOWS */
 
 PyDoc_STRVAR(posix_unlink__doc__,
-"unlink(path, *, dir_fd=None, rmdir=False)\n\n\
+"unlink(path, *, dir_fd=None)\n\n\
 Remove a file (same as remove()).\n\
 \n\
 If dir_fd is not None, it should be a file descriptor open to a directory,\n\
   and path should be relative; path will then be relative to that directory.\n\
 dir_fd may not be implemented on your platform.\n\
-  If it is unavailable, using it will raise a NotImplementedError.\n\
-If rmdir is True, unlink will behave like os.rmdir().");
+  If it is unavailable, using it will raise a NotImplementedError.");
 
 PyDoc_STRVAR(posix_remove__doc__,
-"remove(path, *, dir_fd=None, rmdir=False)\n\n\
+"remove(path, *, dir_fd=None)\n\n\
 Remove a file (same as unlink()).\n\
 \n\
 If dir_fd is not None, it should be a file descriptor open to a directory,\n\
   and path should be relative; path will then be relative to that directory.\n\
 dir_fd may not be implemented on your platform.\n\
-  If it is unavailable, using it will raise a NotImplementedError.\n\
-If rmdir is True, remove will behave like os.rmdir().");
+  If it is unavailable, using it will raise a NotImplementedError.");
 
 static PyObject *
 posix_unlink(PyObject *self, PyObject *args, PyObject *kwargs)
 {
     path_t path;
     int dir_fd = DEFAULT_DIR_FD;
-    int remove_dir = 0;
-    static char *keywords[] = {"path", "dir_fd", "rmdir", NULL};
+    static char *keywords[] = {"path", "dir_fd", NULL};
     int result;
     PyObject *return_value = NULL;
 
     memset(&path, 0, sizeof(path));
-    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O&|$O&p:unlink", keywords,
+    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O&|$O&:unlink", keywords,
             path_converter, &path,
 #ifdef HAVE_UNLINKAT
-            dir_fd_converter, &dir_fd,
+            dir_fd_converter, &dir_fd
 #else
-            dir_fd_unavailable, &dir_fd,
+            dir_fd_unavailable, &dir_fd
 #endif
-            &remove_dir))
+            ))
         return NULL;
 
     Py_BEGIN_ALLOW_THREADS
 #ifdef MS_WINDOWS
-    if (remove_dir) {
-        if (path.wide)
-            result = RemoveDirectoryW(path.wide);
-        else
-            result = RemoveDirectoryA(path.narrow);
-    }
-    else {
-        if (path.wide)
-            result = Py_DeleteFileW(path.wide);
-        else
-            result = DeleteFileA(path.narrow);
-    }
+    if (path.wide)
+        result = Py_DeleteFileW(path.wide);
+    else
+        result = DeleteFileA(path.narrow);
     result = !result; /* Windows, success=1, UNIX, success=0 */
 #else
-    if (remove_dir && (dir_fd == DEFAULT_DIR_FD))
-        result = rmdir(path.narrow);
-    else
 #ifdef HAVE_UNLINKAT
     if (dir_fd != DEFAULT_DIR_FD)
-        result = unlinkat(dir_fd, path.narrow, remove_dir ? AT_REMOVEDIR : 0);
+        result = unlinkat(dir_fd, path.narrow, 0);
     else
 #endif /* HAVE_UNLINKAT */
         result = unlink(path.narrow);
@@ -10806,7 +10837,9 @@ static PyMethodDef posix_methods[] = {
     {"replace",         (PyCFunction)posix_replace,
                         METH_VARARGS | METH_KEYWORDS,
                         posix_replace__doc__},
-    {"rmdir",           posix_rmdir, METH_VARARGS, posix_rmdir__doc__},
+    {"rmdir",           (PyCFunction)posix_rmdir,
+                        METH_VARARGS | METH_KEYWORDS,
+                        posix_rmdir__doc__},
     {"stat",            (PyCFunction)posix_stat,
                         METH_VARARGS | METH_KEYWORDS,
                         posix_stat__doc__},