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

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);
 
@@ -1334,7 +1318,12 @@ bool Buffer::save() const
 		}
 	}
 
-	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;
 	}

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(),

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;

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

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;