Get the editor from git through `git var` #11516

mathijs81 · 2025-12-11T09:25:54Z

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 and simplify our code. This also handles the case where e.g. debian/ubuntu have compiled git with a different fallback than the "vi" default.
I kept the GIT_EDITOR check to be able to skip the git invocation if that's handy, and have the extremely simple vi/notepad fallback at the end still just in case something goes wrong with git.

vercel · 2025-12-11T09:26:02Z

@mathijs81 is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

Copilot

Pull request overview

This PR simplifies the editor command detection logic by delegating to git's built-in git var GIT_EDITOR command instead of manually implementing git's editor resolution logic. The change reduces code complexity while maintaining the same behavior and adding better support for platform-specific git defaults (e.g., Debian/Ubuntu's "editor" fallback instead of "vi").

Replaced manual environment variable checking and git config querying with a single git var GIT_EDITOR call
Removed the terminal "dumb" check logic since it's now handled by git
Updated tests to verify the new implementation with controlled environments

crates/but/src/tui/get_text.rs

Byron · 2025-12-11T10:20:33Z

Thanks a lot, I think that's the way to do it.
With the refactorings we discussed on Discord I think this is basically ready to go - please feel free to request a review when ready.

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.

This fixes e.g. "code --wait" as the editor setting.

mathijs81 · 2025-12-11T13:24:53Z

I reduced the test cases to a minimal check that git runs OR we get our fallback back.
Also made the execution of the editor command more in line with how git does it so that e.g. code --wait now works.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

crates/but/src/tui/get_text.rs

Copilot · 2025-12-11T13:30:55Z

crates/but/src/tui/get_text.rs

-/// - Return error if terminal is dumb and no editor found
-/// - Fall back to platform defaults (vi on Unix, notepad on Windows)
-/// - Add comprehensive unit tests for all scenarios
+/// Note: Because git config parsing is used, the current directory matters for potential local git config overrides.


The comment mentions "git config parsing" but this is potentially misleading. While git var GIT_EDITOR does consult git config (among other sources), the primary mechanism is that git performs its own editor resolution logic, which includes checking environment variables like GIT_EDITOR, VISUAL, EDITOR, the core.editor config, terminal detection, and platform-specific fallbacks. Consider rephrasing to: "Note: Because git's editor resolution logic is used, the current directory matters for potential local git config overrides."

Suggested change

/// Note: Because git config parsing is used, the current directory matters for potential local git config overrides.

/// Note: Because git's editor resolution logic is used (including environment variables and config),

/// the current directory matters for potential local git config overrides.

Copilot · 2025-12-11T13:30:55Z

crates/but/src/tui/get_text.rs

+/// Runs `git var GIT_EDITOR`, which lets git do its resolution of the editor command.
+/// This typically uses the git config value for `core.editor`, and env vars like `GIT_EDITOR` or `EDITOR`.


The documentation says git "typically uses" GIT_EDITOR and EDITOR, but according to Git's documentation, git var GIT_EDITOR follows a specific precedence order: GIT_EDITOR → core.editor → VISUAL (if terminal is not dumb) → EDITOR → platform default. Consider being more precise about the precedence order rather than using "typically."

Suggested change

/// Runs `git var GIT_EDITOR`, which lets git do its resolution of the editor command.

/// This typically uses the git config value for `core.editor`, and env vars like `GIT_EDITOR` or `EDITOR`.

/// Runs `git var GIT_EDITOR`, which lets git resolve the editor command according to its precedence order.

/// The precedence is: `GIT_EDITOR` environment variable, then `core.editor` config, then `VISUAL` (if terminal is not dumb), then `EDITOR`, and finally a platform default.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

- be more verbose when Git couldn't be invoked to ease remote debugging later. - don't force vars to strings, retain their original encoding - avoid hashmap in favor of iterator

Let's set an example in code that is supposed to be used in many comands, more akind to a CLI-STD.

This will make it harder to call from legacy code, but will make it clear for modern code that we don't know the encoding and shouldn't lighthartedly butcher it by converting to unicode lossily. Note that all callers now state explicitly that they handle the message lossily, and we should fix this once the code is modernised. While at it, also avoid rewriting line endings in favor of reusing the ones that the user specified.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

crates/but/src/tui/get_text.rs

Byron

Thanks a lot! I particularly like all the removed tests which felt a bit like an exercise before, and of course, the overall simplification.

While at it, I made the code a bit more idiomatic and also looked at the public functions, which I didn't do before. From there, I put it into a shape that I think should be suitable for a Cli-std if you will.

All my changes have been validated with cargo run --bin but -- -C <repo-path> describe <short-hash> (only).

Please do take a look and give it another refactoring round if you see something to improve.

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

crates/but/src/tui/get_text.rs

Copilot

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

…will follow soon)

Copilot

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Copilot · 2025-12-12T10:05:07Z

crates/gitbutler-url/src/parse.rs

    {
-        let path =
-            path_without_file_protocol.map_or_else(|| input.into(), |stripped_path| stripped_path);
+        let path = path_without_file_protocol.unwrap_or_else(|| input.into());


The refactor changes the logic subtly. The original map_or_else preserves path_without_file_protocol when it's Some, but the new code calls unwrap_or_else which would panic if path_without_file_protocol is Some. Consider using map_or_else or unwrap_or(input.into()) instead to maintain the original behavior.

Suggested change

let path = path_without_file_protocol.unwrap_or_else(|| input.into());

let path = path_without_file_protocol.map_or_else(|| input.into(), |p| p.into());

Copilot · 2025-12-12T10:05:08Z

crates/but/src/tui/get_text.rs

+    let mut cmd = std::process::Command::new(gix::path::env::exe_invocation());
+    let res = cmd
+        .args(["var", "GIT_EDITOR"])
+        .env_clear()
+        .envs(env)
+        .output();
+    if res.is_err() {
+        // Avoid logging explicit env vars
+        cmd.env_clear();
+        tracing::warn!(
+            ?res,
+            ?cmd,


The variable name cmd is very generic. Consider renaming it to git_cmd or git_var_cmd to better reflect that it's specifically invoking git var.

Suggested change

let mut cmd = std::process::Command::new(gix::path::env::exe_invocation());

let res = cmd

.args(["var", "GIT_EDITOR"])

.env_clear()

.envs(env)

.output();

if res.is_err() {

// Avoid logging explicit env vars

cmd.env_clear();

tracing::warn!(

?res,

?cmd,

let mut git_var_cmd = std::process::Command::new(gix::path::env::exe_invocation());

let res = git_var_cmd

.args(["var", "GIT_EDITOR"])

.env_clear()

.envs(env)

.output();

if res.is_err() {

// Avoid logging explicit env vars

git_var_cmd.env_clear();

tracing::warn!(

?res,

?git_var_cmd,

crates/but-rebase/src/lib.rs

Copilot AI review requested due to automatic review settings December 11, 2025 09:25

github-actions bot added the rust Pull requests that update Rust code label Dec 11, 2025

Copilot started reviewing on behalf of mathijs81 December 11, 2025 09:26 View session

mathijs81 mentioned this pull request Dec 11, 2025

Fix default editor on debian-derived distros #11511

Closed

Copilot AI reviewed Dec 11, 2025

View reviewed changes

crates/but/src/tui/get_text.rs Outdated Show resolved Hide resolved

crates/but/src/tui/get_text.rs Outdated Show resolved Hide resolved

crates/but/src/tui/get_text.rs Outdated Show resolved Hide resolved

mathijs81 force-pushed the mv-editor-through-var branch from e3de940 to b7792e5 Compare December 11, 2025 09:33

Byron marked this pull request as draft December 11, 2025 10:20

mathijs81 added 2 commits December 11, 2025 12:31

Spawn editor using shell like git does

9d2a6b4

This fixes e.g. "code --wait" as the editor setting.

mathijs81 force-pushed the mv-editor-through-var branch from b7792e5 to 9d2a6b4 Compare December 11, 2025 13:08

mathijs81 marked this pull request as ready for review December 11, 2025 13:25

Copilot AI review requested due to automatic review settings December 11, 2025 13:25

Copilot started reviewing on behalf of mathijs81 December 11, 2025 13:25 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

Byron added 3 commits December 12, 2025 09:12

refactor

9e0f170

- be more verbose when Git couldn't be invoked to ease remote debugging later. - don't force vars to strings, retain their original encoding - avoid hashmap in favor of iterator

Avoid manual tempfile implementation in get_text::from_editor()

0b8c1ae

Let's set an example in code that is supposed to be used in many comands, more akind to a CLI-STD.

Byron requested a review from Copilot December 12, 2025 08:13

Copilot started reviewing on behalf of Byron December 12, 2025 08:13 View session

Byron requested review from Copilot and removed request for Copilot December 12, 2025 08:14

Copilot started reviewing on behalf of Byron December 12, 2025 08:14 View session

Copilot AI reviewed Dec 12, 2025

View reviewed changes

crates/but/src/tui/get_text.rs Show resolved Hide resolved

crates/but/src/tui/get_text.rs Show resolved Hide resolved

crates/but/src/tui/get_text.rs Outdated Show resolved Hide resolved

Byron approved these changes Dec 12, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings December 12, 2025 08:47

Byron force-pushed the mv-editor-through-var branch from 4fa3738 to 278d80c Compare December 12, 2025 08:47

Byron enabled auto-merge December 12, 2025 08:47

Copilot started reviewing on behalf of Byron December 12, 2025 08:47 View session

Copilot AI reviewed Dec 12, 2025

View reviewed changes

crates/but/src/tui/get_text.rs Show resolved Hide resolved

Copilot AI review requested due to automatic review settings December 12, 2025 08:50

Copilot started reviewing on behalf of Byron December 12, 2025 08:50 View session

Copilot AI reviewed Dec 12, 2025

View reviewed changes

Address copilot review

60845d2

Byron force-pushed the mv-editor-through-var branch from 9979c02 to 60845d2 Compare December 12, 2025 09:28

Byron added 2 commits December 12, 2025 10:56

Fix cargo clippy issues that show up with Rust 1.92

04b81f9

Switch to Rust 1.92 officially (as our image seems to use it, and CI …

e5f89c1

…will follow soon)

Copilot AI review requested due to automatic review settings December 12, 2025 09:58

Copilot started reviewing on behalf of Byron December 12, 2025 09:59 View session

Copilot AI reviewed Dec 12, 2025

View reviewed changes

Byron merged commit 91a32ee into gitbutlerapp:master Dec 12, 2025
26 of 27 checks passed

	/// Note: Because git config parsing is used, the current directory matters for potential local git config overrides.
	/// Note: Because git's editor resolution logic is used (including environment variables and config),
	/// the current directory matters for potential local git config overrides.

		/// Runs `git var GIT_EDITOR`, which lets git do its resolution of the editor command.
		/// This typically uses the git config value for `core.editor`, and env vars like `GIT_EDITOR` or `EDITOR`.

	let path = path_without_file_protocol.unwrap_or_else(\|\| input.into());
	let path = path_without_file_protocol.map_or_else(\|\| input.into(), \|p\| p.into());

Get the editor from git through git var #11516

Get the editor from git through git var #11516

Conversation

mathijs81 commented Dec 11, 2025

Uh oh!

vercel bot commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Byron commented Dec 11, 2025

Uh oh!

mathijs81 commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Get the editor from git through `git var` #11516

Get the editor from git through `git var` #11516