Improve file saving strategy

- The TempFile class guarantees to generate a file name, we are not limited to
  100 tries of a predictable scheme anymore, which could break if LyX
  frequently crashes.
- The temp file name generation has no race condition against another LyX
  instance in the same directory anymore.
- Symlinks survive saving again (regression of 10364082c835).

diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 7bc75e4c6008d941dd03edcbc05eb38c9c27877f..866e4dcc3658c601750f327d684c3d48b63a29dd 100644 (file)
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -1284,25 +1284,8 @@ bool Buffer::save() const
        // we first write the file to a new name, then move it to its
        // proper location once that has been done successfully. that
        // way we preserve the original file if something goes wrong.
-       string const savepath = fileName().onlyPath().absFileName();
-       int fnum = 1;
-       string const fname = fileName().onlyFileName();
-       string savename = "tmp-" + convert<string>(fnum) + "-" + fname;
-       FileName savefile(addName(savepath, savename));
-       while (savefile.exists()) {
-               // surely that is enough tries?
-               if (fnum > 100) {
-                       Alert::error(_("Write failure"),
-                                    bformat(_("Cannot find temporary filename for:\n  %1$s.\n"
-                                        "Even %2$s exists!"),
-                                            from_utf8(fileName().absFileName()),
-                                from_utf8(savefile.absFileName())));
-                       return false;
-               }
-               fnum += 1;
-               savename = "tmp-" + convert<string>(fnum) + "-" + fname;
-               savefile.set(addName(savepath, savename));
-       }
+       TempFile tempfile(fileName().onlyPath(), "tmpXXXXXX.lyx");
+       FileName savefile(tempfile.name());
 
        LYXERR(Debug::FILES, "Saving to " << savefile.absFileName());
        if (!writeFile(savefile))
@@ -1310,6 +1293,7 @@ bool Buffer::save() const
 
        // we will set this to false if we fail
        bool made_backup = true;
+       bool const symlink = fileName().isSymLink();
        if (lyxrc.make_backup) {
                FileName backupName(absFileName() + '~');
                if (!lyxrc.backupdir_path.empty()) {
@@ -1321,7 +1305,7 @@ bool Buffer::save() const
 
                // Except file is symlink do not copy because of #6587.
                // Hard links have bad luck.
-               made_backup = fileName().isSymLink() ?
+               made_backup = symlink ?
                        fileName().copyTo(backupName):
                        fileName().moveTo(backupName);
 
@@ -1333,8 +1317,13 @@ bool Buffer::save() const
                        //LYXERR(Debug::DEBUG, "Fs error: " << fe.what());
                }
        }
-       
-       if (made_backup && savefile.moveTo(fileName())) {
+
+       // If we have no symlink, we can simply rename the temp file.
+       // Otherwise, we need to copy it so the symlink stays intact.
+       if (!symlink)
+               tempfile.setAutoRemove(false);
+       if (made_backup &&
+           (symlink ? savefile.copyTo(fileName(), true) : savefile.moveTo(fileName()))) {
                markClean();
                return true;
        }

diff --git a/src/support/FileName.cpp b/src/support/FileName.cpp
index 47d66699e73e5ccbde4a973bb854e1db25711aca..08d726e0c6347de476fa0950798ffa5527574b69 100644 (file)
--- a/src/support/FileName.cpp
+++ b/src/support/FileName.cpp
@@ -217,9 +217,26 @@ void FileName::erase()
 }
 
 
-bool FileName::copyTo(FileName const & name) const
+bool FileName::copyTo(FileName const & name, bool keepsymlink) const
 {
-       LYXERR(Debug::FILES, "Copying " << name);
+       FileNameSet visited;
+       visited.insert(*this);
+       return copyTo(name, keepsymlink, visited);
+}
+
+
+bool FileName::copyTo(FileName const & name, bool keepsymlink,
+                      FileName::FileNameSet & visited) const
+{
+       LYXERR(Debug::FILES, "Copying " << name << " keep symlink: " << keepsymlink);
+       if (keepsymlink && name.isSymLink()) {
+               FileName const target(fromqstr(name.d->fi.symLinkTarget()));
+               if (visited.find(target) != visited.end()) {
+                       LYXERR(Debug::FILES, "Found circular symlink: " << target);
+                       return false;
+               }
+               return copyTo(target, true);
+       }
        QFile::remove(name.d->fi.absoluteFilePath());
        bool success = QFile::copy(d->fi.absoluteFilePath(), name.d->fi.absoluteFilePath());
        if (!success)
@@ -231,6 +248,7 @@ bool FileName::copyTo(FileName const & name) const
 
 bool FileName::renameTo(FileName const & name) const
 {
+       LYXERR(Debug::FILES, "Renaming " << name);
        bool success = QFile::rename(d->fi.absoluteFilePath(), name.d->fi.absoluteFilePath());
        if (!success)
                LYXERR0("Could not rename file " << *this << " to " << name);
@@ -240,6 +258,7 @@ bool FileName::renameTo(FileName const & name) const
 
 bool FileName::moveTo(FileName const & name) const
 {
+       LYXERR(Debug::FILES, "Moving " << name);
        QFile::remove(name.d->fi.absoluteFilePath());
 
        bool success = QFile::rename(d->fi.absoluteFilePath(),

diff --git a/src/support/FileName.h b/src/support/FileName.h
index fe15163889ec1214fa1299b6692f3a7170ead170..1b37ef262902bc03033db74dfcc4ce387d624292 100644 (file)
--- a/src/support/FileName.h
+++ b/src/support/FileName.h
@@ -16,6 +16,7 @@
 #include "support/strfwd.h"
 
 #include <ctime>
+#include <set>
 
 
 namespace lyx {
@@ -123,7 +124,9 @@ public:
        /// \return true when file/directory is writable (write test file)
        /// \warning This methods has different semantics when system level
        /// copy command, it will overwrite the \c target file if it exists,
-       bool copyTo(FileName const & target) const;
+       /// If \p keepsymlink is true, the copy will be written to the symlink
+       /// target. Otherwise, the symlink will be destroyed.
+       bool copyTo(FileName const & target, bool keepsymlink = false) const;
 
        /// remove pointed file.
        /// \return true on success.
@@ -136,6 +139,8 @@ public:
        bool renameTo(FileName const & target) const;
 
        /// move pointed file to \param target.
+       /// If \p target exists it will be overwritten (if it is a symlink,
+       /// the symlink will be destroyed).
        /// \return true on success.
        bool moveTo(FileName const & target) const;
 
@@ -212,6 +217,12 @@ public:
 
 private:
        friend bool equivalent(FileName const &, FileName const &);
+       /// Set for tracking of already visited file names.
+       /// Uses operator==() (which may be case insensitive), and not
+       /// equvalent(), so that symlinks are not resolved.
+       typedef std::set<FileName> FileNameSet;
+       /// Helper for public copyTo() to find circular symlink chains
+       bool copyTo(FileName const &, bool, FileNameSet &) const;
        ///
        struct Private;
        Private * const d;

diff --git a/src/support/TempFile.cpp b/src/support/TempFile.cpp
index 5c4b389b8499c146fdf50fce3d610c132746788c..21559e144fe5418d245701dc6520468098ccdd49 100644 (file)
--- a/src/support/TempFile.cpp
+++ b/src/support/TempFile.cpp
@@ -74,5 +74,10 @@ FileName TempFile::name() const
 }
 
 
+void TempFile::setAutoRemove(bool autoremove)
+{
+       d->f.setAutoRemove(autoremove);
+}
+
 } // namespace support
 } // namespace lyx

diff --git a/src/support/TempFile.h b/src/support/TempFile.h
index c9342f91f2a6bfe38e4bd661f6087dce92827047..6410dda7fcee4e1e99195a51474575a444fab58f 100644 (file)
--- a/src/support/TempFile.h
+++ b/src/support/TempFile.h
@@ -46,6 +46,13 @@ public:
         * This is empty if the file could not be created.
         */
        FileName name() const;
+       /**
+        * Set whether the file should be automatically deleted in the
+        * destructor.
+        * Automatic deletion is the default, but it can be switched off if
+        * the file should be kept, because it should be renamed afterwards.
+        */
+       void setAutoRemove(bool autoremove);
 private:
        ///
        struct Private;