Truncate DocumentFile before writing to it

[amberin]

Before 43d88fd, this was not needed, because the file was always deleted and re-created first.

This should resolve issues #385 and #395.

[amberin]

Big thanks to @toridnyc and @Sergey-Makarov for reporting this serious issue. 🙏

[doak]

I've found it "by accident" because I followed the changelog out of curiosity. Indeed my files were already broken. Perhaps it would make sense to ask people to check their files explicitly.

[decadent]

@amberin Do I understand it right that the app opens the synced file directly? This still can lead to loss of on-disk data, e.g. if the system runs out of disk space right before the file is written, or if the phone is unexpectedly turned off. In desktop programming, this is usually circumvented by writing to a temporary file (on the same disk volume) and then renaming the file in place of the original — which is an atomic operation. If this can be done on Android, perhaps it's better to do it this way.

(Android might already provide a readymade solution for this, though — but idk if it does.)

[decadent]

Also, I second doak's comment that the changelog should be more explicit about the fact that data might've been lost — especially since the changelog is shown in the app itself. At least people could then check their backup copies or any copies preserved by their sync methods. Seeing as people using this app are likely somewhat versed in sync workflows, can fiddle with files, and are concerned about their notes.

[amberin]

@decadent No, Orgzly stores notebooks in its on-disk database, and they are exported as text files during sync.

@doak Orgzly's view of the file stays correct until you make remote changes in a corrupted file and sync them. That's of course pretty likely to happen, but if it hasn't, the next edit from Orgzly will remove the corruption.

Thankfully, the bug only meant that content at the end of a file was left in, when it should have been deleted. It looks scary, but nobody has actually lost information in a strict sense.

[decadent]

Orgzly stores notebooks in its on-disk database, and they are exported as text files during sync.

I understand that, I'm talking about when the app exports the file in syncing. The best practice is to not write directly into the existing file, since this operation can fail midway. Instead one should write a new file, ensure that no errors occurred, and then rename it in place of the original — this way only the directory's pointer to the file needs to be overwritten to finish the operation. (Though one issue with this is that the app should also copy the creation time from the original file, to preserve that metadata.)

[amberin]

@decadent PRs with improvements are always welcome. Yes, writing the file can fail midway but I consider it a pretty academic problem compared to this serious bug. But moving the new file in place should be best, I don't see anything preventing that off the top of my head. (When I introduced this bug, I replaced the previous logic which started by deleting the existing file...)

[decadent]

the bug only meant that content at the end of a file was left in, when it should have been deleted. It looks scary, but nobody has actually lost information in a strict sense.

I know I'm being somewhat annoying here, but: another argument for writing more explicitly about such issues in the changelog and the in-app changelog dialog is that currently, when people discover junk at the end of the file, left over from the untruncated writing, they won't know where it came from. Whereas explicit instructions could tell them that it's indeed junk and they're free to delete it.

Also btw, I just discovered that in the case of non-ascii utf-8 characters, they could be chopped off mid-sequence when overwritten, so that the remaining bytes aren't valid utf-8. Emacs itself falls back to loading such a file in an ascii mode with a binary representation of non-ascii characters, which looks kinda scary. And external tools can have some trouble with such files too.

[amberin]

another argument for writing more explicitly about such issues in the changelog and the in-app changelog dialog is that currently, when people discover junk at the end of the file, left over from the untruncated writing, they won't know where it came from. Whereas explicit instructions could tell them that it's indeed junk and they're free to delete it.

I completely agree. I might have added something like that had I fully grasped the consequences before I released the fix. I'll add a few words in the next release.

[decadent]

Perhaps a pinned issue could be made, explaining the happenings — and linked from the changelog dialog, if it allows links.