Skip to content

Commit e3de940

Browse files
mathijs81mathijs81
committed
Get the editor from git through git var
We were invoking git anyway for the git config value of core.editor so we can just as well let git return the result of its full logic instead. This also handles the case where e.g. debian/ubuntu have compiled git with a different fallback than the "vi" default.
1 parent 59c5a4b commit e3de940

File tree

1 file changed

+64
-136
lines changed

1 file changed

+64
-136
lines changed

‎crates/but/src/tui/get_text.rs‎

Lines changed: 64 additions & 136 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
//! Various functions that involve launching the editor.
22
use anyhow::Result;
3+
use std::collections::HashMap;
34

45
/// Launches the user's preferred text editor to edit some initial text,
56
/// identified by a unique identifier (to avoid temp file collisions).
@@ -43,41 +44,33 @@ pub fn from_editor(identifier: &str, initial_text: &str) -> Result<String> {
4344
Ok(edited_text)
4445
}
4546

46-
/// Checks if the terminal is dumb (TERM environment variable is "dumb")
47-
fn is_terminal_dumb() -> bool {
48-
std::env::var("TERM")
49-
.map(|term| term == "dumb")
50-
.unwrap_or(false)
51-
}
52-
53-
/// Implement get_editor_command to match Git's C implementation of `git_editor`.
54-
///
55-
/// - Check GIT_EDITOR environment variable first
56-
/// - Check git config `core.editor` (editor_program)
57-
/// - Check VISUAL if terminal is not dumb
58-
/// - Check EDITOR
59-
/// - Return error if terminal is dumb and no editor found
60-
/// - Fall back to platform defaults (vi on Unix, notepad on Windows)
61-
/// - Add comprehensive unit tests for all scenarios
47+
/// Gets the editor command following git's logic:
48+
/// 1. Check GIT_EDITOR environment variable (handy if you want to skip invoking git)
49+
/// 2. Run `git var GIT_EDITOR`, git logic makes use of:
50+
/// - git config (core.editor)
51+
/// - env vars ($GIT_EDITOR, $VISUAL, $EDITOR)
52+
/// - built-in fallback (usually "vi", but "editor" on debian-based distros)
53+
/// 3. We fallback to notepad (Windows) or vi otherwise just in case we don't get something
54+
/// usable from `git var`.
6255
fn get_editor_command() -> Result<String> {
63-
get_editor_command_impl(&|key| std::env::var(key), is_terminal_dumb())
56+
let env: HashMap<String, String> = std::env::vars().collect();
57+
get_editor_command_impl(&env)
6458
}
6559

66-
/// Internal get_editor_command_implementation that can be tested without modifying environment
67-
fn get_editor_command_impl<F>(env_var: &F, terminal_is_dumb: bool) -> Result<String>
68-
where
69-
F: Fn(&str) -> Result<String, std::env::VarError>,
70-
{
71-
// Try $GIT_EDITOR first
72-
if let Ok(editor) = env_var("GIT_EDITOR")
60+
/// Internal implementation that can be tested with controlled environment
61+
fn get_editor_command_impl(env: &HashMap<String, String>) -> Result<String> {
62+
// Check GIT_EDITOR from the env map
63+
if let Some(editor) = env.get("GIT_EDITOR")
7364
&& !editor.is_empty()
7465
{
75-
return Ok(editor);
66+
return Ok(editor.clone());
7667
}
7768

78-
// Try git config `core.editor` (editor_program)
69+
// Run git var with the controlled environment
7970
if let Ok(output) = std::process::Command::new(gix::path::env::exe_invocation())
80-
.args(["config", "--get", "core.editor"])
71+
.args(["var", "GIT_EDITOR"])
72+
.env_clear()
73+
.envs(env)
8174
.output()
8275
&& output.status.success()
8376
{
@@ -87,139 +80,74 @@ where
8780
}
8881
}
8982

90-
// Try $VISUAL if terminal is not dumb
91-
if !terminal_is_dumb
92-
&& let Ok(editor) = env_var("VISUAL")
93-
&& !editor.is_empty()
94-
{
95-
return Ok(editor);
96-
}
97-
98-
if let Ok(editor) = env_var("EDITOR")
99-
&& !editor.is_empty()
100-
{
101-
return Ok(editor);
102-
}
103-
104-
// If terminal is dumb and no editor was found, return an error
105-
if terminal_is_dumb {
106-
return Err(anyhow::anyhow!(
107-
"Terminal is dumb, but no editor specified in GIT_EDITOR, core.editor, or EDITOR"
108-
));
109-
}
110-
111-
// Fallback to platform defaults (DEFAULT_EDITOR)
112-
let default_editor = if cfg!(windows) { "notepad" } else { "vi" };
113-
Ok(default_editor.to_string())
83+
// Simple fallback to platform defaults
84+
Ok(if cfg!(windows) { "notepad" } else { "vi" }.to_string())
11485
}
11586

11687
#[cfg(test)]
11788
mod tests {
11889
use super::*;
119-
use std::collections::HashMap;
120-
121-
// Helper to create a mock environment function from a hashmap
122-
fn mock_env(vars: HashMap<&str, &str>) -> impl Fn(&str) -> Result<String, std::env::VarError> {
123-
move |key: &str| {
124-
vars.get(key)
125-
.map(|v| v.to_string())
126-
.ok_or(std::env::VarError::NotPresent)
127-
}
128-
}
129-
130-
#[test]
131-
fn git_editor_takes_precedence() {
132-
let env = mock_env(HashMap::from([
133-
("GIT_EDITOR", "git-editor"),
134-
("VISUAL", "visual-editor"),
135-
("EDITOR", "editor"),
136-
]));
137-
let actual = get_editor_command_impl(&env, false).unwrap();
138-
assert_eq!(actual, "git-editor");
139-
}
90+
use std::sync::LazyLock;
14091

141-
#[test]
142-
fn visual_when_terminal_not_dumb() {
143-
let env = mock_env(visual_and_editor());
144-
let actual = get_editor_command_impl(&env, false).unwrap();
145-
assert_eq!(actual, "visual-editor");
146-
}
147-
148-
#[test]
149-
fn skips_visual_when_terminal_dumb() {
150-
let env = mock_env(visual_and_editor());
151-
let actual = get_editor_command_impl(&env, true).unwrap();
152-
assert_eq!(actual, "editor", "Should skip VISUAL and use EDITOR");
153-
}
92+
const OUR_PLATFORM_DEFAULT: &str = if cfg!(windows) { "notepad" } else { "vi" };
15493

155-
#[test]
156-
fn uses_editor() {
157-
let env = mock_env(HashMap::from([("EDITOR", "editor")]));
158-
let actual = get_editor_command_impl(&env, false).unwrap();
159-
assert_eq!(actual, "editor");
160-
}
161-
162-
#[test]
163-
fn fails_when_terminal_dumb_and_no_editor() {
164-
let env = mock_env(HashMap::new());
165-
let actual = get_editor_command_impl(&env, true);
166-
assert!(actual.unwrap_err().to_string().contains("Terminal is dumb"));
167-
}
94+
static BASIC_ENV: LazyLock<HashMap<String, String>> = LazyLock::new(|| {
95+
HashMap::from([
96+
("GIT_EDITOR".to_string(), "from-GIT_EDITOR".to_string()),
97+
("EDITOR".to_string(), "from-EDITOR".to_string()),
98+
("VISUAL".to_string(), "from-VISUAL".to_string()),
99+
// Just an example value, otherwise git thinks the terminal is "dumb" and follows other logic
100+
("TERM".to_string(), "xterm-256color".to_string()),
101+
])
102+
});
168103

169104
#[test]
170-
fn fails_when_terminal_dumb_with_only_visual() {
171-
let env = mock_env(HashMap::from([("VISUAL", "visual-editor")]));
172-
let actual = get_editor_command_impl(&env, true);
173-
assert!(
174-
actual.unwrap_err().to_string().contains("Terminal is dumb"),
175-
"VISUAL isn't used in dumb terminals"
105+
fn git_editor_takes_precedence() {
106+
let actual = get_editor_command_impl(&BASIC_ENV.clone()).unwrap();
107+
assert_eq!(
108+
actual, "from-GIT_EDITOR",
109+
"GIT_EDITOR should take precedence"
176110
);
177111
}
178112

179113
#[test]
180-
fn ignores_empty_git_editor() {
181-
let env = mock_env(HashMap::from([
182-
("GIT_EDITOR", ""),
183-
("VISUAL", "visual-editor"),
184-
("EDITOR", "editor"),
185-
]));
186-
let actual = get_editor_command_impl(&env, false).unwrap();
114+
fn falls_through_to_git_var_with_editor() {
115+
let mut env = BASIC_ENV.clone();
116+
env.remove("GIT_EDITOR"); // Ensure GIT_EDITOR is not set
117+
let actual = get_editor_command_impl(&env).unwrap();
187118
assert_eq!(
188-
actual, "visual-editor",
189-
"Empty GIT_EDITOR should be ignored, fall through to VISUAL"
119+
actual, "from-VISUAL",
120+
"git var should respect VISUAL env var"
190121
);
191122
}
192123

193124
#[test]
194-
fn ignores_empty_visual() {
195-
let env = mock_env(HashMap::from([("VISUAL", ""), ("EDITOR", "editor")]));
196-
let actual = get_editor_command_impl(&env, false).unwrap();
125+
fn falls_through_to_git_fallback_when_nothing_set() {
126+
let mut env = HashMap::new();
127+
env.insert("TERM".to_string(), "xterm-256color".to_string());
128+
let actual = get_editor_command_impl(&env).unwrap();
129+
// Our CI uses debian/ubuntu linux, which compiles git with "editor" as fallback
130+
// Other platforms should have the default fallback from editor.c, which is "vi"
131+
let expected = if cfg!(target_os = "linux") {
132+
"editor"
133+
} else {
134+
"vi"
135+
};
197136
assert_eq!(
198-
actual, "editor",
199-
"Empty VISUAL should be ignored, fall through to EDITOR"
137+
actual, expected,
138+
"git fallback should be used without any other env vars"
200139
);
201140
}
202141

203142
#[test]
204-
fn ignores_empty_editor() {
205-
let env = mock_env(HashMap::from([("EDITOR", "")]));
206-
let actual = get_editor_command_impl(&env, false).unwrap();
143+
fn falls_back_when_nothing_set() {
144+
// Empty environment, git considers this "dumb terminal" and `git var` will return empty string
145+
// so our own fallback will be used
146+
let mut env = HashMap::new();
147+
let actual = get_editor_command_impl(&env).unwrap();
207148
assert_eq!(
208-
actual, PLATFORM_DEFAULT,
209-
"Empty EDITOR should be ignored, fall back to default"
149+
actual, OUR_PLATFORM_DEFAULT,
150+
"Should fall back to vi/notepad when nothing is set"
210151
);
211152
}
212-
213-
#[test]
214-
fn falls_back_to_default_when_no_vars_set() {
215-
let env = mock_env(HashMap::new());
216-
let actual = get_editor_command_impl(&env, false).unwrap();
217-
assert_eq!(actual, PLATFORM_DEFAULT);
218-
}
219-
220-
const PLATFORM_DEFAULT: &str = if cfg!(windows) { "notepad" } else { "vi" };
221-
222-
fn visual_and_editor() -> HashMap<&'static str, &'static str> {
223-
HashMap::from([("VISUAL", "visual-editor"), ("EDITOR", "editor")])
224-
}
225153
}

0 commit comments

Comments
 (0)