The attached patches (i) revert the previous data saving patch for
stable and (ii) implement a different, and safer, approach. There are
some issues here (e.g, with symlinks), but these already exist in the
code that makes user-requested backups. The only real change is to the
code that figures out to what name we should backup the original file.
So this seems about as safe as it could be.
The reason we need this patch for 2.1.x is that the more sophisticated
approach needs more refinement, and testing, that it can get before 2.1.1.
Here's a summary of what this patch does.
If the user has not requested that we backup the original file, we do so
anyway, not to file.lyx~ but to a temporary name of our own creation,
lyxbak-NUM-file.lyx, where NUM is some number we find that gives us a
non-existent file. Note that this filename depends upon which file we
are saving, and we try NUM up to 1024. So we should always be able to
find such a name, unless something very bad has happened.
If we are unable to backup the original file, we issue a warning and ask
the user if s'he wants to continue.
If the file is saved successfully, then we delete our temporary backup
file.
If the file is not saved successfully, then we attempt to move the
backup file back to the original location. If that fails, then we inform
the user that the original file can still be found at a new location.
I put a "return false" at the beginning of Buffer::writeFile() and
verified that the original file is properly restored. Since we move the
file back and forth unless it is a symlink, this should preserve
permissions as well.
Comments?
Richard
>From ff425b1597b8437d49498dc035345888f1f2aac3 Mon Sep 17 00:00:00 2001
From: Richard Heck <rgh...@lyx.org>
Date: Mon, 9 Jun 2014 15:28:56 -0400
Subject: [PATCH 1/2] Revert "Use a different naming scheme, per Enrico's
suggestion." This reverts commit
fff454fa4bde75ca3554fc4b22cfcad6f36b026d.
Revert "Per a suggestion of JMarc's, first write the saved file to a"
This reverts commit 094129f804ad6f81de833d8957db42d0991b6882.
---
src/Buffer.cpp | 65 +++++++++++++++-------------------------------------------
status.21x | 3 ---
2 files changed, 17 insertions(+), 51 deletions(-)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 867eddd..ab2164b 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -1276,42 +1276,12 @@ bool Buffer::save() const
// We don't need autosaves in the immediate future. (Asger)
resetAutosaveTimers();
- // if the file does not yet exist, none of the backup activity
- // that follows is necessary
- if (!fileName().exists())
- return writeFile(fileName());
-
- // 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));
- }
-
- LYXERR(Debug::FILES, "Saving to " << savefile.absFileName());
- if (!writeFile(savefile))
- return false;
+ FileName backupName;
+ bool madeBackup = false;
- // we will set this to false if we fail
- bool made_backup = true;
- if (lyxrc.make_backup) {
- FileName backupName(absFileName() + '~');
+ // make a backup if the file already exists
+ if (lyxrc.make_backup && fileName().exists()) {
+ backupName = FileName(absFileName() + '~');
if (!lyxrc.backupdir_path.empty()) {
string const mangledName =
subst(subst(backupName.absFileName(), '/', '!'), ':', '!');
@@ -1321,11 +1291,12 @@ bool Buffer::save() const
// Except file is symlink do not copy because of #6587.
// Hard links have bad luck.
- made_backup = fileName().isSymLink() ?
- fileName().copyTo(backupName):
- fileName().moveTo(backupName);
+ if (fileName().isSymLink())
+ madeBackup = fileName().copyTo(backupName);
+ else
+ madeBackup = fileName().moveTo(backupName);
- if (!made_backup) {
+ if (!madeBackup) {
Alert::error(_("Backup failure"),
bformat(_("Cannot create backup file %1$s.\n"
"Please check whether the directory exists and is writable."),
@@ -1333,18 +1304,16 @@ bool Buffer::save() const
//LYXERR(Debug::DEBUG, "Fs error: " << fe.what());
}
}
-
- if (made_backup && savefile.moveTo(fileName())) {
+
+ if (writeFile(d->filename)) {
markClean();
return true;
+ } else {
+ // Saving failed, so backup is not backup
+ if (madeBackup)
+ backupName.moveTo(d->filename);
+ return false;
}
- // else
- Alert::error(_("Write failure"),
- bformat(_("Cannot move saved file to:\n %1$s.\n"
- "But the file has successfully been saved as:\n %2$s."),
- from_utf8(fileName().absFileName()),
- from_utf8(savefile.absFileName())));
- return false;
}
diff --git a/status.21x b/status.21x
index c86f5da..6c8e8b2 100644
--- a/status.21x
+++ b/status.21x
@@ -28,9 +28,6 @@ What's new
* DOCUMENT INPUT/OUTPUT
-- When saving a file, LyX now writes the saved file first to a temporary
- filename (tmp-oldfile.lyx) and only deletes the original file once the
- new file has successfully been written.
- We now flush the output stream more frequently, as a temporary measure
to help us gather information about the crash mentioned above.
--
1.7.11.7
>From 4088931cdc863625230f3ff9aec0442e81686ae0 Mon Sep 17 00:00:00 2001
From: Richard Heck <rgh...@lyx.org>
Date: Mon, 9 Jun 2014 17:35:17 -0400
Subject: [PATCH 2/2] Backup the existing LyX file before attempting to write
the new one. This avoids dataloss in case we are unable
to write the new one after all.
A more sophisticated approach, due to Georg, is in master, but it needs
more testing that it will be able to get before the release of 2.1.1.
That should be committed to 2.1.x when it is ready and this patch backed
out again.
---
src/Buffer.cpp | 92 +++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 69 insertions(+), 23 deletions(-)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index ab2164b..dcd74bc 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -1276,44 +1276,90 @@ bool Buffer::save() const
// We don't need autosaves in the immediate future. (Asger)
resetAutosaveTimers();
+ // none of the backup activity that follows is necessary if the
+ // file does not exist
+ if (!fileName().exists())
+ return writeFile(fileName());
+
+ // this will hold the name of the location to which we backup
+ // the existing file, if we do that.
FileName backupName;
- bool madeBackup = false;
// make a backup if the file already exists
- if (lyxrc.make_backup && fileName().exists()) {
+ if (lyxrc.make_backup) {
backupName = FileName(absFileName() + '~');
if (!lyxrc.backupdir_path.empty()) {
string const mangledName =
subst(subst(backupName.absFileName(), '/', '!'), ':', '!');
backupName = FileName(addName(lyxrc.backupdir_path,
- mangledName));
- }
-
- // Except file is symlink do not copy because of #6587.
- // Hard links have bad luck.
- if (fileName().isSymLink())
- madeBackup = fileName().copyTo(backupName);
- else
- madeBackup = fileName().moveTo(backupName);
-
- if (!madeBackup) {
- Alert::error(_("Backup failure"),
- bformat(_("Cannot create backup file %1$s.\n"
- "Please check whether the directory exists and is writable."),
- from_utf8(backupName.absFileName())));
- //LYXERR(Debug::DEBUG, "Fs error: " << fe.what());
+ mangledName));
}
+ } else {
+ // we make a backup anyway, to avoid over-writing the original file
+ // before the new one has been saved.
+ //
+ // FIXME A more sophisticated approach is in master and should
+ // be moved into 2.1.x once it has been properly tested.
+ string const savepath = fileName().onlyPath().absFileName();
+ string const fname = fileName().onlyFileName();
+ string savename;
+ int fnum = 0;
+
+ // try to find a temporary filename to which we can backup the
+ // existing LyX file
+ do {
+ fnum += 1;
+ if (fnum > 1024) {
+ Alert::error(_("Write failure"),
+ bformat(_("Cannot find temporary filename for:\n %1$s.\n"
+ "Even %2$s exists!"),
+ from_utf8(fileName().absFileName()),
+ from_utf8(backupName.absFileName())));
+ return false;
+ }
+ savename = "lyxbak-" + convert<string>(fnum) + "-" + fname;
+ backupName.set(addName(savepath, savename));
+ } while (backupName.exists());
+ }
+
+ LYXERR(Debug::FILES, "Backup being made at " << backupName);
+ // We make the backup by moving the original file unless it is
+ // a symlink, in which case we copy it because of #6587.
+ // Hard links are broken: The new file we write will not be hard
+ // linked as the original file was. Sorry!
+ bool const madeBackup = fileName().isSymLink() ?
+ fileName().copyTo(backupName) :
+ fileName().moveTo(backupName);
+
+ if (!madeBackup) {
+ int const ret = Alert::prompt(_("Backup failure"),
+ bformat(_("Cannot create backup file:\n %1$s.\n"
+ "Do you want to try to save the file anyway?\n"
+ "This will over-write the original file."),
+ from_utf8(backupName.absFileName())),
+ 1, 1, _("&Overwrite"), _("&Cancel"));
+ if (ret != 0)
+ return false;
}
if (writeFile(d->filename)) {
markClean();
+ if (madeBackup && !lyxrc.make_backup)
+ backupName.removeFile();
return true;
- } else {
- // Saving failed, so backup is not backup
- if (madeBackup)
- backupName.moveTo(d->filename);
- return false;
}
+
+ // else saving failed, so backup is not backup
+ if (madeBackup) {
+ if (!backupName.moveTo(d->filename)) {
+ Alert::error(_("Write failure"),
+ bformat(_("Cannot restore original file to:\n %1$s.\n"
+ "But it can be found at:\n %2$s."),
+ from_utf8(fileName().absFileName()),
+ from_utf8(backupName.absFileName())));
+ }
+ }
+ return false;
}
--
1.7.11.7
