fix external editor for cmd and bat on windows

Cargo.toml

@@ -60,7 +60,9 @@ which = "4.4"
 pprof = { version = "0.11", features = ["flamegraph"], optional = true }
 
 [dev-dependencies]
+asyncgit = { path = "asyncgit", features = ["test_utils"] }
 pretty_assertions = "1.3"
+serial_test = "1.0.0"
 tempfile = "3.4"
 
 [badges]

asyncgit/Cargo.toml

@@ -14,6 +14,7 @@ keywords = ["git"]
 [dependencies]
 crossbeam-channel = "0.5"
 easy-cast = "0.5"
+env_logger = "0.10"
 git2 = "0.17"
 log = "0.4"
 # git2 = { path = "../../extern/git2-rs", features = ["vendored-openssl"]}
@@ -24,6 +25,7 @@ rayon-core = "1.11"
 scopetime = { path = "../scopetime", version = "0.1" }
 serde = { version = "1.0", features = ["derive"] }
 shellexpand = "3.1"
+tempfile = "3.4"
 thiserror = "1.0"
 unicode-truncate = "0.2.0"
 url = "2.3"
@@ -33,9 +35,9 @@ env_logger = "0.10"
 invalidstring = { path = "../invalidstring", version = "0.1" }
 pretty_assertions = "1.3"
 serial_test = "1.0"
-tempfile = "3.4"
 
 [features]
 default = ["trace-libgit"]
 trace-libgit = []
 vendor-openssl = ["openssl-sys"]
+test_utils = []

asyncgit/src/lib.rs

@@ -23,7 +23,7 @@
 // #![deny(clippy::expect_used)]
 //TODO: consider cleaning some up and allow specific places
 #![allow(clippy::significant_drop_tightening)]
-
+#![allow(clippy::multiple_crate_versions)]
 pub mod asyncjob;
 mod blame;
 mod branches;

asyncgit/src/sync/mod.rs

@@ -100,8 +100,19 @@ pub use utils::{
 
 pub use git2::ResetType;
 
-#[cfg(test)]
-mod tests {
+#[cfg(feature = "test_utils")]
+/// test utilities - exported now
+// see https://github.com/rust-lang/cargo/issues/8379
+pub mod tests {
+	// these are now not under 'test' so they get clippied with 'all-features'
+	// we dont care about tests that panic
+	#![allow(clippy::unwrap_used, clippy::missing_panics_doc)]
+	// minor niggles
+	#![allow(clippy::nursery)]
+	// this clippy is confused by the name 'read'
+	// should probably be changed to read_into
+	#![allow(clippy::read_zero_byte_vec)]
+
 	use super::{
 		commit,
 		repository::repo,
@@ -278,7 +289,7 @@ mod tests {
 
 	// init log
 	fn init_log() {
-		let _ = env_logger::builder()
+		let _b = env_logger::builder()
 			.is_test(true)
 			.filter_level(log::LevelFilter::Trace)
 			.try_init();

deny.toml

@@ -23,4 +23,5 @@ multiple-versions = "deny"
 skip-tree = [
     { name = "windows-sys" },
     { name = "hermit-abi" }
+
 ]

src/components/commit.rs

@@ -531,7 +531,6 @@ impl Component for CommitComponent {
 						self.input.set_text(msg);
 						self.commit_msg_history_idx += 1;
 					}
-				} else {
 				}
 				// stop key event propagation
 				return Ok(EventState::Consumed);

src/components/externaleditor.rs

@@ -64,9 +64,12 @@ impl ExternalEditorComponent {
 			bail!("file not found: {:?}", path);
 		}
 
-		io::stdout().execute(LeaveAlternateScreen)?;
-		defer! {
-			io::stdout().execute(EnterAlternateScreen).expect("reset terminal");
+		// so that the output is not messed up when running tests
+		if cfg!(not(test)) {
+			io::stdout().execute(LeaveAlternateScreen)?;
+			defer! {
+				io::stdout().execute(EnterAlternateScreen).expect("reset terminal");
+			}
 		}
 
 		let environment_options = ["GIT_EDITOR", "VISUAL", "EDITOR"];
@@ -80,6 +83,7 @@ impl ExternalEditorComponent {
 			.or_else(|| env::var(environment_options[2]).ok())
 			.unwrap_or_else(|| String::from("vi"));
 
+		log::trace!("external editor:{}", editor);
 		// TODO: proper handling arguments containing whitespaces
 		// This does not do the right thing if the input is `editor --something "with spaces"`
 
@@ -112,11 +116,53 @@ impl ExternalEditorComponent {
 
 		args.push(path.as_os_str());
 
-		Command::new(command.clone())
-			.current_dir(work_dir)
-			.args(args)
-			.status()
-			.map_err(|e| anyhow!("\"{}\": {}", command, e))?;
+		let exec_result = Command::new(&command)
+			.current_dir(&work_dir)
+			.args(&args)
+			.status();
+
+		if cfg!(windows) {
+			// if command failed to run on windows retry as a batch file (.bat, .cmd,...)
+			if exec_result.is_err() {
+				/*  here args contains the arguments pulled from the configured editor string
+					"myeditor --color blue" ->
+						args[0] = "--color"
+						args[1] = "blue"
+
+					now insert before these
+						"/C"
+						"myeditor"
+				*/
+
+				args.insert(0, OsStr::new("/C"));
+				args.insert(1, OsStr::new(&command));
+				let exec_result2 = Command::new("cmd")
+					.current_dir(work_dir)
+					.args(args)
+					.status();
+
+				match exec_result2 {
+					// failed to start (unlikely as cmd would have to be missing)
+					Err(e) => bail!("\"{}\": {}", command, e),
+
+					// ran, did it complete OK?
+					Ok(stat) => {
+						// no result is treated as arbitrary failure code of 99
+						let code = stat.code().unwrap_or(99);
+						if code != 0 {
+							bail!(
+								"\"{}\": cmd.exe returned {}",
+								command,
+								code
+							)
+						}
+					}
+				};
+			}
+		} else {
+			exec_result
+				.map_err(|e| anyhow!("\"{}\": {}", command, e))?;
+		}
 
 		Ok(())
 	}
@@ -192,3 +238,180 @@ impl Component for ExternalEditorComponent {
 		Ok(())
 	}
 }
+#[cfg(test)]
+mod tests {
+	use crate::components::ExternalEditorComponent;
+	use anyhow::Result;
+	use asyncgit::sync::tests::repo_init;
+	#[cfg(windows)]
+	use asyncgit::sync::utils::read_file;
+	use asyncgit::sync::RepoPath;
+	use serial_test::serial;
+	use std::env;
+	use std::fs::File;
+	use std::io::Write;
+	use tempfile::TempDir;
+
+	fn write_temp_file(
+		td: &TempDir,
+		file: &str,
+		content: &str,
+	) -> Result<()> {
+		let binding = td.path().join(file);
+		let file_path = binding.to_str().unwrap();
+		let mut file = File::create(file_path)?;
+		file.write_all(content.as_bytes())?;
+		Ok(())
+	}
+	const TEST_FILE_NAME: &str = "test1.txt";
+	const TEST_FILE_DATA: &str = "test file data";
+
+	fn setup_repo() -> (TempDir, RepoPath) {
+		let (td, repo) = repo_init().unwrap();
+		let root = repo.path().parent().unwrap();
+		let repo_path: RepoPath =
+			root.as_os_str().to_str().unwrap().into();
+
+		// create a dummy file to operate on
+		let txt = String::from(TEST_FILE_DATA);
+		write_temp_file(&td, TEST_FILE_NAME, &txt).unwrap();
+		(td, repo_path)
+	}
+
+	// these have to de serialzied because they set env variables to control which editor to use
+
+	#[test]
+	#[serial]
+	fn editor_missing() {
+		let (td, repo_path) = setup_repo();
+		let target_file_path = td.path().join(TEST_FILE_NAME);
+		env::set_var("GIT_EDITOR", "i_doubt_this_exists");
+		let foo = ExternalEditorComponent::open_file_in_editor(
+			&repo_path,
+			&target_file_path,
+		);
+		assert!(foo.is_err());
+	}
+
+	#[cfg(windows)]
+	mod win_test {
+		use super::*;
+		#[test]
+		#[serial]
+		fn editor_is_bat() {
+			let (td, repo_path) = setup_repo();
+			let target_file_path = td.path().join(TEST_FILE_NAME);
+			env::set_var("GIT_EDITOR", "testbat");
+			let bat = String::from("@echo off\ntype %1 >made.txt");
+			write_temp_file(&td, "testbat.bat", &bat).unwrap();
+
+			let runit = ExternalEditorComponent::open_file_in_editor(
+				&repo_path,
+				&target_file_path,
+			);
+			assert!(runit.is_ok());
+
+			let echo_file = td.path().join("made.txt");
+			let read_text = read_file(echo_file.as_path()).unwrap();
+
+			assert_eq!(
+				read_text.lines().next(),
+				Some(TEST_FILE_DATA)
+			);
+		}
+		#[test]
+		#[serial]
+		fn editor_is_bat_ext() {
+			let (td, repo_path) = setup_repo();
+			let target_file_path = td.path().join(TEST_FILE_NAME);
+
+			env::set_var("GIT_EDITOR", "testbat.bat");
+
+			let bat = String::from("@echo off\ntype %1 >made.txt");
+			write_temp_file(&td, "testbat.bat", &bat).unwrap();
+
+			let runit = ExternalEditorComponent::open_file_in_editor(
+				&repo_path,
+				&target_file_path,
+			);
+			assert!(runit.is_ok());
+
+			let echo_file = td.path().join("made.txt");
+			let read_text = read_file(echo_file.as_path()).unwrap();
+			assert_eq!(
+				read_text.lines().next(),
+				Some(TEST_FILE_DATA)
+			);
+		}
+		#[test]
+		#[serial]
+		fn editor_is_bat_noext_arg() {
+			let (td, repo_path) = setup_repo();
+			let target_file_path = td.path().join(TEST_FILE_NAME);
+
+			env::set_var("GIT_EDITOR", "testbat --foo");
+
+			let bat = String::from("@echo off\ntype %2 >made.txt");
+			write_temp_file(&td, "testbat.bat", &bat).unwrap();
+
+			let runit = ExternalEditorComponent::open_file_in_editor(
+				&repo_path,
+				&target_file_path,
+			);
+			assert!(runit.is_ok());
+
+			let echo_file = td.path().join("made.txt");
+			let read_text = read_file(echo_file.as_path()).unwrap();
+			assert_eq!(
+				read_text.lines().next(),
+				Some(TEST_FILE_DATA)
+			);
+		}
+		#[test]
+		#[serial]
+		fn editor_is_cmd() {
+			let (td, repo_path) = setup_repo();
+			let target_file_path = td.path().join(TEST_FILE_NAME);
+			env::set_var("GIT_EDITOR", "testcmd");
+			let bat = String::from("@echo off\ntype %1 >made.txt");
+			write_temp_file(&td, "testcmd.cmd", &bat).unwrap();
+
+			let runit = ExternalEditorComponent::open_file_in_editor(
+				&repo_path,
+				&target_file_path,
+			);
+			assert!(runit.is_ok());
+
+			let echo_file = td.path().join("made.txt");
+			let read_text = read_file(echo_file.as_path()).unwrap();
+
+			assert_eq!(
+				read_text.lines().next(),
+				Some(TEST_FILE_DATA)
+			);
+		}
+		#[test]
+		#[serial]
+		fn editor_is_cmd_arg() {
+			let (td, repo_path) = setup_repo();
+			let target_file_path = td.path().join(TEST_FILE_NAME);
+			env::set_var("GIT_EDITOR", "testcmd --bar");
+			let bat = String::from("@echo off\ntype %2 >made.txt");
+			write_temp_file(&td, "testcmd.cmd", &bat).unwrap();
+
+			let runit = ExternalEditorComponent::open_file_in_editor(
+				&repo_path,
+				&target_file_path,
+			);
+			assert!(runit.is_ok());
+
+			let echo_file = td.path().join("made.txt");
+			let read_text = read_file(echo_file.as_path()).unwrap();
+
+			assert_eq!(
+				read_text.lines().next(),
+				Some(TEST_FILE_DATA)
+			);
+		}
+	}
+}

src/components/help.rs

@@ -135,7 +135,6 @@ impl Component for HelpComponent {
 					self.move_selection(true);
 				} else if key_match(e, self.key_config.keys.move_up) {
 					self.move_selection(false);
-				} else {
 				}
 			}