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.)

Author: larryhastings

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,17 +2244,17 @@ 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.
 
    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)

Lib/os.py

@@ -157,6 +157,7 @@ def _add(str, fn):
     _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 @@ def _add(str, fn):
     _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

Lib/test/test_os.py

@@ -785,7 +785,10 @@ def tearDown(self):
                 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)
 
 

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.

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__},