Using :write after any command that produces changes introduces a phantom change

[paveloom]

Summary

Something in the code introduces a change of state after executing a sequence of commands that contains :pipe and ends with :write.

Reproduction Steps

Consider the following config:

# ~/.config/helix/config.toml
# <...>
[keys.normal]
C-l = [":pipe echo '# test'", ":write"]
# <...>

After restarting Helix, do the following:

1. Open any writable buffer
2. Press Ctrl + l
3. Quit the editor, aborting the changes: :q!
4. Restart, open the same buffer, discover that the text was actually written.

I expected this to happen:
The key should print # test and write the buffer.

Instead, this happened:
The key does the above, but it also introduces a phantom change. That is, the state of the buffer appears to be modified, according to the indicator in the status bar.

Helix log

Reproducing this issue doesn't emit relevant log messages.

Platform

Linux

Terminal Emulator

GNOME Terminal 3.42.2 using VTE 0.66.2 +BIDI +GNUTLS +ICU +SYSTEMD

Helix Version

helix 22.05-dev (35d2693)

[jappeace]

oh :pipe sleep 0.1 also produces a change according to the buffer.

so here you got two bugs, 1 pipe producing a change even though one didn't neccisarly happen, 2 write not clearing the change if a change happened

[paveloom]

Correction: the issue happens with any command that produces changes. Try

# ~/.config/helix/config.toml
# <...>
[keys.normal]
C-h = ["indent", ":write"]
# <...>

@jappeace I would argue that :pipe sleep 0.1 producing a change is expected, since the change actually happens. According to the code, :pipe will replace all current selections with the output of a shell command, which in this case is empty.

[paveloom]

So... here's a chain of relevant calls on write:

helix/helix-term/src/commands/typed.rs

Lines 235 to 241 in d2b1add

	fn write(
	cx: &mut compositor::Context,
	args: &[Cow<str>],
	_event: PromptEvent,
	) -> anyhow::Result<()> {
	write_impl(cx, args.first(), false)
	}

In write_impl:

helix/helix-term/src/commands/typed.rs

Lines 224 to 225 in d2b1add

	let future = doc.format_and_save(fmt, force);
	cx.jobs.add(Job::new(future).wait_before_exiting());

helix/helix-view/src/document.rs

Lines 441 to 447 in d2b1add

	pub fn format_and_save(
	&mut self,
	formatting: Option<impl Future<Output = LspFormatting>>,
	force: bool,
	) -> impl Future<Output = anyhow::Result<()>> {
	self.save_impl(formatting, force)
	}

In save_impl:

helix/helix-view/src/document.rs

Lines 469 to 470 in d2b1add

	// mark changes up to now as saved
	self.reset_modified();

helix/helix-view/src/document.rs

Lines 832 to 838 in d2b1add

	/// Save modifications to history, and so [`Self::is_modified`] will return false.
	pub fn reset_modified(&mut self) {
	let history = self.history.take();
	let current_revision = history.current_revision();
	self.history.set(history);
	self.last_saved_revision = current_revision;
	}

So, reset_modified should save the current revision as the last one, therefore marking the state as not modified. However, it's still modified after running a sequence of commands because self.changes isn't empty, so the is_modified method will return true:

helix/helix-view/src/document.rs

Lines 824 to 830 in d2b1add

	/// If there are unsaved modifications.
	pub fn is_modified(&self) -> bool {
	let history = self.history.take();
	let current_revision = history.current_revision();
	self.history.set(history);
	current_revision != self.last_saved_revision || !self.changes.is_empty()
	}

Taking the example from above, the difference between the mapped sequence of commands ["indent", ":write"] and two manual calls to > and :w is that in the latter case, the changes are appended to the history before they're written:

In handle_event:

helix/helix-term/src/ui/editor.rs

Lines 1255 to 1259 in d2b1add

	// Store a history state if not in insert mode. This also takes care of
	// committing changes when leaving insert mode.
	if doc.mode() != Mode::Insert {
	doc.append_changes_to_history(view.id);
	}

helix/helix-view/src/document.rs

Lines 799 to 818 in d2b1add

	/// Commit pending changes to history
	pub fn append_changes_to_history(&mut self, view_id: ViewId) {
	if self.changes.is_empty() {
	return;
	}
	let new_changeset = ChangeSet::new(self.text());
	let changes = std::mem::replace(&mut self.changes, new_changeset);
	// Instead of doing this messy merge we could always commit, and based on transaction
	// annotations either add a new layer or compose into the previous one.
	let transaction =
	Transaction::from(changes).with_selection(self.selection(view_id).clone());
	// HAXX: we need to reconstruct the state as it was before the changes..
	let old_state = self.old_state.take().expect("no old_state available");
	let mut history = self.history.take();
	history.commit_revision(&transaction, &old_state);
	self.history.set(history);
	}

A solution to this issue would be to append the changes of each command in a command sequence to history. Or maybe only before commands that should mark the document as unmodified. Speaking of which, there is a new handy-dandy command to append the changes manually:

helix/helix-term/src/commands.rs

Lines 3005 to 3008 in d2b1add

	fn commit_undo_checkpoint(cx: &mut Context) {
	let (view, doc) = current!(cx.editor);
	doc.append_changes_to_history(view.id);
	}

One can use it as a workaround to the original issue as follows:

# ~/.config/helix/config.toml
# <...>
[keys.normal]
C-l = [":pipe echo '# test'", "commit_undo_checkpoint", ":write"]
C-h = ["indent", "commit_undo_checkpoint", ":write"]
# <...>

[sourcefrog]

Another place that a similar or the same bug shows up is if you bind C-s to :write in insert mode: it does save the file, but Helix thinks it's still modified.

[JakeEP]

Came across this issue while searching to see if the problem @sourcefrog mentions already has an issue filed. Unsure if this "save in insert mode has remaining [+]" problem is covered by this issue. Either way, hope it gets a fix soon-ish, as while it's a minor visual bug it is quite distracting for me personally.

[dead10ck]

This was likely fixed by #2267. Feel free to reopen if not.

[TobTobXX]

Hmm... it does not appear fixed (at least on current HEAD: 789833c).

I have this config.toml

[keys.insert]
"esc" = ["normal_mode", ":write"]

It does work, but it doesn't update the statusline correctly. (Try inserting in a file and then hit escape. The change was written to disk, but the statusline still says it's dirty.)

Interestingly, when I press i, don't make a change and press esc immediately after, the flag disappears in the statusline.

I'm not 100% sure if this is the bug mentioned here. I'll open another issue if needed.