Skip to content

Simplify saving logic and make it well behaved - #13916

Closed
RGBCube wants to merge 4 commits into
helix-editor:masterfrom
cull-os:correct-saving
Closed

Simplify saving logic and make it well behaved#13916
RGBCube wants to merge 4 commits into
helix-editor:masterfrom
cull-os:correct-saving

Conversation

@RGBCube

@RGBCube RGBCube commented Jul 8, 2025

Copy link
Copy Markdown

Previously, Helix did the following when we saved a file:

  1. Created a .bck (backup) file, copied the original's contents over.
  2. Re-created the original, empty this time. Then wrote to it.
  3. If writing or creating failed, it moved the backup back into the original file.
    3.1. Though, it did different stuff for hardlinks. Also not ideal.

The issue here is that if helix crashes / gets killed while executing
step 2 or 3, the file would be in an inconsistent state. This is not ideal
at all, file writing isn't atomic with this strategy.

This patch simplifies this logic into:

  • Create a temporary file, write the stuff we want to write into it.
  • If that fails, we don't need to do anything as the original file is
    untouched. (Though we do delete that tmp file if writing to it or moving it in-place fails)
  • If it is successful, we rename the temporary file over the actual
    write destionation. This is atomic, and if it fails the destination is
    unmodified.

There are also other benefits from doing the latter, such as inotify.
Previously, we would get these events for an edit of .cargo/config.toml:

Create /Users/pala/Projects/helix/.cargo/config.toml.bck
Write /Users/pala/Projects/helix/.cargo/config.toml.bck
Create /Users/pala/Projects/helix/.cargo/config.toml
Write /Users/pala/Projects/helix/.cargo/config.toml
Remove /Users/pala/Projects/helix/.cargo/config.toml.bck

Now, we get these:

Create /Users/pala/Projects/helix/.cargo/config.toml.tmp
Write /Users/pala/Projects/helix/.cargo/config.toml.tmp
Write /Users/pala/Projects/helix/.cargo/config.toml

This is better and easier to handle, because inotify events
actually look like we are writing to the target file, instead of
re-creating it.

RGBCube added 3 commits July 8, 2025 16:07
Previously, Helix did the following when we saved a file:

1. Created a .bck (backup) file, copied the original's contents over.
2. Re-created the original, empty this time. Then wrote to it.
3. If writing or creating failed, it moved the backup back into the original file.
  3.1. Though, it did different stuff for hardlinks. Also not ideal.

The issue here is that if helix crashes / gets killed while executing
step 2 or 3, the file would be in an inconsistent state. This is not ideal
at all, file writing isn't atomic with this strategy.

This patch simplifies this logic into:

- Create a temporary file, write the stuff we want to write into it.
- If that fails, we don't need to do anything as the original file is
  untouched. Though up for question: should we try to delete the "half-
  written" file on write failures (or is that even possible?).
- If it is successful, we rename the temporary file over the actual
  write destionation. This is atomic, and if it fails the destination is
  unmodified.

There are also other benefits from doing the latter, such as inotify.
Previously, we would get these events for an edit of `.cargo/config.toml`:

    Create /Users/pala/Projects/helix/.cargo/config.toml.bck
    Write /Users/pala/Projects/helix/.cargo/config.toml.bck
    Create /Users/pala/Projects/helix/.cargo/config.toml
    Write /Users/pala/Projects/helix/.cargo/config.toml
    Remove /Users/pala/Projects/helix/.cargo/config.toml.bck

Now, we get these:

    Create /Users/pala/Projects/helix/.cargo/config.toml.tmp
    Write /Users/pala/Projects/helix/.cargo/config.toml.tmp
    Write /Users/pala/Projects/helix/.cargo/config.toml

This is better and easier to handle, because inotify events
actually look like we are writing to the target file, instead of
re-creating this.
@RGBCube

RGBCube commented Jul 8, 2025

Copy link
Copy Markdown
Author

I removed the tempfile dependency and added fastrand (which it used internally). Any suggestions on eliminating that crate?

@the-mikedavis

Copy link
Copy Markdown
Member

Saving is not simple and has a lot of edge-cases. This doesn't work with hardlinks, for example: the hard link is destroyed. Instead of writing something novel we should be moving closer to what established editors do, like in #11374

@RGBCube

RGBCube commented Jul 8, 2025

Copy link
Copy Markdown
Author

That's interesting, the main thing that got me to do this was inofity, where Emacs did what was done in this PR. I'll take a look at what it does too, and comment on that PR if necessary

@RGBCube RGBCube closed this Jul 8, 2025
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