83b6042 · helix · git.iko.soy

git.iko.soy/helix

fix(write): do not set new path on document until write succeeds

If a document is written with a new path, currently, in the event that the write fails, the document still gets its path changed. This fixes it so that the path is not updated unless the write succeeds.

commit83b6042b97d13fca751e3d5d0c32f04e3ad04c9a authorSkyler Hawthorne <skyler@dead10ck.com> dateSun 24 Apr 2022, 15:33 (5 years ago) parenta5a9318

4 files changed+116−32

compare with master

unifiedsplit

helix-term/src/application.rs+18−8 helix-term/src/commands/typed.rs+2−7 helix-term/tests/test/write.rs+59−2 helix-view/src/document.rs+37−15

helix-term/src/application.rs+18−8

@@ -3,6 +3,7 @@ use futures_util::Stream;

33use helix_core::{

44 config::{default_syntax_loader, user_syntax_loader},

55 diagnostic::{DiagnosticTag, NumberOrString},

6+ path::get_relative_path,

67 pos_at_coords, syntax, Selection,

78};

89use helix_lsp::{lsp, util::lsp_pos_to_pos, LspProgressMap};

@@ -489,17 +490,26 @@ impl Application {

489490 );

490491

491492 doc.set_last_saved_revision(doc_save_event.revision);

493+

492494 let lines = doc.text().len_lines();

493495 let bytes = doc.text().len_bytes();

494496

495− let path_str = doc

496− .path()

497− .expect("document written without path")

498− .to_string_lossy()

499− .into_owned();

500−

501− self.editor

502− .set_status(format!("'{}' written, {}L {}B", path_str, lines, bytes));

497+ if let Err(err) = doc.set_path(Some(&doc_save_event.path)) {

498+ log::error!(

499+ "error setting path for doc '{:?}': {}",

500+ doc.path(),

501+ err.to_string(),

502+ );

503+ self.editor.set_error(err.to_string());

504+ } else {

505+ // TODO: fix being overwritten by lsp

506+ self.editor.set_status(format!(

507+ "'{}' written, {}L {}B",

508+ get_relative_path(&doc_save_event.path).to_string_lossy(),

509+ lines,

510+ bytes

511+ ));

512+ }

503513 }

504514

505515 pub fn handle_terminal_events(&mut self, event: Result<CrosstermEvent, crossterm::ErrorKind>) {

helix-term/src/commands/typed.rs+2−7

@@ -268,11 +268,6 @@ fn write_impl(

268268 let jobs = &mut cx.jobs;

269269 let doc = doc_mut!(cx.editor);

270270

271− if let Some(ref path) = path {

272− doc.set_path(Some(path.as_ref().as_ref()))

273− .context("invalid filepath")?;

274− }

275−

276271 if doc.path().is_none() {

277272 bail!("cannot write a buffer without a filename");

278273 }

@@ -292,7 +287,7 @@ fn write_impl(

292287 None

293288 };

294289

295− doc.format_and_save(fmt, force)?;

290+ doc.format_and_save(fmt, path.map(AsRef::as_ref), force)?;

296291

297292 if path.is_some() {

298293 let id = doc.id();

@@ -607,7 +602,7 @@ fn write_all_impl(

607602 None

608603 };

609604

610− doc.format_and_save(fmt, force)?;

605+ doc.format_and_save::<_, PathBuf>(fmt, None, force)?;

611606 }

612607

613608 if quit {

helix-term/tests/test/write.rs+59−2

@@ -35,7 +35,7 @@ async fn test_write() -> anyhow::Result<()> {

3535 Ok(())

3636}

3737

38−#[tokio::test]

38+#[tokio::test(flavor = "multi_thread")]

3939async fn test_write_quit() -> anyhow::Result<()> {

4040 let mut file = tempfile::NamedTempFile::new()?;

4141

@@ -129,7 +129,64 @@ async fn test_write_fail_mod_flag() -> anyhow::Result<()> {

129129}

130130

131131#[tokio::test]

132−#[ignore]

132+async fn test_write_new_path() -> anyhow::Result<()> {

133+ let mut file1 = tempfile::NamedTempFile::new().unwrap();

134+ let mut file2 = tempfile::NamedTempFile::new().unwrap();

135+

136+ test_key_sequences(

137+ &mut Application::new(

138+ Args {

139+ files: vec![(file1.path().to_path_buf(), Position::default())],

140+ ..Default::default()

141+ },

142+ Config::default(),

143+ )?,

144+ vec![

145+ (

146+ Some("ii can eat glass, it will not hurt me<ret><esc>:w<ret>"),

147+ Some(&|app| {

148+ let doc = doc!(app.editor);

149+ assert!(!app.editor.is_err());

150+ assert_eq!(file1.path(), doc.path().unwrap());

151+ }),

152+ ),

153+ (

154+ Some(&format!(":w {}<ret>", file2.path().to_string_lossy())),

155+ Some(&|app| {

156+ let doc = doc!(app.editor);

157+ assert!(!app.editor.is_err());

158+ assert_eq!(file2.path(), doc.path().unwrap());

159+ assert!(app.editor.document_by_path(file1.path()).is_none());

160+ }),

161+ ),

162+ ],

163+ false,

164+ )

165+ .await?;

166+

167+ file1.as_file_mut().flush()?;

168+ file1.as_file_mut().sync_all()?;

169+ file2.as_file_mut().flush()?;

170+ file2.as_file_mut().sync_all()?;

171+

172+ let mut file1_content = String::new();

173+ file1.as_file_mut().read_to_string(&mut file1_content)?;

174+ assert_eq!(

175+ helpers::platform_line("i can eat glass, it will not hurt me\n"),

176+ file1_content

177+ );

178+

179+ let mut file2_content = String::new();

180+ file2.as_file_mut().read_to_string(&mut file2_content)?;

181+ assert_eq!(

182+ helpers::platform_line("i can eat glass, it will not hurt me\n"),

183+ file2_content

184+ );

185+

186+ Ok(())

187+}

188+

189+#[tokio::test]

133190async fn test_write_fail_new_path() -> anyhow::Result<()> {

134191 let file = helpers::new_readonly_tempfile()?;

135192

helix-view/src/document.rs+37−15

@@ -91,6 +91,7 @@ impl Serialize for Mode {

9191pub struct DocumentSaveEvent {

9292 pub revision: usize,

9393 pub doc_id: DocumentId,

94+ pub path: PathBuf,

9495}

9596

9697pub type DocumentSaveEventResult = Result<DocumentSaveEvent, anyhow::Error>;

@@ -512,41 +513,61 @@ impl Document {

512513 Some(fut.boxed())

513514 }

514515

515− pub fn save(&mut self, force: bool) -> Result<(), anyhow::Error> {

516− self.save_impl::<futures_util::future::Ready<_>>(None, force)

516+ pub fn save<P: Into<PathBuf>>(

517+ &mut self,

518+ path: Option<P>,

519+ force: bool,

520+ ) -> Result<(), anyhow::Error> {

521+ self.save_impl::<futures_util::future::Ready<_>, _>(None, path, force)

517522 }

518523

519− pub fn format_and_save(

524+ pub fn format_and_save<F, P>(

520525 &mut self,

521− formatting: Option<

522− impl Future<Output = Result<Transaction, FormatterError>> + 'static + Send,

523− >,

526+ formatting: Option<F>,

527+ path: Option<P>,

524528 force: bool,

525− ) -> anyhow::Result<()> {

526− self.save_impl(formatting, force)

529+ ) -> anyhow::Result<()>

530+ where

531+ F: Future<Output = Result<Transaction, FormatterError>> + 'static + Send,

532+ P: Into<PathBuf>,

533+ {

534+ self.save_impl(formatting, path, force)

527535 }

528536

529− // TODO: impl Drop to handle ensuring writes when closed

530537 /// The `Document`'s text is encoded according to its encoding and written to the file located

531538 /// at its `path()`.

532539 ///

533540 /// If `formatting` is present, it supplies some changes that we apply to the text before saving.

534− fn save_impl<F: Future<Output = Result<Transaction, FormatterError>> + 'static + Send>(

541+ fn save_impl<F, P>(

535542 &mut self,

536543 formatting: Option<F>,

544+ path: Option<P>,

537545 force: bool,

538− ) -> Result<(), anyhow::Error> {

546+ ) -> Result<(), anyhow::Error>

547+ where

548+ F: Future<Output = Result<Transaction, FormatterError>> + 'static + Send,

549+ P: Into<PathBuf>,

550+ {

539551 if self.save_sender.is_none() {

540552 bail!("saves are closed for this document!");

541553 }

542554

543555 // we clone and move text + path into the future so that we asynchronously save the current

544556 // state without blocking any further edits.

545−

546557 let mut text = self.text().clone();

547− let path = self.path.clone().expect("Can't save with no path set!");

548− let identifier = self.identifier();

549558

559+ let path = match path {

560+ Some(path) => helix_core::path::get_canonicalized_path(&path.into())?,

561+ None => {

562+ if self.path.is_none() {

563+ bail!("Can't save with no path set!");

564+ }

565+

566+ self.path.as_ref().unwrap().clone()

567+ }

568+ };

569+

570+ let identifier = self.identifier();

550571 let language_server = self.language_server.clone();

551572

552573 // mark changes up to now as saved

@@ -586,12 +607,13 @@ impl Document {

586607 }

587608 }

588609

589− let mut file = File::create(path).await?;

610+ let mut file = File::create(&path).await?;

590611 to_writer(&mut file, encoding, &text).await?;

591612

592613 let event = DocumentSaveEvent {

593614 revision: current_rev,

594615 doc_id,

616+ path,

595617 };

596618

597619 if let Some(language_server) = language_server {