Fix FileSaver and add some autotests to make sure it works

Change-Id: I707edb703068868014b4434b7508bfa41617383c
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

5 files changed, 173 insertions, 16 deletions

diff --git a/src/lib/corelib/tools/filesaver.cpp b/src/lib/corelib/tools/filesaver.cpp
index db795b012..0fd652657 100644
--- a/src/lib/corelib/tools/filesaver.cpp
+++ b/src/lib/corelib/tools/filesaver.cpp
@@ -40,11 +40,9 @@
 #include "filesaver.h"
 #include "stlutils.h"
 
-#include <QtCore/qfile.h>
-#include <QtCore/qsavefile.h>
-
 #include <tools/iosutils.h>
 
+#include <cerrno>
 #include <fstream>
 
 namespace qbs {
@@ -71,34 +69,50 @@ bool FileSaver::open()
             m_oldFileContents.clear();
     }
 
-    m_newFileContents.clear();
-    m_memoryDevice = std::make_shared<std::ostringstream>(m_newFileContents);
+    m_memoryDevice = std::make_shared<std::stringstream>();
     return true;
 }
 
 bool FileSaver::commit()
 {
-    if (!m_overwriteIfUnchanged && m_oldFileContents == m_newFileContents)
+    if (!device())
+        return false;
+
+    device()->flush();
+    if (!device()->good())
+        return false;
+
+    const std::string newFileContents = m_memoryDevice->str();
+    if (!m_overwriteIfUnchanged && m_oldFileContents == newFileContents)
         return true; // no need to write unchanged data
 
-    const std::string tempFilePath = std::tmpnam(nullptr);
+    const std::string tempFilePath = m_filePath + "~";
     std::ofstream tempFile(utf8_to_native_path(tempFilePath));
     if (!tempFile.is_open())
         return false;
 
-    tempFile.write(m_newFileContents.data(), m_newFileContents.size());
+    tempFile.write(newFileContents.data(), newFileContents.size());
+    tempFile.close();
     if (!tempFile.good())
         return false;
 
-    return Internal::rename(tempFilePath, m_filePath) == 0;
+    if (Internal::rename(tempFilePath, m_filePath) != 0) {
+        if (errno != EEXIST)
+            return false;
+        if (Internal::unlink(m_filePath) != 0)
+            return false;
+        return Internal::rename(tempFilePath, m_filePath) == 0;
+    }
+
+    return true;
 }
 
-size_t FileSaver::write(const std::vector<char> &data)
+bool FileSaver::write(const std::vector<char> &data)
 {
     return fwrite(data, device());
 }
 
-size_t FileSaver::write(const std::string &data)
+bool FileSaver::write(const std::string &data)
 {
     return fwrite(data, device());
 }
diff --git a/src/lib/corelib/tools/filesaver.h b/src/lib/corelib/tools/filesaver.h
index bee04405f..275ad0f01 100644
--- a/src/lib/corelib/tools/filesaver.h
+++ b/src/lib/corelib/tools/filesaver.h
@@ -60,13 +60,12 @@ public:
     std::ostream *device();
     bool open();
     bool commit();
-    size_t write(const std::vector<char> &data);
-    size_t write(const std::string &data);
+    bool write(const std::vector<char> &data);
+    bool write(const std::string &data);
 
 private:
-    std::string m_newFileContents;
     std::string m_oldFileContents;
-    std::shared_ptr<std::ostringstream> m_memoryDevice;
+    std::shared_ptr<std::stringstream> m_memoryDevice;
     const std::string m_filePath;
     const bool m_overwriteIfUnchanged;
 };
diff --git a/src/lib/corelib/tools/iosutils.h b/src/lib/corelib/tools/iosutils.h
index 3fc7b0356..9374b24b1 100644
--- a/src/lib/corelib/tools/iosutils.h
+++ b/src/lib/corelib/tools/iosutils.h
@@ -41,15 +41,19 @@
 #define QBS_IOSUTILS_H
 
 #include <cstdio>
+#include <cstring>
 #include <ostream>
 
 #if defined(_WIN32) && defined(_MSC_VER)
 #include <codecvt>
 #include <locale>
 #define QBS_RENAME_IMPL ::_wrename
+#define QBS_UNLINK_IMPL ::_wunlink
 typedef std::wstring qbs_filesystem_path_string_type;
 #else
+#include <unistd.h>
 #define QBS_RENAME_IMPL ::rename
+#define QBS_UNLINK_IMPL ::unlink
 typedef std::string qbs_filesystem_path_string_type;
 #endif
 
@@ -58,6 +62,8 @@ namespace Internal {
 
 static inline bool fwrite(const char *values, size_t nitems, std::ostream *stream)
 {
+    if (!stream)
+        return false;
     stream->write(values, nitems);
     return stream->good();
 }
@@ -90,6 +96,12 @@ static inline int rename(const std::string &oldName, const std::string &newName)
     return QBS_RENAME_IMPL(wOldName.c_str(), wNewName.c_str());
 }
 
+static inline int unlink(const std::string &name)
+{
+    const auto wName = utf8_to_native_path(name);
+    return QBS_UNLINK_IMPL(wName.c_str());
+}
+
 } // namespace Internal
 } // namespace qbs
 
diff --git a/tests/auto/tools/tst_tools.cpp b/tests/auto/tools/tst_tools.cpp
index dca88f438..b29e2007c 100644
--- a/tests/auto/tools/tst_tools.cpp
+++ b/tests/auto/tools/tst_tools.cpp
@@ -46,6 +46,7 @@
 #include <tools/buildoptions.h>
 #include <tools/error.h>
 #include <tools/fileinfo.h>
+#include <tools/filesaver.h>
 #include <tools/hostosinfo.h>
 #include <tools/processutils.h>
 #include <tools/profile.h>
@@ -67,7 +68,8 @@
 using namespace qbs;
 using namespace qbs::Internal;
 
-TestTools::TestTools(Settings *settings) : m_settings(settings)
+TestTools::TestTools(Settings *settings)
+    : m_settings(settings), testDataDir(testWorkDir("tools"))
 {
 }
 
@@ -76,6 +78,129 @@ TestTools::~TestTools()
     qDeleteAll(m_tmpDirs);
 }
 
+void TestTools::initTestCase()
+{
+    QDir().mkpath(testDataDir);
+}
+
+void TestTools::fileSaver()
+{
+    QVERIFY(QDir::setCurrent(testDataDir));
+
+    static const char *fn = "foo.txt";
+    const auto run = [](const std::function<void()> &func) {
+        if (QFile::exists(fn))
+            QVERIFY(QFile::remove(fn));
+        func();
+        if (QFile::exists(fn))
+            QVERIFY(QFile::remove(fn));
+    };
+
+    // failing to open the file means nothing works
+    run([] {
+        Internal::FileSaver fs(fn);
+        QVERIFY(!fs.device());
+        QVERIFY(!fs.write("hello"));
+        QVERIFY(!fs.commit());
+        QVERIFY(!QFile::exists(fn));
+    });
+
+    // verify that correct usage creates a file with the right contents
+    run([] {
+        Internal::FileSaver fs(fn);
+        QVERIFY(fs.open());
+        QVERIFY(fs.device());
+        QVERIFY(fs.write("hello"));
+        QVERIFY(fs.commit());
+        QVERIFY(QFile::exists(fn));
+        QFile f(fn);
+        QVERIFY(f.open(QIODevice::ReadOnly));
+        QCOMPARE(f.readAll(), QByteArrayLiteral("hello"));
+    });
+
+    // failing to commit writes nothing
+    run([] {
+        Internal::FileSaver fs(fn);
+        QVERIFY(fs.open());
+        QVERIFY(fs.device());
+        QVERIFY(fs.write("hello"));
+        QVERIFY(!QFile::exists(fn));
+    });
+
+    // verify that correct usage creates a file with the right contents and does not overwrite
+    run([] {
+        {
+            Internal::FileSaver fs(fn);
+            QVERIFY(fs.open());
+            QVERIFY(fs.device());
+            QVERIFY(fs.write("hello"));
+            QVERIFY(fs.commit());
+            QVERIFY(QFile::exists(fn));
+            QFile f(fn);
+            QVERIFY(f.open(QIODevice::ReadOnly));
+            QCOMPARE(f.readAll(), QByteArrayLiteral("hello"));
+        }
+
+        const auto lm = QFileInfo(fn).lastModified();
+        QVERIFY(lm.isValid());
+
+        waitForNewTimestamp(".");
+
+        {
+            Internal::FileSaver fs(fn);
+            QVERIFY(fs.open());
+            QVERIFY(fs.device());
+            QVERIFY(fs.write("hello"));
+            QVERIFY(fs.commit());
+            QVERIFY(QFile::exists(fn));
+        }
+
+        const auto lm2 = QFileInfo(fn).lastModified();
+        QVERIFY(lm2.isValid());
+
+        QCOMPARE(lm, lm2); // timestamps should be the same since content did not change
+
+        waitForNewTimestamp(".");
+
+        {
+            Internal::FileSaver fs(fn);
+            QVERIFY(fs.open());
+            QVERIFY(fs.device());
+            QVERIFY(fs.write("hello2"));
+            QVERIFY(fs.commit());
+            QVERIFY(QFile::exists(fn));
+            QFile f(fn);
+            QVERIFY(f.open(QIODevice::ReadOnly));
+            QCOMPARE(f.readAll(), QByteArrayLiteral("hello2"));
+        }
+
+        const auto lm3 = QFileInfo(fn).lastModified();
+        QVERIFY(lm3.isValid());
+
+        QVERIFY(lm != lm3); // timestamps should differ since the content changed
+
+        waitForNewTimestamp(".");
+
+        {
+            // Test overwriteIfUnchanged
+            Internal::FileSaver fs(fn, true);
+            QVERIFY(fs.open());
+            QVERIFY(fs.device());
+            QVERIFY(fs.write("hello2"));
+            QVERIFY(fs.commit());
+            QVERIFY(QFile::exists(fn));
+            QFile f(fn);
+            QVERIFY(f.open(QIODevice::ReadOnly));
+            QCOMPARE(f.readAll(), QByteArrayLiteral("hello2"));
+        }
+
+        const auto lm4 = QFileInfo(fn).lastModified();
+        QVERIFY(lm4.isValid());
+
+        QVERIFY(lm3 != lm4); // timestamps should differ since we always overwrite
+    });
+}
+
 void TestTools::testFileInfo()
 {
     QCOMPARE(FileInfo::fileName("C:/waffl/copter.exe"), QString("copter.exe"));
diff --git a/tests/auto/tools/tst_tools.h b/tests/auto/tools/tst_tools.h
index e80d5fbf1..bd8538be2 100644
--- a/tests/auto/tools/tst_tools.h
+++ b/tests/auto/tools/tst_tools.h
@@ -56,7 +56,12 @@ public:
     TestTools(qbs::Settings *settings);
     ~TestTools();
 
+public slots:
+   virtual void initTestCase();
+
 private slots:
+    void fileSaver();
+
     void fileCaseCheck();
     void testBuildConfigMerging();
     void testFileInfo();
@@ -101,4 +106,6 @@ private:
 
     qbs::Settings * const m_settings;
     QList<QTemporaryDir *> m_tmpDirs;
+
+    const QString testDataDir;
 };