Add full support for the external editor command

This provides full support of arguments to the external editor.

7 files changed, 418 insertions, 41 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 69ec0ed..43013dc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Added
 - Support for 256-color terminals
 - Highlight of selected line(s) on 256-color terminals
+- Full support for external editor
 
 ### Fixed
 - Missing ncursesw dependency listing for deb build
diff --git a/README.md b/README.md
index f8ac128..cbe87a5 100644
--- a/README.md
+++ b/README.md
@@ -158,6 +158,18 @@ color triplet with the format `<red>,<green>,<blue>` can be used. Each color has
 `255, 255, 255` resulting in white and `0,0,0` resulting in black. A value of `-1` or `transparent` can be used to use
 the default terminal color.
 
+
+#### Configuring External Editor
+
+The external editor action will first attempt to start the editor defined by the
+[Git configuration "core.editor"][git-core-editor], followed by the `VISUAL` and
+`EDITOR` environment variables. Finally, if neither is set the external editor
+defaults to using `vi`.
+
+The `%` character in the value will be replaced with the git rebase todo file.
+If the `%` character is not found, then the git rebase todo file will be
+provided as the last argument.
+
 ## Development
 
 ### Install Rust
@@ -221,6 +233,7 @@ See [Third Party Licenses](THIRD_PARTY_LICENSES) for licenses for third-party li
 [cargo]:https://github.com/rust-lang/cargo
 [crates-io]:https://crates.io/crates/git-interactive-rebase-tool
 [git-config]:https://git-scm.com/docs/git-config
+[git-core-editor]:https://www.git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_code_core_editor_code
 [git-sequence-editor]:https://git-scm.com/docs/git-config#git-config-sequenceeditor
 [install-rust]:https://doc.rust-lang.org/book/getting-started.html
 [license]:https://raw.githubusercontent.com/MitMaro/git-interactive-rebase-tool/master/LICENSE
diff --git a/src/config/config.rs b/src/config/config.rs
index 36301e3..62cce63 100644
--- a/src/config/config.rs
+++ b/src/config/config.rs
@@ -1,23 +1,14 @@
 use crate::color::Color;
-use crate::config::utils::{
-	editor_from_env,
-	get_bool,
-	get_color,
-	get_input,
-	get_os_string,
-	get_string,
-	open_git_config,
-};
+use crate::config::utils::{editor_from_env, get_bool, get_color, get_input, get_string, open_git_config};
 use crate::config::Theme;
 use std::convert::TryFrom;
-use std::ffi::OsString;
 
 #[derive(Clone, Debug)]
 pub struct Config {
 	pub theme: Theme,
 	pub auto_select_next: bool,
 	pub comment_char: String,
-	pub editor: OsString,
+	pub editor: String,
 	pub input_abort: String,
 	pub input_action_break: String,
 	pub input_action_drop: String,
@@ -78,7 +69,7 @@ impl Config {
 			},
 			auto_select_next: get_bool(&git_config, "interactive-rebase-tool.autoSelectNext", false)?,
 			comment_char: get_string(&git_config, "core.commentChar", "#")?,
-			editor: get_os_string(&git_config, "core.editor", editor_from_env())?,
+			editor: get_string(&git_config, "core.editor", editor_from_env().as_str())?,
 			input_abort: get_input(&git_config, "interactive-rebase-tool.inputAbort", "q")?,
 			input_action_break: get_input(&git_config, "interactive-rebase-tool.inputActionBreak", "b")?,
 			input_action_drop: get_input(&git_config, "interactive-rebase-tool.inputActionDrop", "d")?,
diff --git a/src/config/utils.rs b/src/config/utils.rs
index cde72bb..8befdc4 100644
--- a/src/config/utils.rs
+++ b/src/config/utils.rs
@@ -1,6 +1,6 @@
 use crate::color::Color;
 use std::convert::TryFrom;
-use std::{env, ffi::OsString};
+use std::env;
 
 pub(in crate::config) fn get_input(config: &git2::Config, name: &str, default: &str) -> Result<String, String> {
 	let value = get_string(config, name, default)?;
@@ -32,19 +32,6 @@ pub(in crate::config) fn get_string(config: &git2::Config, name: &str, default:
 	}
 }
 
-pub(in crate::config) fn get_os_string(
-	config: &git2::Config,
-	name: &str,
-	default: OsString,
-) -> Result<OsString, String>
-{
-	match config.get_string(name) {
-		Ok(v) => Ok(OsString::from(v)),
-		Err(ref e) if e.code() == git2::ErrorCode::NotFound => Ok(default),
-		Err(e) => Err(format!("Error reading git config: {}", e)),
-	}
-}
-
 pub(in crate::config) fn get_bool(config: &git2::Config, name: &str, default: bool) -> Result<bool, String> {
 	match config.get_bool(name) {
 		Ok(v) => Ok(v),
@@ -61,10 +48,10 @@ pub(in crate::config) fn get_color(config: &git2::Config, name: &str, default_co
 	}
 }
 
-pub(in crate::config) fn editor_from_env() -> OsString {
-	env::var_os("VISUAL")
-		.or_else(|| env::var_os("EDITOR"))
-		.unwrap_or_else(|| OsString::from("vi"))
+pub(in crate::config) fn editor_from_env() -> String {
+	env::var("VISUAL")
+		.or_else(|_| env::var("EDITOR"))
+		.unwrap_or_else(|_| String::from("vi"))
 }
 
 pub(in crate::config) fn open_git_config() -> Result<git2::Config, String> {
diff --git a/src/external_editor/argument_tolkenizer.rs b/src/external_editor/argument_tolkenizer.rs
new file mode 100644
index 0000000..f1b007a
--- /dev/null
+++ b/src/external_editor/argument_tolkenizer.rs
@@ -0,0 +1,365 @@
+#[derive(Clone, Copy, Debug, PartialEq)]
+enum State {
+	Normal,
+	Escape,
+	DoubleQuote,
+	SingleQuote,
+	WhiteSpace,
+}
+
+pub fn tolkenize(input: &str) -> Option<Vec<String>> {
+	let mut previous_state = State::Normal;
+	let mut state = State::Normal;
+	let mut token_start: usize = 0;
+	let mut value = String::from("");
+	let mut force_value = false;
+
+	let mut tokens = vec![];
+	for (i, c) in input.chars().enumerate() {
+		// 		eprintln!("'{}' '{}'", i, c);
+		match state {
+			State::Normal => {
+				if c == '\\' {
+					previous_state = State::Normal;
+					state = State::Escape;
+				}
+				else if c == '"' {
+					value.push_str(&input[token_start..i]);
+					token_start = i + 1;
+					state = State::DoubleQuote;
+				}
+				else if c == '\'' {
+					value.push_str(&input[token_start..i]);
+					token_start = i + 1;
+					state = State::SingleQuote;
+				}
+				else if c.is_ascii_whitespace() {
+					state = State::WhiteSpace;
+					if token_start != i || !value.is_empty() {
+						tokens.push(format!("{}{}", value, &input[token_start..i]));
+						value.clear();
+					}
+				}
+			},
+			State::DoubleQuote => {
+				if c == '\\' {
+					previous_state = State::DoubleQuote;
+					state = State::Escape;
+				}
+				else if c == '"' {
+					let v = &input[token_start..i];
+					if v.is_empty() {
+						force_value = true;
+					}
+					value.push_str(&input[token_start..i]);
+					token_start = i + 1;
+					state = State::Normal;
+				}
+			},
+			State::SingleQuote => {
+				if c == '\'' {
+					let v = &input[token_start..i];
+					if v.is_empty() {
+						force_value = true;
+					}
+					value.push_str(&input[token_start..i]);
+					token_start = i + 1;
+					state = State::Normal;
+				}
+			},
+			State::WhiteSpace => {
+				force_value = false;
+				token_start = i;
+				if c == '\\' {
+					// this next character should be parsed in normal state
+					previous_state = State::Normal;
+					state = State::Escape;
+				}
+				else if c == '"' {
+					value.push_str(&input[token_start..i]);
+					token_start = i + 1;
+					state = State::DoubleQuote;
+				}
+				else if c == '\'' {
+					value.push_str(&input[token_start..i]);
+					token_start = i + 1;
+					state = State::SingleQuote;
+				}
+				else if !c.is_ascii_whitespace() {
+					state = State::Normal;
+				}
+			},
+			State::Escape => {
+				value.push_str(&input[token_start..(i - 1)]);
+				value.push_str(&input[i..=i]);
+				state = previous_state;
+				token_start = i + 1;
+			},
+		}
+	}
+
+	if state != State::Normal && state != State::WhiteSpace {
+		return None;
+	}
+
+	if state == State::Normal && token_start < input.len() {
+		value.push_str(&input[token_start..]);
+	}
+
+	if force_value || !value.is_empty() {
+		tokens.push(value);
+	}
+
+	Some(tokens)
+}
+
+#[cfg(test)]
+mod tests {
+	use crate::external_editor::argument_tolkenizer::tolkenize;
+
+	#[test]
+	fn tolkenize_empty_string() {
+		assert_eq!(tolkenize("").unwrap().len(), 0);
+	}
+
+	#[test]
+	fn tolkenize_single_spaces() {
+		assert_eq!(tolkenize(" ").unwrap().len(), 0);
+	}
+
+	#[test]
+	fn tolkenize_single_tab() {
+		assert_eq!(tolkenize("\t").unwrap().len(), 0);
+	}
+
+	#[test]
+	fn tolkenize_multiple_spaces() {
+		assert_eq!(tolkenize("    ").unwrap().len(), 0);
+	}
+
+	#[test]
+	fn tolkenize_multiple_tabs() {
+		assert_eq!(tolkenize("\t\t\t").unwrap().len(), 0);
+	}
+
+	#[test]
+	fn tolkenize_empty_double_quoted_string() {
+		assert_eq!(tolkenize("\"\"").unwrap(), vec![""]);
+	}
+
+	#[test]
+	fn tolkenize_empty_single_quoted_string() {
+		assert_eq!(tolkenize("''").unwrap(), vec![""]);
+	}
+
+	#[test]
+	fn tolkenize_single_character() {
+		assert_eq!(tolkenize("a").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_single_character_in_double_quoted_string() {
+		assert_eq!(tolkenize("\"a\"").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_single_character_in_single_quoted_string() {
+		assert_eq!(tolkenize("'a'").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_single_leading_spaces() {
+		assert_eq!(tolkenize(" a").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_multiple_leading_spaces() {
+		assert_eq!(tolkenize("     a").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_single_leading_tab() {
+		assert_eq!(tolkenize("\ta").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_multiple_leading_tabs() {
+		assert_eq!(tolkenize("\t\t\ta").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_single_trailing_spaces() {
+		assert_eq!(tolkenize("a ").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_multiple_trailing_spaces() {
+		assert_eq!(tolkenize("a     ").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_single_trailing_tab() {
+		assert_eq!(tolkenize("a\t").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_multiple_trailing_tabs() {
+		assert_eq!(tolkenize("a\t\t\t").unwrap(), vec!["a"]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_space() {
+		assert_eq!(tolkenize("\\ ").unwrap(), vec![" "]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_double_quote() {
+		assert_eq!(tolkenize("\\\"").unwrap(), vec!["\""]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_single_quote() {
+		assert_eq!(tolkenize("\\'").unwrap(), vec!["'"]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_slash() {
+		assert_eq!(tolkenize("\\\\").unwrap(), vec!["\\"]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_space_before_parameter() {
+		assert_eq!(tolkenize("\\ foo").unwrap(), vec![" foo"]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_space_with_space_before() {
+		assert_eq!(tolkenize(" \\ ").unwrap(), vec![" "]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_space_after_parameter() {
+		assert_eq!(tolkenize("foo\\ ").unwrap(), vec!["foo "]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_space_before_double_quotes() {
+		assert_eq!(tolkenize("\\ \"foo\"").unwrap(), vec![" foo"]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_space_before_single_quotes() {
+		assert_eq!(tolkenize("\\ 'foo'").unwrap(), vec![" foo"]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_space_after_double_quotes() {
+		assert_eq!(tolkenize("\"foo\"\\ ").unwrap(), vec!["foo "]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_space_after_single_quotes() {
+		assert_eq!(tolkenize("'foo'\\ ").unwrap(), vec!["foo "]);
+	}
+
+	#[test]
+	fn tolkenize_escaped_spaces_1() {
+		assert_eq!(tolkenize(" \\ aaa\\ bbb\\  ").unwrap(), vec![" aaa bbb "]);
+	}
+
+	#[test]
+	fn tolkenize_mixed_whitespace_1() {
+		assert_eq!(tolkenize("\t\taaa \t bbb\t \tccc \tddd\t eee  ").unwrap(), vec![
+			"aaa", "bbb", "ccc", "ddd", "eee"
+		]);
+	}
+
+	#[test]
+	fn tolkenize_mixed_whitespace_2() {
+		assert_eq!(
+			tolkenize("\t\t\"aaa \t bbb\t \tccc\" \td\\\"dd\t eee  ").unwrap(),
+			vec!["aaa \t bbb\t \tccc", "d\"dd", "eee"]
+		);
+	}
+
+	#[test]
+	fn tolkenize_mixed_whitespace_3() {
+		assert_eq!(tolkenize("\t\"a\" e").unwrap(), vec!["a", "e"]);
+	}
+
+	#[test]
+	fn tolkenize_basic_string() {
+		assert_eq!(tolkenize("a simple arguments").unwrap(), vec![
+			"a",
+			"simple",
+			"arguments"
+		]);
+	}
+
+	#[test]
+	fn tolkenize_joined_double_quote() {
+		assert_eq!(tolkenize("foo\"bar\"").unwrap(), vec!["foobar"]);
+	}
+
+	#[test]
+	fn tolkenize_argument_with_space_in_quotes() {
+		assert_eq!(tolkenize("\"bar with space\"").unwrap(), vec!["bar with space"]);
+	}
+
+	#[test]
+	fn tolkenize_argument_with_escaped_double_quote() {
+		assert_eq!(tolkenize("\"bar \\\"with\\\" space\"").unwrap(), vec![
+			"bar \"with\" space"
+		]);
+	}
+
+	#[test]
+	fn tolkenize_argument_with_embedded_single_quote() {
+		assert_eq!(tolkenize("\"bar 'with' space\"").unwrap(), vec!["bar 'with' space"]);
+	}
+
+	#[test]
+	fn tolkenize_joined_double_quoted_arguments() {
+		assert_eq!(tolkenize("\"foo\"\"bar\"").unwrap(), vec!["foobar"]);
+	}
+
+	#[test]
+	fn tolkenize_joined_single_quoted_arguments() {
+		assert_eq!(tolkenize("'foo''bar'").unwrap(), vec!["foobar"]);
+	}
+
+	#[test]
+	fn tolkenize_mixed_joined_1() {
+		assert_eq!(tolkenize("'foo'bar").unwrap(), vec!["foobar"]);
+	}
+
+	#[test]
+	fn tolkenize_mixed_joined_2() {
+		assert_eq!(tolkenize("foo'bar'").unwrap(), vec!["foobar"]);
+	}
+
+	#[test]
+	fn tolkenize_just_escaped() {
+		assert!(tolkenize("\\").is_none());
+	}
+
+	#[test]
+	fn tolkenize_just_double_quote() {
+		assert!(tolkenize("\"").is_none());
+	}
+
+	#[test]
+	fn tolkenize_just_single_quote() {
+		assert!(tolkenize("'").is_none());
+	}
+
+	#[test]
+	fn tolkenize_double_quote_unmatched() {
+		assert!(tolkenize("\"   ").is_none());
+	}
+
+	#[test]
+	fn tolkenize_single_quote_unmatched() {
+		assert!(tolkenize("'   ").is_none());
+	}
+}
diff --git a/src/external_editor/external_editor.rs b/src/external_editor/external_editor.rs
index f1ca70c..b1cf48c 100644
--- a/src/external_editor/external_editor.rs
+++ b/src/external_editor/external_editor.rs
@@ -1,5 +1,6 @@
 use crate::config::Config;
 use crate::display::Display;
+use crate::external_editor::argument_tolkenizer::tolkenize;
 use crate::git_interactive::GitInteractive;
 use crate::input::{Input, InputHandler};
 use crate::process::{
@@ -12,6 +13,7 @@ use crate::process::{
 	State,
 };
 use crate::view::View;
+use std::ffi::OsString;
 use std::process::Command;
 use std::process::ExitStatus as ProcessExitStatus;
 
@@ -66,20 +68,37 @@ impl<'e> ExternalEditor<'e> {
 	}
 
 	pub fn run_editor(&mut self, git_interactive: &GitInteractive) -> Result<(), String> {
+		let mut arguments = match tolkenize(self.config.editor.as_str()) {
+			Some(args) => {
+				if args.is_empty() {
+					return Err(String::from("No editor configured"));
+				}
+				args.into_iter().map(OsString::from)
+			},
+			None => {
+				return Err(format!("Invalid editor: {}", self.config.editor));
+			},
+		};
+
 		git_interactive.write_file()?;
 		let filepath = git_interactive.get_filepath();
 		let callback = || -> Result<ProcessExitStatus, String> {
-			// TODO: This doesn't handle editor with arguments (e.g. EDITOR="edit --arg")
-			Command::new(&self.config.editor)
-				.arg(filepath.as_os_str())
-				.status()
-				.map_err(|e| {
-					format!(
-						"Unable to run editor ({}):\n{}",
-						self.config.editor.to_string_lossy(),
-						e.to_string()
-					)
-				})
+			let mut file_pattern_found = false;
+			let mut cmd = Command::new(arguments.next().unwrap());
+			for arg in arguments {
+				if arg.as_os_str() == "%" {
+					file_pattern_found = true;
+					cmd.arg(filepath.as_os_str());
+				}
+				else {
+					cmd.arg(arg);
+				}
+			}
+			if !file_pattern_found {
+				cmd.arg(filepath.as_os_str());
+			}
+			cmd.status()
+				.map_err(|e| format!("Unable to run editor ({}):\n{}", self.config.editor, e.to_string()))
 		};
 		let exit_status: ProcessExitStatus = self.display.leave_temporarily(callback)?;
 
diff --git a/src/external_editor/mod.rs b/src/external_editor/mod.rs
index efa7480..495d507 100644
--- a/src/external_editor/mod.rs
+++ b/src/external_editor/mod.rs
@@ -1,3 +1,4 @@
+mod argument_tolkenizer;
 #[allow(clippy::module_inception)]
 mod external_editor;