From 42bde675debfe56eca110467e003cf5838b7baf1 Mon Sep 17 00:00:00 2001
From: Ahmad Samir <a.samirh78@gmail.com>
Date: Mon, 10 Jun 2024 19:04:19 +0300
Subject: [PATCH] QFileSystemEngine/Unix: refactor rmpath()

Convert the path to QByteArray once up top, instead of using
QFile::encodeName() multiple times in the for-loop, and then use the
same QByteArray and truncate() it in each iteration.

Extend tst_qdir unittests.

Change-Id: I0d879e7f429db25879859acab074c2c197f41f6c
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/corelib/io/qfilesystemengine_unix.cpp | 16 +++++++++------
 tests/auto/corelib/io/qdir/CMakeLists.txt |  1 -
 tests/auto/corelib/io/qdir/tst_qdir.cpp   | 25 +++++++++++++++++++----
 3 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/src/corelib/io/qfilesystemengine_unix.cpp b/src/corelib/io/qfilesystemengine_unix.cpp
index 627447ec381..84010d5a909 100644
--- a/src/corelib/io/qfilesystemengine_unix.cpp
+++ b/src/corelib/io/qfilesystemengine_unix.cpp
@@ -1198,15 +1198,19 @@ bool QFileSystemEngine::rmdir(const QFileSystemEntry &entry)
 
 bool QFileSystemEngine::rmpath(const QFileSystemEntry &entry)
 {
-    const QString path = QDir::cleanPath(entry.filePath());
+    QByteArray path = QFile::encodeName(QDir::cleanPath(entry.filePath()));
     Q_CHECK_FILE_NAME(path, false);
 
-    for (qsizetype oldslash = 0, slash = path.size(); slash > 0; oldslash = slash) {
-        const QByteArray chunk = QFile::encodeName(path.left(slash));
-        if (::rmdir(chunk.constData()) != 0)
-            return oldslash != 0;
+    if (::rmdir(path.constData()) != 0)
+        return false; // Only return false if `entry` couldn't be deleted
 
-        slash = path.lastIndexOf(QDir::separator(), oldslash - 1);
+    const char sep = QDir::separator().toLatin1();
+    qsizetype slash = path.lastIndexOf(sep);
+    // `slash > 0` because truncate(0) would make `path` empty
+    for (; slash > 0; slash = path.lastIndexOf(sep)) {
+        path.truncate(slash);
+        if (::rmdir(path.constData()) != 0)
+            break;
     }
 
     return true;
diff --git a/tests/auto/corelib/io/qdir/CMakeLists.txt b/tests/auto/corelib/io/qdir/CMakeLists.txt
index c0ee97528c2..4be2342664d 100644
--- a/tests/auto/corelib/io/qdir/CMakeLists.txt
+++ b/tests/auto/corelib/io/qdir/CMakeLists.txt
@@ -20,7 +20,6 @@ list(APPEND test_data_dirs "entrylist")
 list(APPEND test_data_dirs "types")
 list(APPEND test_data_dirs "tst_qdir.cpp")
 
-
 set(test_data_files)
 
 foreach(dir  ${test_data_dirs})
diff --git a/tests/auto/corelib/io/qdir/tst_qdir.cpp b/tests/auto/corelib/io/qdir/tst_qdir.cpp
index 90d2d45c689..0e5b997eccb 100644
--- a/tests/auto/corelib/io/qdir/tst_qdir.cpp
+++ b/tests/auto/corelib/io/qdir/tst_qdir.cpp
@@ -364,10 +364,11 @@ void tst_QDir::mkdirRmdir_data()
     const struct {
         const char *name; // shall have a prefix added
         const char *path; // relative
-        bool recurse;
+        bool recurse; // QDir::rmpath() vs. QDir::rmdir()
     } cases[] = {
         { "plain", "testdir/one", false },
         { "recursive", "testdir/two/three/four", true },
+        { "recursive-name-length-1", "a/b/c", true },
         { "with-..", "testdir/../testdir/three", false },
     };
 
@@ -383,6 +384,8 @@ void tst_QDir::mkdirRmdir()
     QFETCH(QString, path);
     QFETCH(bool, recurse);
 
+    QTest::ThrowOnFailEnabler thrower;
+
     QDir dir;
     dir.rmdir(path);
     if (recurse)
@@ -394,10 +397,24 @@ void tst_QDir::mkdirRmdir()
     QFileInfo fi(path);
     QVERIFY2(fi.exists() && fi.isDir(), msgDoesNotExist(path).constData());
 
-    if (recurse)
-        QVERIFY(dir.rmpath(path));
-    else
+    if (recurse) {
+        // Check that rmpath() removed all empty parent dirs
+        auto verifyRmPath = [&dir, &path](QLatin1StringView subdir) {
+            QFileInfo fi(QDir::currentPath() + subdir);
+            QVERIFY(fi.exists());
+            QVERIFY(dir.rmpath(path));
+            fi.refresh();
+            QVERIFY(!fi.exists());
+        };
+        if (path.contains("testdir/two/three/four"_L1))
+            verifyRmPath("/testdir/two"_L1);
+        else if (path.contains("a/b/c"_L1))
+            verifyRmPath("/a"_L1);
+        else
+            QVERIFY(dir.rmpath(path));
+    } else {
         QVERIFY(dir.rmdir(path));
+    }
 
     //make sure it really doesn't exist (ie that rmdir returns the right value)
     fi.refresh();