LyX 2.1.1

[Richard Heck]

With this patch in place, I would like to move toward a 2.1.1 release as
soon as possible. This does not fix the random crash people have been
seeing, but it should prevent really serious dataloss, and that is a
very good reason for a release.

What other bugs do people think we should try to fix before 2.1.1? Any?
Other than the one that is such a mystery?

Richard

commit 094129f804ad6f81de833d8957db42d0991b6882
Date: Tue Jun 3 10:42:07 2014 -0400
Per a suggestion of JMarc's, first write the saved file to a
temporary name, then move it to its real location if we succeed.
This prevents our over-writing the existing file with a corrupt
one.
(cherry picked from commit 10364082c8351b0f4a258699742370195aadde6b)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index ab2164b..8fab72d 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -1276,12 +1276,38 @@ bool Buffer::save() const
// We don't need autosaves in the immediate future. (Asger)
resetAutosaveTimers();
- FileName backupName;
- bool madeBackup = false;
+ // 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();
+ string savename = "tmp-" + fileName().onlyFileName();
+ FileName savefile(addName(savepath, savename));
+ while (savefile.exists()) {
+ if (savefile.absFileName().size() > 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(savefile.absFileName())));
+ return false;
+ }
+ savename = "tmp-" + savename;
+ savefile.set(addName(savepath, savename));
+ }
+
+ LYXERR(Debug::FILES, "Saving to " << savefile.absFileName());
+ if (!writeFile(savefile))
+ return false;
- // make a backup if the file already exists
- if (lyxrc.make_backup && fileName().exists()) {
- backupName = FileName(absFileName() + '~');
+ // we will set this to false if we fail
+ bool made_backup = true;
+ if (lyxrc.make_backup) {
+ FileName backupName(absFileName() + '~');
if (!lyxrc.backupdir_path.empty()) {
string const mangledName =
subst(subst(backupName.absFileName(), '/', '!'), ':', '!');
@@ -1291,12 +1317,11 @@ bool Buffer::save() const
// 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);
+ made_backup = fileName().isSymLink() ?
+ fileName().moveTo(backupName);
- if (!madeBackup) {
+ if (!made_backup) {
Alert::error(_("Backup failure"),
bformat(_("Cannot create backup file %1$s.\n"
"Please check whether the directory exists and is writable."),
@@ -1304,16 +1329,18 @@ bool Buffer::save() const
//LYXERR(Debug::DEBUG, "Fs error: " << fe.what());
}
}
-
- if (writeFile(d->filename)) {
+
+ if (made_backup && savefile.moveTo(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 a6a27aa..74bb25a 100644
--- a/status.21x
+++ b/status.21x
@@ -28,6 +28,9 @@ 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.
* TEX2LYX IMPROVEMENTS

[Richard Heck]

Post by Richard Heck
With this patch in place, I would like to move toward a 2.1.1 release
as soon as possible. This does not fix the random crash people have
been seeing, but it should prevent really serious dataloss, and that
is a very good reason for a release.
What other bugs do people think we should try to fix before 2.1.1?
Any? Other than the one that is such a mystery?

PS It'd be great if people could test this.

Post by Richard Heck
Richard

commit 094129f804ad6f81de833d8957db42d0991b6882
Date: Tue Jun 3 10:42:07 2014 -0400
Per a suggestion of JMarc's, first write the saved file to a
temporary name, then move it to its real location if we succeed.
This prevents our over-writing the existing file with a corrupt
one.
(cherry picked from commit
10364082c8351b0f4a258699742370195aadde6b)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index ab2164b..8fab72d 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -1276,12 +1276,38 @@ bool Buffer::save() const
// We don't need autosaves in the immediate future. (Asger)
resetAutosaveTimers();
- FileName backupName;
- bool madeBackup = false;
+ // 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();
+ string savename = "tmp-" + fileName().onlyFileName();
+ FileName savefile(addName(savepath, savename));
+ while (savefile.exists()) {
+ if (savefile.absFileName().size() > 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(savefile.absFileName())));
+ return false;
+ }
+ savename = "tmp-" + savename;
+ savefile.set(addName(savepath, savename));
+ }
+
+ LYXERR(Debug::FILES, "Saving to " << savefile.absFileName());
+ if (!writeFile(savefile))
+ return false;
- // make a backup if the file already exists
- if (lyxrc.make_backup && fileName().exists()) {
- backupName = FileName(absFileName() + '~');
+ // we will set this to false if we fail
+ bool made_backup = true;
+ if (lyxrc.make_backup) {
+ FileName backupName(absFileName() + '~');
if (!lyxrc.backupdir_path.empty()) {
string const mangledName =
subst(subst(backupName.absFileName(), '/', '!'), ':', '!');
@@ -1291,12 +1317,11 @@ bool Buffer::save() const
// 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);
+ made_backup = fileName().isSymLink() ?
+ fileName().moveTo(backupName);
- if (!madeBackup) {
+ if (!made_backup) {
Alert::error(_("Backup failure"),
bformat(_("Cannot create backup file %1$s.\n"
"Please check whether the directory
exists and is writable."),
@@ -1304,16 +1329,18 @@ bool Buffer::save() const
//LYXERR(Debug::DEBUG, "Fs error: " << fe.what());
}
}
-
- if (writeFile(d->filename)) {
+
+ if (made_backup && savefile.moveTo(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 a6a27aa..74bb25a 100644
--- a/status.21x
+++ b/status.21x
@@ -28,6 +28,9 @@ 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.
* TEX2LYX IMPROVEMENTS

[Jean-Marc Lasgouttes]

Post by Richard Heck
With this patch in place, I would like to move toward a 2.1.1 release as
soon as possible. This does not fix the random crash people have been
seeing, but it should prevent really serious dataloss, and that is a
very good reason for a release.
What other bugs do people think we should try to fix before 2.1.1? Any?
Other than the one that is such a mystery?

We could also add some os <<endl (or os << flush) statements in some
strategic points of the write() functions (aftr each inset and each
paragraph, for example). This would reduce a tiny bit data loss, and
maybe point to something closer to the real culprit.

JMarc

[Richard Heck]

Post by Jean-Marc Lasgouttes

Post by Richard Heck
With this patch in place, I would like to move toward a 2.1.1 release as
soon as possible. This does not fix the random crash people have been
seeing, but it should prevent really serious dataloss, and that is a
very good reason for a release.
What other bugs do people think we should try to fix before 2.1.1? Any?
Other than the one that is such a mystery?

We could also add some os <<endl (or os << flush) statements in some
strategic points of the write() functions (aftr each inset and each
paragraph, for example). This would reduce a tiny bit data loss, and
maybe point to something closer to the real culprit.
JMarc

[Richard Heck]

Post by Jean-Marc Lasgouttes

Post by Richard Heck
With this patch in place, I would like to move toward a 2.1.1 release as
soon as possible. This does not fix the random crash people have been
seeing, but it should prevent really serious dataloss, and that is a
very good reason for a release.
What other bugs do people think we should try to fix before 2.1.1? Any?
Other than the one that is such a mystery?

We could also add some os <<endl (or os << flush) statements in some
strategic points of the write() functions (aftr each inset and each
paragraph, for example). This would reduce a tiny bit data loss, and
maybe point to something closer to the real culprit.

This would do something like that:

diff --git a/src/Paragraph.cpp b/src/Paragraph.cpp
index 75f0171..0c1fe67 100644
--- a/src/Paragraph.cpp
+++ b/src/Paragraph.cpp
@@ -1669,6 +1669,7 @@ void Paragraph::write(ostream & os, BufferParams
const & b
os << "\\begin_inset ";
inset->write(os);
os << "\n\\end_inset\n\n";
+ os << flush;
column = 0;
}
}
@@ -1706,6 +1707,7 @@ void Paragraph::write(ostream & os, BufferParams
const & b

flushString(os, write_buffer);
os << "\n\\end_layout\n";
+ os << flush;
}

OK?

Richard

[Jean-Marc Lasgouttes]

Indeed. Maybe also one after each cell inset in Tabular::write?

JMarc

Post by Richard Heck
diff --git a/src/Paragraph.cpp b/src/Paragraph.cpp
index 75f0171..0c1fe67 100644
--- a/src/Paragraph.cpp
+++ b/src/Paragraph.cpp
@@ -1669,6 +1669,7 @@ void Paragraph::write(ostream & os, BufferParams
const & b
os << "\\begin_inset ";
inset->write(os);
os << "\n\\end_inset\n\n";
+ os << flush;
column = 0;
}
}
@@ -1706,6 +1707,7 @@ void Paragraph::write(ostream & os, BufferParams
const & b
flushString(os, write_buffer);
os << "\n\\end_layout\n";
+ os << flush;
}
OK?
Richard

[Richard Heck]

Post by Jean-Marc Lasgouttes
Indeed. Maybe also one after each cell inset in Tabular::write?

Done, and committed.

rh

Post by Jean-Marc Lasgouttes
JMarc

Post by Richard Heck
diff --git a/src/Paragraph.cpp b/src/Paragraph.cpp
index 75f0171..0c1fe67 100644
--- a/src/Paragraph.cpp
+++ b/src/Paragraph.cpp
@@ -1669,6 +1669,7 @@ void Paragraph::write(ostream & os, BufferParams
const & b
os << "\\begin_inset ";
inset->write(os);
os << "\n\\end_inset\n\n";
+ os << flush;
column = 0;
}
}
@@ -1706,6 +1707,7 @@ void Paragraph::write(ostream & os, BufferParams
const & b
flushString(os, write_buffer);
os << "\n\\end_layout\n";
+ os << flush;
}
OK?
Richard

[Jean-Marc Lasgouttes]

Post by Richard Heck
With this patch in place, I would like to move toward a 2.1.1 release as
soon as possible. This does not fix the random crash people have been
seeing, but it should prevent really serious dataloss, and that is a
very good reason for a release.
What other bugs do people think we should try to fix before 2.1.1? Any?
Other than the one that is such a mystery?

I have the fix to 9142 that I'd like to apply. I'll do it tomorrow.

JMarc

[Richard Heck]

Post by Jean-Marc Lasgouttes

Post by Richard Heck
With this patch in place, I would like to move toward a 2.1.1 release as
soon as possible. This does not fix the random crash people have been
seeing, but it should prevent really serious dataloss, and that is a
very good reason for a release.
What other bugs do people think we should try to fix before 2.1.1? Any?
Other than the one that is such a mystery?

I have the fix to 9142 that I'd like to apply. I'll do it tomorrow.

OK.

rh

[Georg Baum]

Post by Richard Heck
With this patch in place, I would like to move toward a 2.1.1 release as
soon as possible. This does not fix the random crash people have been
seeing, but it should prevent really serious dataloss, and that is a
very good reason for a release.

I agree.

Post by Richard Heck
What other bugs do people think we should try to fix before 2.1.1? Any?
Other than the one that is such a mystery?

I am not aware of any. There are a number of other problems, but those can
wait.

However, this patch needs very good testing during the next days, because it
has the potential to make the situation even worse. I'll also try to look
carefully over it when I find some time.


Georg

[Enrico Forestieri]

+ // 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();
+ string savename = "tmp-" + fileName().onlyFileName();
+ FileName savefile(addName(savepath, savename));
+ while (savefile.exists()) {
+ if (savefile.absFileName().size() > 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(savefile.absFileName())));
+ return false;
+ }
+ savename = "tmp-" + savename;
+ savefile.set(addName(savepath, savename));
+ }

Note that you may have troubles on Windows if the file name is longer
than 260 characters. I'd suggest changing the scheme for choosing the
temporary name by trying the sequence "tmp1-...", "tmp2-..." and so on.

[Richard Heck]

Post by Enrico Forestieri

+ // 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();
+ string savename = "tmp-" + fileName().onlyFileName();
+ FileName savefile(addName(savepath, savename));
+ while (savefile.exists()) {
+ if (savefile.absFileName().size() > 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(savefile.absFileName())));
+ return false;
+ }
+ savename = "tmp-" + savename;
+ savefile.set(addName(savepath, savename));
+ }

Note that you may have troubles on Windows if the file name is longer
than 260 characters. I'd suggest changing the scheme for choosing the
temporary name by trying the sequence "tmp1-...", "tmp2-..." and so on.

Will do.

rh

[Peter Kümmel]

With this patch in place, I would like to move toward a 2.1.1 release as soon as possible. This does not fix the random
crash people have been seeing, but it should prevent really serious dataloss, and that is a very good reason for a release.
What other bugs do people think we should try to fix before 2.1.1? Any? Other than the one that is such a mystery?
Richard

What about showing a backtrace in the crash dialog (on Linux)?
See PATCH.

Peter