Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn against overwriting directories #12654

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mandx
Copy link
Contributor

@mandx mandx commented Jan 23, 2025

Warn against overwriting directories if the destination path doesn't end with a directory separator.

  • Create a directory test-dir and a test-file.txt inside (so we end with test-dir/test-file.txt)
  • Open Helix in the current directory
  • Use :n to create a new buffer and add some content to it
  • Use :w test-dir to save the new buffer (note the directory name is used without the trailing slash)
  • Observe that now the buffer was correctly saved to test-dir as a file
  • However the directory itself was not lost, but renamed with a temporary/backup name (something like test-dirp4F0MM.bck/).

While not a critical data loss bug (the backup-rename of the original directory happens every time) it would be nice to prevent this situation in the first place, very much like we warn against external modifications by checking modification time.

@kirawi
Copy link
Member

kirawi commented Jan 25, 2025

It would be good to have a test for this in https://github.com/helix-editor/helix/blob/master/helix-term/tests/test/commands/write.rs

@mandx
Copy link
Contributor Author

mandx commented Jan 26, 2025

It would be good to have a test for this in master/helix-term/tests/test/commands/write.rs

@kirawi I just added one; I copied the test_overwrite_protection test and tweaked just enough for what we want to test.

However I didn't find a way to check for that the warning message is displayed; I see that app.editor has a status_msg field of type Option<(Cow<'static, str>, Severity)> but in the test I always get None...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants