diff --git a/src/git/error.rs b/src/git/error.rs index 687a9de35..3ef7cdc0e 100644 --- a/src/git/error.rs +++ b/src/git/error.rs @@ -219,14 +219,14 @@ impl CommandError { /// Build from the captured `Output` of a non-zero exit. pub fn from_failed_output( program: impl Into, - args: &[&str], + args: &[impl AsRef], output: &std::process::Output, ) -> Self { let stderr = String::from_utf8_lossy(&output.stderr).replace('\r', "\n"); let stdout = String::from_utf8_lossy(&output.stdout).replace('\r', "\n"); Self { program: program.into(), - args: args.iter().map(|&s| s.to_string()).collect(), + args: args.iter().map(|s| s.as_ref().to_string()).collect(), stderr, stdout, exit_code: output.status.code(), diff --git a/src/git/repository/config.rs b/src/git/repository/config.rs index 0a0fd1472..392c192de 100644 --- a/src/git/repository/config.rs +++ b/src/git/repository/config.rs @@ -7,6 +7,8 @@ use color_print::cformat; use crate::config::ProjectConfig; +use crate::git::CommandError; + use super::{DefaultBranchName, GitError, Repository}; impl Repository { @@ -67,15 +69,15 @@ impl Repository { /// Removes the canonical form (matching what git emits) to stay in /// sync with `set_config_value` and `config_last`. pub(super) fn unset_config_value(&self, key: &str) -> anyhow::Result { - let output = self.run_command_output(&["config", "--unset", key])?; + let args = ["config", "--unset", key]; + let output = self.run_command_output(&args)?; let existed = if output.status.success() { true } else if output.status.code() == Some(5) { // --unset exit code 5 = key didn't exist false } else { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("git config --unset {}: {}", key, stderr.trim()); + return Err(CommandError::from_failed_output("git", &args, &output).into()); }; if let Some(lock) = self.cache.all_config.get() { // `shift_remove` preserves remaining order (swap_remove would @@ -94,15 +96,15 @@ impl Repository { /// surfaced as `Err`). Use this instead of `run_command` + `.unwrap_or_default()`, /// which conflates the two. pub fn get_config_regexp(&self, pattern: &str) -> anyhow::Result { - let output = self.run_command_output(&["config", "--get-regexp", pattern])?; + let args = ["config", "--get-regexp", pattern]; + let output = self.run_command_output(&args)?; if output.status.success() { Ok(String::from_utf8_lossy(&output.stdout).into_owned()) } else if output.status.code() == Some(1) { // Exit 1 = no keys matched the pattern Ok(String::new()) } else { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("git config --get-regexp {}: {}", pattern, stderr.trim()); + Err(CommandError::from_failed_output("git", &args, &output).into()) } } @@ -824,6 +826,44 @@ mod tests { assert_eq!(output, ""); } + #[test] + fn test_get_config_regexp_failure_is_command_error() { + // A real failure (invalid pattern, exit 6) must surface as a typed + // `CommandError` — unlike exit 1, which means "no keys matched". + let test = TestRepo::with_initial_commit(); + let repo = Repository::at(test.root_path()).unwrap(); + + let err = repo.get_config_regexp("(").unwrap_err(); + let cmd_err = CommandError::find_in(&err).expect("error should carry a CommandError"); + assert_eq!(cmd_err.command_string(), "git config --get-regexp ("); + } + + #[test] + fn test_unset_config_failure_is_command_error() { + // A real failure (invalid key, exit 1) must surface as a typed + // `CommandError` — unlike exit 5, which means "key didn't exist". + let test = TestRepo::with_initial_commit(); + let repo = Repository::at(test.root_path()).unwrap(); + + let err = repo.unset_config("inva lid.key").unwrap_err(); + let cmd_err = CommandError::find_in(&err).expect("error should carry a CommandError"); + assert_eq!(cmd_err.command_string(), "git config --unset inva lid.key"); + } + + #[test] + fn test_config_read_failure_is_command_error() { + // Corrupting the config after the repository is open (the bulk map + // populates lazily) makes `git config --list -z` fail — the failure + // must surface as a typed `CommandError`. + let test = TestRepo::with_initial_commit(); + let repo = Repository::at(test.root_path()).unwrap(); + std::fs::write(test.root_path().join(".git/config"), "[bad\n").unwrap(); + + let err = repo.config_value("user.name").unwrap_err(); + let cmd_err = CommandError::find_in(&err).expect("error should carry a CommandError"); + assert_eq!(cmd_err.command_string(), "git config --list -z"); + } + #[test] fn test_get_config_regexp_returns_matches() { let test = TestRepo::with_initial_commit(); diff --git a/src/git/repository/diff.rs b/src/git/repository/diff.rs index b0775b76a..c170afb08 100644 --- a/src/git/repository/diff.rs +++ b/src/git/repository/diff.rs @@ -270,15 +270,18 @@ impl Repository { } // Exit codes: 0 = found, 1 = no common ancestor, 128+ = invalid ref - let output = self.run_command_output(&["merge-base", sha1, sha2])?; + let args = ["merge-base", sha1, sha2]; + let output = self.run_command_output(&args)?; let result = if output.status.success() { Some(String::from_utf8_lossy(&output.stdout).trim().to_owned()) } else if output.status.code() == Some(1) { None } else { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("git merge-base failed for {sha1} {sha2}: {}", stderr.trim()); + return Err(crate::git::CommandError::from_failed_output( + "git", &args, &output, + ) + .into()); }; super::sha_cache::put_merge_base(self, sha1, sha2, &result); diff --git a/src/git/repository/integration.rs b/src/git/repository/integration.rs index d03737124..48d0a04a4 100644 --- a/src/git/repository/integration.rs +++ b/src/git/repository/integration.rs @@ -291,15 +291,17 @@ impl Repository { /// than success is an error; exit 0 → [`MergeTreeOutcome::Clean`] with the /// resulting tree SHA (first line of stdout). fn compute_merge_tree_outcome(&self, a: &str, b: &str) -> anyhow::Result { - // Exit codes: 0 = clean merge, 1 = conflicts, 128+ = error (invalid ref, corrupt repo) - let output = self.run_command_output(&["merge-tree", "--write-tree", a, b])?; + // Exit codes: 0 = clean merge, 1 = conflicts (git also uses 1 for + // unresolvable args — callers pass pre-resolved commit SHAs, see + // `run_merge_tree`), anything else = error (corrupt repo, bad usage) + let args = ["merge-tree", "--write-tree", a, b]; + let output = self.run_command_output(&args)?; if output.status.code() == Some(1) { return Ok(MergeTreeOutcome::Conflict); } if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("git merge-tree failed for {a} {b}: {}", stderr.trim()); + return Err(crate::git::CommandError::from_failed_output("git", &args, &output).into()); } // Clean merge — first line of stdout is the resulting tree SHA. @@ -1129,6 +1131,30 @@ mod patch_id_tests { } } +#[cfg(test)] +mod merge_tree_error_tests { + use super::*; + use crate::testing::TestRepo; + + /// A hard `git merge-tree` failure (here: a fatal config error, exit + /// 128) must surface as a typed `CommandError`, distinct from exit 1, + /// which means "merged with conflicts". + #[test] + fn hard_failure_is_command_error() { + let test = TestRepo::with_initial_commit(); + let repo = Repository::at(test.root_path()).unwrap(); + std::fs::write(test.root_path().join(".git/config"), "[bad\n").unwrap(); + + let err = repo.compute_merge_tree_outcome("HEAD", "HEAD").unwrap_err(); + let cmd_err = + crate::git::CommandError::find_in(&err).expect("error should carry a CommandError"); + assert_eq!( + cmd_err.command_string(), + "git merge-tree --write-tree HEAD HEAD" + ); + } +} + #[cfg(test)] mod merge_tree_cache_tests { use super::*; diff --git a/src/git/repository/mod.rs b/src/git/repository/mod.rs index 43d5d713f..49a9650dc 100644 --- a/src/git/repository/mod.rs +++ b/src/git/repository/mod.rs @@ -118,7 +118,7 @@ use color_print::cformat; use dashmap::DashMap; use once_cell::sync::OnceCell; -use anyhow::{Context, bail}; +use anyhow::Context; use dunce::canonicalize; use crate::config::{LoadError, ProjectConfig, ResolvedConfig, UserConfig}; @@ -891,16 +891,18 @@ impl Repository { // captures the surrounding canonicalize + cache-insert work too. let _span = crate::trace::Span::new("resolve_git_common_dir"); + let args = ["rev-parse", "--git-common-dir"]; let output = Cmd::new("git") - .args(["rev-parse", "--git-common-dir"]) + .args(args) .current_dir(discovery_path) .context(path_to_logging_context(discovery_path)) .run() .context("Failed to execute: git rev-parse --git-common-dir")?; if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("{}", stderr.trim()); + return Err( + super::error::CommandError::from_failed_output("git", &args, &output).into(), + ); } let stdout = String::from_utf8_lossy(&output.stdout); @@ -1189,15 +1191,17 @@ impl Repository { &self, ) -> anyhow::Result<&std::sync::RwLock>>> { self.cache.all_config.get_or_try_init(|| { + let args = ["config", "--list", "-z"]; let output = Cmd::new("git") - .args(["config", "--list", "-z"]) + .args(args) .current_dir(&self.git_common_dir) .context(path_to_logging_context(&self.git_common_dir)) .run() .context("failed to read git config")?; if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("git config --list failed: {}", stderr.trim()); + return Err( + super::error::CommandError::from_failed_output("git", &args, &output).into(), + ); } Ok(std::sync::RwLock::new(parse_config_list_z(&output.stdout))) }) diff --git a/src/git/repository/working_tree.rs b/src/git/repository/working_tree.rs index 14858ca87..627b09fbd 100644 --- a/src/git/repository/working_tree.rs +++ b/src/git/repository/working_tree.rs @@ -518,14 +518,12 @@ impl<'a> WorkingTree<'a> { "--".to_string(), ]; args.extend(paths); - let command = args.join(" "); let output = idx - .git(args) + .git(&args) .run() .context("Failed to compute untracked diff stats")?; if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("git {} failed: {}", command, stderr.trim()); + return Err(CommandError::from_failed_output("git", &args, &output).into()); } let mut stats = LineDiff::default(); @@ -932,6 +930,25 @@ mod tests { ); } + #[test] + fn untracked_diff_stats_unborn_head_is_command_error() { + // With an unborn HEAD the untracked files stage fine into the temp + // index, but `git diff --cached --numstat HEAD` cannot resolve HEAD — + // the failure must surface as a typed `CommandError`. + let test = TestRepo::new(); + std::fs::write(test.root_path().join("new.txt"), "hello\n").unwrap(); + let repo = Repository::at(test.root_path()).unwrap(); + + let err = repo.current_worktree().untracked_diff_stats().unwrap_err(); + let cmd_err = + crate::git::CommandError::find_in(&err).expect("error should carry a CommandError"); + assert!( + cmd_err + .command_string() + .starts_with("git diff --cached --numstat HEAD") + ); + } + #[test] fn temp_index_tolerates_missing_real_index() { // A worktree whose `/index` file is absent must not error diff --git a/src/llm.rs b/src/llm.rs index f64a9787d..2537c33b6 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -7,7 +7,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; use worktrunk::config::CommitGenerationConfig; -use worktrunk::git::{CommitMessageDetail, Repository}; +use worktrunk::git::{CommandError, CommitMessageDetail, Repository}; use worktrunk::path::format_path_for_display; use worktrunk::shell_exec::{Cmd, ShellConfig}; use worktrunk::styling::{eprintln, warning_message}; @@ -767,18 +767,16 @@ pub(crate) fn generate_commit_message( // Check if commit generation is configured (non-empty command) if commit_generation_config.is_configured() { let command = commit_generation_config.command.as_ref().unwrap(); + // Prompt-build failures (git plumbing) propagate as-is; only a + // failure of the LLM command itself gets the `LlmCommandFailed` + // wrapper — mirroring `generate_squash_message`. + let prompt = build_commit_prompt(commit_generation_config, index_override, project_append)?; // A slow or hung command is otherwise silent (stdout is captured); the // watchdog surfaces a "still waiting" status. Held until this function // returns, clearing the block before the caller prints the message. let _watchdog = watch_llm_command(command, "the commit message"); // Commit generation is explicitly configured - fail if it doesn't work - return try_generate_commit_message( - command, - commit_generation_config, - index_override, - project_append, - ) - .map_err(|e| { + return execute_llm_command(command, &prompt).map_err(|e| { worktrunk::git::GitError::LlmCommandFailed { command: command.clone(), error: e.to_string(), @@ -793,13 +791,11 @@ pub(crate) fn generate_commit_message( // Fallback: generate a descriptive commit message based on changed files let repo = Repository::current()?; - let mut name_only = Cmd::new("git") - .args(["diff", "--staged", "--name-only", "-z"]) - .current_dir(repo.discovery_path()); - if let Some(path) = index_override { - name_only = name_only.env("GIT_INDEX_FILE", path); - } - let file_list = run_git_capture(name_only, "diff --staged --name-only")?; + let file_list = run_git_capture( + &["diff", "--staged", "--name-only", "-z"], + repo.discovery_path(), + index_override, + )?; let staged_files = file_list .split('\0') .map(|s| s.trim()) @@ -826,28 +822,26 @@ pub(crate) fn generate_commit_message( Ok(message) } -fn try_generate_commit_message( - command: &str, - config: &CommitGenerationConfig, - index_override: Option<&Path>, - project_append: Option<&str>, -) -> anyhow::Result { - let prompt = build_commit_prompt(config, index_override, project_append)?; - execute_llm_command(command, &prompt) -} - -/// Run a git `Cmd` and bail on non-zero exit, mirroring [`Repository::run_command`]. +/// Run a git command and capture stdout, mirroring [`Repository::run_command`] +/// (including its [`CommandError`] on non-zero exit). /// /// Used by call sites that need to set `GIT_INDEX_FILE` (`--dry-run`) and so can't go /// through `Repository::run_command`. Without this check, a failing `git diff` would /// silently feed an empty diff to the LLM. -fn run_git_capture(cmd: Cmd, what: &str) -> anyhow::Result { +fn run_git_capture( + args: &[&str], + cwd: &Path, + index_override: Option<&Path>, +) -> anyhow::Result { + let mut cmd = Cmd::new("git").args(args.iter().copied()).current_dir(cwd); + if let Some(index) = index_override { + cmd = cmd.env("GIT_INDEX_FILE", index); + } let output = cmd .run() - .with_context(|| format!("Failed to execute git {what}"))?; + .with_context(|| format!("Failed to execute: git {}", args.join(" ")))?; if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("git {what} failed: {}", stderr.trim()); + return Err(CommandError::from_failed_output("git", args, &output).into()); } Ok(String::from_utf8_lossy(&output.stdout).into_owned()) } @@ -867,21 +861,16 @@ pub(crate) fn build_commit_prompt( project_append: Option<&str>, ) -> anyhow::Result { let repo = Repository::current()?; - let cwd = repo.discovery_path().to_path_buf(); - - let mut diff_cmd = Cmd::new("git") - .args(DIFF_PREFIX_OVERRIDES) - .args(["--no-pager", "diff", "--staged"]) - .current_dir(&cwd); - let mut diff_stat_cmd = Cmd::new("git") - .args(["--no-pager", "diff", "--staged", "--stat"]) - .current_dir(&cwd); - if let Some(index) = index_override { - diff_cmd = diff_cmd.env("GIT_INDEX_FILE", index); - diff_stat_cmd = diff_stat_cmd.env("GIT_INDEX_FILE", index); - } - let diff_output = run_git_capture(diff_cmd, "diff --staged")?; - let diff_stat = run_git_capture(diff_stat_cmd, "diff --staged --stat")?; + let cwd = repo.discovery_path(); + + let mut diff_args: Vec<&str> = DIFF_PREFIX_OVERRIDES.to_vec(); + diff_args.extend(["--no-pager", "diff", "--staged"]); + let diff_output = run_git_capture(&diff_args, cwd, index_override)?; + let diff_stat = run_git_capture( + &["--no-pager", "diff", "--staged", "--stat"], + cwd, + index_override, + )?; // Prepare diff (may filter if too large) let prepared = prepare_diff(diff_output, diff_stat); @@ -1087,17 +1076,18 @@ mod tests { ); } - /// `run_git_capture` must surface a non-zero exit as an error that names the `what` - /// label and includes the captured stderr — without that, the dry-run path would - /// feed an empty diff to the LLM on a `git` failure. + /// `run_git_capture` must surface a non-zero exit as a typed [`CommandError`] + /// carrying the command and its captured stderr — without that, the dry-run + /// path would feed an empty diff to the LLM on a `git` failure. #[test] fn test_run_git_capture_bails_on_nonzero_exit() { - let cmd = Cmd::new("git").args(["frobnicate-nonexistent"]); - let err = run_git_capture(cmd, "frobnicate").unwrap_err(); - let msg = format!("{err}"); + let err = run_git_capture(&["frobnicate-nonexistent"], Path::new("."), None).unwrap_err(); + let cmd_err = CommandError::find_in(&err).expect("error should carry a CommandError"); + assert_eq!(cmd_err.command_string(), "git frobnicate-nonexistent"); assert!( - msg.contains("git frobnicate failed:"), - "error message should name the `what` label; got: {msg}" + cmd_err.stderr.contains("frobnicate-nonexistent"), + "stderr should name the failing command; got: {}", + cmd_err.stderr ); } diff --git a/tests/snapshots/integration__integration_tests__list__list_shows_warning_on_git_error.snap b/tests/snapshots/integration__integration_tests__list__list_shows_warning_on_git_error.snap index 4f8fe2189..3c9996d54 100644 --- a/tests/snapshots/integration__integration_tests__list__list_shows_warning_on_git_error.snap +++ b/tests/snapshots/integration__integration_tests__list__list_shows_warning_on_git_error.snap @@ -35,6 +35,7 @@ info: WORKTRUNK_TEST_GEMINI_INSTALLED: "0" WORKTRUNK_TEST_NUSHELL_ENV: "0" WORKTRUNK_TEST_OPENCODE_INSTALLED: "0" + WORKTRUNK_TEST_PARENT_SHELL: "" WORKTRUNK_TEST_POWERSHELL_ENV: "0" WORKTRUNK_TEST_POWERSHELL_INSTALLED: "0" WORKTRUNK_TEST_SKIP_URL_HEALTH_CHECK: "1" @@ -57,8 +58,8 @@ exit_code: 0 ○ Showing 5 worktrees, 3 ahead; 10 tasks failed ▲ 10 tasks failed:   main: upstream status (fatal: missing object 0000000000000000000000000000000000000001 for refs/heads/feature) -  feature: ahead/behind counts (git merge-base failed for 05a4a45d0b981dad5c27db59dca482836d59f89e 0000000000000000000000000000000000000001: fatal: Not a valid commit name 0000000000000000000000000000000000000001) -  feature: branch diff (git merge-base failed for 05a4a45d0b981dad5c27db59dca482836d59f89e 0000000000000000000000000000000000000001: fatal: Not a valid commit name 0000000000000000000000000000000000000001) +  feature: ahead/behind counts (fatal: Not a valid commit name 0000000000000000000000000000000000000001) +  feature: branch diff (fatal: Not a valid commit name 0000000000000000000000000000000000000001)   feature: working-tree diff (fatal: bad object HEAD)   feature: merge-conflict check (fatal: bad object HEAD)   feature: working-tree conflict check (fatal: bad object HEAD) diff --git a/tests/snapshots/integration__integration_tests__list__list_warns_when_commit_details_batch_fails.snap b/tests/snapshots/integration__integration_tests__list__list_warns_when_commit_details_batch_fails.snap index 78e626156..337bf5ba4 100644 --- a/tests/snapshots/integration__integration_tests__list__list_warns_when_commit_details_batch_fails.snap +++ b/tests/snapshots/integration__integration_tests__list__list_warns_when_commit_details_batch_fails.snap @@ -35,6 +35,7 @@ info: WORKTRUNK_TEST_GEMINI_INSTALLED: "0" WORKTRUNK_TEST_NUSHELL_ENV: "0" WORKTRUNK_TEST_OPENCODE_INSTALLED: "0" + WORKTRUNK_TEST_PARENT_SHELL: "" WORKTRUNK_TEST_POWERSHELL_ENV: "0" WORKTRUNK_TEST_POWERSHELL_INSTALLED: "0" WORKTRUNK_TEST_SKIP_URL_HEALTH_CHECK: "1" @@ -56,9 +57,9 @@ exit_code: 0 ○ Showing 5 worktrees, 3 ahead; 6 tasks failed ▲ 6 tasks failed: -  (detached): ahead/behind counts (git merge-base failed for 05a4a45d0b981dad5c27db59dca482836d59f89e 0000000000000000000000000000000000000001: fatal: Not a valid commit name 0000000000000000000000000000000000000001) +  (detached): ahead/behind counts (fatal: Not a valid commit name 0000000000000000000000000000000000000001)   (detached): trees-match check (fatal: Needed a single revision) -  (detached): branch diff (git merge-base failed for 05a4a45d0b981dad5c27db59dca482836d59f89e 0000000000000000000000000000000000000001: fatal: Not a valid commit name 0000000000000000000000000000000000000001) +  (detached): branch diff (fatal: Not a valid commit name 0000000000000000000000000000000000000001)   (detached): working-tree diff (fatal: bad object HEAD)   (detached): merge-conflict check (fatal: bad object HEAD)   (detached): working-tree conflict check (fatal: bad object HEAD)