From c14927df8f7856a53cb3810663fa25511e2e3fdf Mon Sep 17 00:00:00 2001 From: Test Date: Sat, 7 Mar 2026 00:29:54 +0100 Subject: [PATCH] feat: add GitPushResult with error classification for push failures Replaces raw string return from git_push with a structured GitPushResult that classifies errors as rejected/auth_error/network_error/error, each with an actionable user-facing message. Co-Authored-By: Claude Opus 4.6 --- src-tauri/src/commands.rs | 4 +- src-tauri/src/git/mod.rs | 2 +- src-tauri/src/git/remote.rs | 176 ++++++++++++++++++++++++++++++++++-- 3 files changed, 173 insertions(+), 9 deletions(-) diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 32861ea5..93b0c798 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -5,7 +5,7 @@ use crate::claude_cli::{ AgentStreamRequest, ChatStreamRequest, ClaudeCliStatus, ClaudeStreamEvent, }; use crate::frontmatter::FrontmatterValue; -use crate::git::{GitCommit, GitPullResult, LastCommitInfo, ModifiedFile, PulseCommit}; +use crate::git::{GitCommit, GitPullResult, GitPushResult, LastCommitInfo, ModifiedFile, PulseCommit}; use crate::github::{DeviceFlowPollResult, DeviceFlowStart, GitHubUser, GithubRepo}; use crate::indexing::{IndexStatus, IndexingProgress}; use crate::search::SearchResponse; @@ -276,7 +276,7 @@ pub fn git_commit_conflict_resolution(vault_path: String) -> Result Result { +pub fn git_push(vault_path: String) -> Result { let vault_path = expand_tilde(&vault_path); git::git_push(&vault_path) } diff --git a/src-tauri/src/git/mod.rs b/src-tauri/src/git/mod.rs index d5ec0945..f1d8826a 100644 --- a/src-tauri/src/git/mod.rs +++ b/src-tauri/src/git/mod.rs @@ -15,7 +15,7 @@ pub use conflict::{ }; pub use history::{get_file_diff, get_file_diff_at_commit, get_file_history}; pub use pulse::{get_last_commit_info, get_vault_pulse, LastCommitInfo, PulseCommit, PulseFile}; -pub use remote::{git_pull, git_push, has_remote, GitPullResult}; +pub use remote::{git_pull, git_push, has_remote, GitPullResult, GitPushResult}; pub use status::{get_modified_files, ModifiedFile}; use serde::Serialize; diff --git a/src-tauri/src/git/remote.rs b/src-tauri/src/git/remote.rs index b004d2e4..5506ef24 100644 --- a/src-tauri/src/git/remote.rs +++ b/src-tauri/src/git/remote.rs @@ -110,8 +110,74 @@ fn parse_updated_files(stdout: &str) -> Vec { .collect() } +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] +pub struct GitPushResult { + pub status: String, // "ok" | "rejected" | "auth_error" | "network_error" | "error" + pub message: String, +} + +/// Classify a git push stderr message into a user-friendly status and message. +pub fn classify_push_error(stderr: &str) -> GitPushResult { + let lower = stderr.to_lowercase(); + + if lower.contains("non-fast-forward") + || lower.contains("[rejected]") + || lower.contains("fetch first") + || lower.contains("failed to push some refs") + && (lower.contains("updates were rejected") || lower.contains("non-fast-forward")) + { + return GitPushResult { + status: "rejected".to_string(), + message: "Push rejected: remote has new commits. Pull first, then push.".to_string(), + }; + } + + if lower.contains("authentication failed") + || lower.contains("could not read username") + || lower.contains("permission denied") + || lower.contains("403") + || lower.contains("invalid credentials") + { + return GitPushResult { + status: "auth_error".to_string(), + message: "Push failed: authentication error. Check your credentials.".to_string(), + }; + } + + if lower.contains("could not resolve host") + || lower.contains("unable to access") + || lower.contains("connection refused") + || lower.contains("network is unreachable") + || lower.contains("timed out") + { + return GitPushResult { + status: "network_error".to_string(), + message: "Push failed: network error. Check your connection and try again.".to_string(), + }; + } + + // Fallback: extract the hint line if present, otherwise use the full stderr + let hint_line = stderr + .lines() + .find(|l| l.trim_start().starts_with("hint:")) + .map(|l| l.trim_start().strip_prefix("hint:").unwrap_or(l).trim()) + .unwrap_or("") + .to_string(); + + let detail = if hint_line.is_empty() { + stderr.trim().to_string() + } else { + hint_line + }; + + GitPushResult { + status: "error".to_string(), + message: format!("Push failed: {detail}"), + } +} + /// Push to remote. -pub fn git_push(vault_path: &str) -> Result { +pub fn git_push(vault_path: &str) -> Result { let vault = Path::new(vault_path); let output = Command::new("git") @@ -122,13 +188,13 @@ pub fn git_push(vault_path: &str) -> Result { if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - return Err(format!("git push failed: {}", stderr)); + return Ok(classify_push_error(&stderr)); } - // git push often writes to stderr even on success - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); - Ok(format!("{}{}", stdout, stderr)) + Ok(GitPushResult { + status: "ok".to_string(), + message: "Pushed to remote".to_string(), + }) } #[cfg(test)] @@ -227,6 +293,104 @@ mod tests { assert!(files.is_empty()); } + #[test] + fn test_classify_push_error_non_fast_forward() { + let stderr = r#"To github.com:user/repo.git + ! [rejected] main -> main (non-fast-forward) +error: failed to push some refs to 'github.com:user/repo.git' +hint: Updates were rejected because the remote contains work that you do not +hint: have locally."#; + let result = classify_push_error(stderr); + assert_eq!(result.status, "rejected"); + assert!(result.message.contains("Pull first")); + } + + #[test] + fn test_classify_push_error_fetch_first() { + let stderr = "error: failed to push some refs\nhint: Updates were rejected because the tip of your current branch is behind\nhint: its remote counterpart. Integrate the remote changes (e.g.\nhint: 'git pull ...') before pushing again.\nhint: See the 'Note about fast-forwards' in 'git push --help' for details.\n ! [rejected] main -> main (fetch first)\n"; + let result = classify_push_error(stderr); + assert_eq!(result.status, "rejected"); + } + + #[test] + fn test_classify_push_error_auth_failure() { + let stderr = "remote: Permission denied to user/repo.git\nfatal: unable to access 'https://github.com/user/repo.git/': The requested URL returned error: 403"; + let result = classify_push_error(stderr); + assert_eq!(result.status, "auth_error"); + assert!(result.message.contains("authentication")); + } + + #[test] + fn test_classify_push_error_network() { + let stderr = "fatal: unable to access 'https://github.com/user/repo.git/': Could not resolve host: github.com"; + let result = classify_push_error(stderr); + assert_eq!(result.status, "network_error"); + assert!(result.message.contains("network")); + } + + #[test] + fn test_classify_push_error_unknown() { + let stderr = "error: something unexpected happened\nhint: Try again later"; + let result = classify_push_error(stderr); + assert_eq!(result.status, "error"); + assert!(result.message.contains("Try again later")); + } + + #[test] + fn test_classify_push_error_unknown_no_hint() { + let stderr = "error: something totally weird"; + let result = classify_push_error(stderr); + assert_eq!(result.status, "error"); + assert!(result.message.contains("something totally weird")); + } + + #[test] + fn test_git_push_result_serialization() { + let result = GitPushResult { + status: "rejected".to_string(), + message: "Push rejected".to_string(), + }; + let json = serde_json::to_string(&result).unwrap(); + assert!(json.contains("\"rejected\"")); + let parsed: GitPushResult = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed.status, "rejected"); + } + + #[test] + fn test_git_push_success_returns_ok() { + let (_bare, clone_a, _clone_b) = setup_remote_pair(); + let vp_a = clone_a.path().to_str().unwrap(); + + fs::write(clone_a.path().join("note.md"), "# Note\n").unwrap(); + git_commit(vp_a, "initial").unwrap(); + let result = git_push(vp_a).unwrap(); + assert_eq!(result.status, "ok"); + } + + #[test] + fn test_git_push_rejected_returns_rejected() { + let (_bare, clone_a, clone_b) = setup_remote_pair(); + let vp_a = clone_a.path().to_str().unwrap(); + let vp_b = clone_b.path().to_str().unwrap(); + + // Both clones commit and push — second push should be rejected + fs::write(clone_a.path().join("note.md"), "# A\n").unwrap(); + git_commit(vp_a, "from A").unwrap(); + git_push(vp_a).unwrap(); + + git_pull(vp_b).unwrap(); + fs::write(clone_b.path().join("note.md"), "# B\n").unwrap(); + git_commit(vp_b, "from B").unwrap(); + git_push(vp_b).unwrap(); + + // Now A has a new commit but hasn't pulled B's changes + fs::write(clone_a.path().join("other.md"), "# Other\n").unwrap(); + git_commit(vp_a, "from A again").unwrap(); + let result = git_push(vp_a).unwrap(); + assert_eq!(result.status, "rejected"); + assert!(result.message.contains("Pull first")); + } + #[test] fn test_git_pull_result_serialization() { let result = GitPullResult {