Undo unrecoverably deletes files created by other programs in a directory created by Thunar

Today I discovered the recently added to Thunar Undo/Redo feature in an unpleasant way:

• created a directory in Thunar;
• let an external program create files in this directory;
• pressed Ctrl+Z while Thunar had focus thinking that another application has focus;
• noticed the desktop notification and realized that all files created by the external program are unrecoverably lost.

Undoing the Create-Folder action should behave as rmdir not as rm -r. That is, the undoing should fail if there is at least one file in the directory.

---

A similar issue exists with undoing file creation:

• create a file;
• write some text and save the file;
• Undo in Thunar;
• Redo in Thunar.

Actual result: the file is empty and the Redo action is disabled.

Expected result: Thunar refuses to delete the file if its contents differs from what was originally created (the original contents can be nonempty if the file is created from a template).

---

Other critical bugs can exist in the Undo/Redo feature. I recommend to consider carefully what else might go wrong and fix it rather than wait for another data-loss bug report.

---

Workaround: remove shortcuts for Undo and Redo actions in Thunar's Preferences => Shortcuts tab. I'll keep this workaround even after the bugs are fixed, because other bugs are possible (now or in the future). Furthermore, accidental filesystem changes can go unnoticed until another operation is performed in Thunar or Thunar exits. And then these filesystem changes can no longer be reverted (redone).

I advise removing these default shortcuts from Thunar code. The tiny minority of users, who use this feature so often that the main menu is cumbersome, can assign shortcuts manually. At the very least the default shortcuts should be removed until these bugs are fixed.

Thank you for reporting, and sorry for the inconvenience !

That is indeed a use-case which was missed during testing the undo/redo feature … though, should be rather simple to fix.

added 5. File Operations label

added 5. Undo / Redo label

mentioned in commit a038f5ed

mentioned in commit d6d51d3c

changed milestone to %4.18.4

Fixed for master and 4.18 branch, to be released in 4.18.4.

If any of the files to be undone was changed meanwhile, a dialog will be shown in order to ask if you are sure that the file(s) should be permanently deleted.

If possible, it would be great if you could give it some more testing before it gets released.

closed

I'll test the fixes once satisfied with how the code aims to work. See my comments on a038f5ed.

What do you think about removing the default Undo/Redo shortcuts? Do you think the need to undo/redo filesystem operations is so common that using the main menu is inconvenient?

Just checked how this feature works in Dolphin. Dolphin has no Redo, only Undo. Undo has Ctrl+Z shortcut assigned by default. Dolphin always asks before removing a file or directory, but provides the Do not ask again checkbox unchecked by default. Even if Dolhpin asks before deleting a dir it has created, the deletion of a nonempty dir fails with an error message displayed under the toolbar.

After thinking a bit about it, maybe I was a bit quick with fixing the issue in the way I did. There is as well some other possibility: Don't delete the file, but move it to trash instead.

It would require some deeper changes, since the original operation would need to be changed to be a "restore from trash" operation instead of a "create file" operation, so that it could be "redone". Though, it would make it possible to even restore modified files.

It looks like Caja does something similar, though it seems not to use trash as cache location.

However, I suppose it could be a bit too much of a change to release it on the stable 4.18 branch .. will tinker a bit to see how it would look like.

What do you think about removing the default Undo/Redo shortcuts? Do you think the need to undo/redo filesystem operations is so common that using the main menu is inconvenient?

Since it is possible to just fix the problem instead, I don't think it will be required to remove the shortcuts. I think having these shortcuts is not the problem … even when disabled, you can suffer from the issue you described. Offering undo while not binding it to CTRL+Z just asks for new issue-reports.

reopened

mentioned in merge request !332 (closed)

mentioned in merge request !333 (closed)

Trashing a file can result in unexpected leftover files in Trash. Also Trash does not work on all disks, for example, with this error message: "Unable to trash file <file name> across filesystem boundaries.", "Do you want to permanently delete it?".

I think Thunar should absolutely refuse to remove a directory with files in it as an Undo or Redo step, like Dolphin does. Thunar never creates a directory with files in it, does it?

I think having these shortcuts is not the problem … even when disabled, you can suffer from the issue you described.

But the chance of accidentally triggering an action in the main menu is much smaller than pressing the ubiquitous keyboard shortcut with close-by keys and likely in many users' muscle memory.

Offering undo while not binding it to CTRL+Z just asks for new issue-reports.

The shortcuts could be removed temporarily until the Undo/Redo functionality is better tested, becomes safe and robust. The two of us have little hope of achieving this in a few days.

Just found another bug while trying to check if undoing a trashing of a file silently overwrites a same-name file created by an external program in place of the trashed file (this correctly fails in Dolphin with the error "A file named <...> already exists." by the way):

• Trash a file or a dir. The Undo action doesn't restore it back from Trash despite the message "Undo performed", "Trash operation was undone".

And one more bug:

1. Copy a dir that contains a subdir.
2. Create a file in the subdir of the copy of the dir via an external program.
3. Undo in Thunar removes the copy of the dir recursively.

If undoing a copy goes through the same code path as undoing creating a file and a dir, then the timestamp check won't help in this case, because creating a file in a subdir does not affect the dir's last-modified timestamp. If, on the other hand, timestamps are checked for all files in a dir recursively, then the warning will always be shown, even if files in the copied dir weren't modified, because their last-modified timestamps are not changed when the dir is copied. Dolphin shows a warning in this case: "Do you really want to permanently delete these 4 items?", "This action cannot be undone.", <the recursive list of all file paths within the copy of the dir>.

I think Thunar should absolutely refuse to remove a directory with files in it as an Undo or Redo step, like Dolphin does. Thunar never creates a directory with files in it, does it?

Yes, it does. E.g. on a copy operation. Undo of 'copy' as well is 'delete'.

WRT not having STRG+Z bound, your arguments don't change my POV, I won't change the default keybind for it, but rather fix the bugs, no matter if that takes days or weeks.

Just found another bug while trying to check if undoing a trashing of a file silently overwrites a same-name file created by an external program in place of the trashed file (this correctly fails in Dolphin with the error "A file named <...> already exists." by the way):

• Trash a file or a dir. The Undo action doesn't restore it back from Trash despite the message "Undo performed", "Trash operation was undone".

Can you please open a separate issue for this ? One thing at a time. Trash/Restore is a different story.

And one more bug:

1. Copy a dir that contains a subdir.
2. Create a file in the subdir of the copy of the dir via an external program.
3. Undo in Thunar removes the copy of the dir recursively.

If undoing a copy goes through the same code path as undoing creating a file and a dir, then the timestamp check won't help in this case, because creating a file in a subdir does not affect the dir's last-modified timestamp. If, on the other hand, timestamps are checked for all files in a dir recursively, then the warning will always be shown, even if files in the copied dir weren't modified, because their last-modified timestamps are not changed when the dir is copied. Dolphin shows a warning in this case: "Do you really want to permanently delete these 4 items?", "This action cannot be undone.", .

Yes, the inverse of copy will use the same code path than the inverse of file creation. As well, a separate issue. (I suppose the same than mentioned in a038f5ed (comment 64999) .. so possibly I realy need to cast that on the 4.18 branch)

Yes, it does. E.g. on a copy operation. Undo of 'copy' as well is 'delete'.

Would be nice to distinguish undoing directory creation and copying in order to never remove files within a created directory (rmdir non-recursively), like Dolphin does.

Can you please open a separate issue for this ? One thing at a time. Trash/Restore is a different story.

Done: #1030 (closed).

Yes, the inverse of copy will use the same code path than the inverse of file creation. As well, a separate issue. (I suppose the same than mentioned in a038f5ed (comment 64999) .. so possibly I realy need to cast that on the 4.18 branch)

The issue is not the same as the one mentioned in a comment to that commit. This issue shows that checking timestamps is not a generally applicable solution, because it cannot work correctly for undoing copying a directory (unless the complete recursive file list along with timestamps for each file is stored and compared). Perhaps it would be better (certainly simpler) to not check timestamps at all, but present the user with the complete list of files that will be removed in a confirmation dialog, like Doplhin does.

Another downside of relying on timestamps is that some programs intentionally don't change last-modified timestamps. For example, GNU recode:

--touch The touch option is meaningful only when files are recoded over themselves. Without it, the time-stamps associated with files are preserved, to reflect the fact that changing the code of a file does not really alter its informational contents. When the user wants the recoded files to be time-stamped at the recoding time, this option inhibits the automatic protection of the time-stamps.

Would be nice to distinguish undoing directory creation and copying in order to never remove files within a created directory (rmdir non-recursively), like Dolphin does.

Yea, could probably be done. Though, that only would relax the 'create folder' use case.

Done: #1030 (closed).

Thanks !

The issue is not the same as the one mentioned in a comment to that commit. This issue shows that checking timestamps is not a generally applicable solution, because it cannot work correctly for undoing copying a directory (unless the complete recursive file list along with timestamps for each file is stored and compared). Perhaps it would be better (certainly simpler) to not check timestamps at all, but present the user with the complete list of files that will be removed in a confirmation dialog, like Doplhin does.

I think at least for some simple cases (create file/folder) it will be fine to rely on timestamps. Though yea, if it is about huge folder-structures, I suppose we don't want to maintain a huge collection of modification stamps and check each subfolder for possible new files. Maybe by using a checksum would help, though that can be rather time-intensive.

So yea, think I will do the following:

4.18 branch: Pragmatically, just always ask before deleting anything via undo.

master: Like 4.18, but add some exceptions for file/folder creation

I think at least for some simple cases (create file/folder) it will be fine to rely on timestamps.

• Create folder: no need for timestamps if only an empty dir is deleted.
• Create file: can instead remember the file's origin. Either an empty file or a path to a file template. Then on Undo check the file's contents: either empty or same as the template's. Compare the template's and file's sizes first, then, if the sizes are equal, compare checksums. File templates should be almost always small, so computing these checksums should be fast.

Timestamps have several implementation difficulties and issues discussed under a038f5ed, plus some programs don't update them (e.g. GNU recode mentioned above). So not relying on timestamps at all would be safer, not to mention simpler.

mentioned in commit cfe98e0b

mentioned in commit 795d3f2c

mentioned in commit 91bc7311

mentioned in commit 00122061

Thinking once more about it, I suppose even for empty folders and files which were not modified, it might make sense to ask before deletion.

• The user might have set special user/group permissions, which would be gone
• If done unnoticed (like in your initial use-case), it might be confusing on a later session

So IMO good enough now (always ask before deletion by undo).

closed

The user might have set special user/group permissions, which would be gone

This is a very minor concern. Reassigning permissions to a single dir or file is trivial.

If done unnoticed (like in your initial use-case), it might be confusing on a later session

That's right.

So IMO good enough now (always ask before deletion by undo).

Acceptable. Not removing a created dir recursively (like rmdir) would be an additional safety net for a relatively likely data loss scenario.

When I cancel deletion of the Folder via Undo by clicking Cancel in the Are you sure... dialog, a highly misleading notification appears: Undo performed, Create Folder operation was undone, even though the folder remains on disk.

True, though, it's the same for multiple occasions of failed undo/redo, not only file/folder creation. Could you please open a fresh bug for it ?

I just did so: #1036

reopened

closed