From 1ec6243e8db519121418a2059723986d38969450 Mon Sep 17 00:00:00 2001 From: cst8t <1810150+cst8t@users.noreply.github.com> Date: Wed, 9 Sep 2026 23:02:20 +0100 Subject: [PATCH 1/5] feat(git): show hook progress for pull, merge, and rebase operations Stream integration hook output, support safe bypass retries, roll back rejected merges, and report post-operation hook failures as warnings. --- src-tauri/src/commands/history.rs | 38 +- src-tauri/src/commands/repo.rs | 22 +- src-tauri/src/git/cli.rs | 503 ++++++++++++++++++++- src-tauri/src/git/handler.rs | 48 ++ src-tauri/tests/git.rs | 463 ++++++++++++++++++- src/api/commands.ts | 41 +- src/components/ProjectView.tsx | 72 ++- src/components/centre/CentrePanel.test.tsx | 46 ++ src/components/centre/CentrePanel.tsx | 8 +- src/hooks/useRemoteOperations.test.tsx | 6 + src/hooks/useRemoteOperations.ts | 24 +- src/i18n/locales/en/centre.json | 13 +- src/i18n/locales/en/projectView.json | 12 + src/types.ts | 2 +- 14 files changed, 1221 insertions(+), 77 deletions(-) diff --git a/src-tauri/src/commands/history.rs b/src-tauri/src/commands/history.rs index 56c4e99..e465909 100644 --- a/src-tauri/src/commands/history.rs +++ b/src-tauri/src/commands/history.rs @@ -1,12 +1,14 @@ use crate::AppState; use crate::git::types::{ CherryPickRequest, CherryPickResult, CommitHistoryItem, CommitHistoryRequest, - CommitVerification, FileRequest, MergeRequest, MergeResult, OperationResult, RebaseRequest, - RebaseResult, RepoRequest, ResetRequest, RevertCommitRequest, SignatureStatus, + CommitProgressEvent, CommitVerification, FileRequest, GitHookAttemptResult, MergeRequest, + MergeResult, OperationResult, RebaseRequest, RebaseResult, RepoRequest, ResetRequest, + RevertCommitRequest, SignatureStatus, }; use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::process::Command; +use std::sync::Arc; use std::time::{SystemTime, UNIX_EPOCH}; use tauri::Manager; @@ -696,10 +698,18 @@ mod tests { #[tauri::command] pub async fn merge_branch( request: MergeRequest, + skip_hooks: bool, + on_progress: tauri::ipc::Channel, app: tauri::AppHandle, -) -> Result { +) -> Result, String> { tauri::async_runtime::spawn_blocking(move || { - app.state::().git_service.merge_branch(request) + app.state::() + .git_service + .merge_branch_with_progress( + request, + skip_hooks, + Arc::new(move |event| drop(on_progress.send(event))), + ) }) .await .map_err(|e| e.to_string())? @@ -722,10 +732,16 @@ pub async fn merge_abort( #[tauri::command] pub async fn rebase_start( request: RebaseRequest, + on_progress: tauri::ipc::Channel, app: tauri::AppHandle, -) -> Result { +) -> Result, String> { tauri::async_runtime::spawn_blocking(move || { - app.state::().git_service.rebase_start(request) + app.state::() + .git_service + .rebase_start_with_progress( + request, + Arc::new(move |event| drop(on_progress.send(event))), + ) }) .await .map_err(|e| e.to_string())? @@ -735,10 +751,16 @@ pub async fn rebase_start( #[tauri::command] pub async fn rebase_continue( request: RepoRequest, + on_progress: tauri::ipc::Channel, app: tauri::AppHandle, -) -> Result { +) -> Result, String> { tauri::async_runtime::spawn_blocking(move || { - app.state::().git_service.rebase_continue(request) + app.state::() + .git_service + .rebase_continue_with_progress( + request, + Arc::new(move |event| drop(on_progress.send(event))), + ) }) .await .map_err(|e| e.to_string())? diff --git a/src-tauri/src/commands/repo.rs b/src-tauri/src/commands/repo.rs index edeb0b2..1a613b4 100644 --- a/src-tauri/src/commands/repo.rs +++ b/src-tauri/src/commands/repo.rs @@ -2478,10 +2478,18 @@ pub fn get_repo_diff_tool( #[tauri::command] pub async fn pull_changes( request: RepoRequest, + skip_hooks: bool, + on_progress: tauri::ipc::Channel, app: tauri::AppHandle, -) -> Result { +) -> Result, String> { tauri::async_runtime::spawn_blocking(move || { - app.state::().git_service.pull_changes(request) + app.state::() + .git_service + .pull_changes_with_progress( + request, + skip_hooks, + Arc::new(move |event| drop(on_progress.send(event))), + ) }) .await .map_err(|e| e.to_string())? @@ -2502,12 +2510,18 @@ pub fn analyze_pull( #[tauri::command] pub async fn pull_with_strategy( request: PullStrategyRequest, + skip_hooks: bool, + on_progress: tauri::ipc::Channel, app: tauri::AppHandle, -) -> Result { +) -> Result, String> { tauri::async_runtime::spawn_blocking(move || { app.state::() .git_service - .pull_with_strategy(request) + .pull_with_strategy_with_progress( + request, + skip_hooks, + Arc::new(move |event| drop(on_progress.send(event))), + ) }) .await .map_err(|e| e.to_string())? diff --git a/src-tauri/src/git/cli.rs b/src-tauri/src/git/cli.rs index a082e6d..7e3b31d 100644 --- a/src-tauri/src/git/cli.rs +++ b/src-tauri/src/git/cli.rs @@ -21,17 +21,16 @@ use super::types::{ DiffHunk, DiffLine, DiffLineKind, DiffRequest, ExportCommitPatchRequest, ExportPatchFileSelection, ExportPatchRequest, ExportPatchScope, ExternalDiffRequest, FetchRequest, FileDiff, FileRequest, FileStatusItem, GitHookAttemptResult, GitHookFailure, - GitIdentity, HunkStageRequest, - IdentityRequest, IdentityScope, ImportPatchRequest, LineEndingStyle, MergeRequest, MergeResult, - NumstatRequest, NumstatResult, OperationResult, PruneRemoteRequest, PullAnalysis, - PullRecommendedAction, PullState, PullStrategy, PullStrategyRequest, PushFailureKind, - PushRejectionAnalysis, PushRequest, PushResult, PushTagRequest, RebaseRequest, RebaseResult, - RemoteInfo, RemoveRemoteRequest, RenameBranchRequest, RenameRemoteRequest, RepoRequest, - RepoStatus, ResetMode, ResetRequest, RevertCommitRequest, SetBranchUpstreamRequest, - SetIdentityRequest, SetRemoteUrlRequest, SignatureStatus, SshAllowedSignerReason, - SshAllowedSignerStatus, StageFilesRequest, StashEntry, StashPushRequest, StashRequest, - SubmoduleActionRequest, SubmoduleState, SubmoduleStatus, TagInfo, UnversionedItem, - UnversionedItemKind, UpstreamStatus, + GitIdentity, HunkStageRequest, IdentityRequest, IdentityScope, ImportPatchRequest, + LineEndingStyle, MergeRequest, MergeResult, NumstatRequest, NumstatResult, OperationResult, + PruneRemoteRequest, PullAnalysis, PullRecommendedAction, PullState, PullStrategy, + PullStrategyRequest, PushFailureKind, PushRejectionAnalysis, PushRequest, PushResult, + PushTagRequest, RebaseRequest, RebaseResult, RemoteInfo, RemoveRemoteRequest, + RenameBranchRequest, RenameRemoteRequest, RepoRequest, RepoStatus, ResetMode, ResetRequest, + RevertCommitRequest, SetBranchUpstreamRequest, SetIdentityRequest, SetRemoteUrlRequest, + SignatureStatus, SshAllowedSignerReason, SshAllowedSignerStatus, StageFilesRequest, StashEntry, + StashPushRequest, StashRequest, SubmoduleActionRequest, SubmoduleState, SubmoduleStatus, + TagInfo, UnversionedItem, UnversionedItemKind, UpstreamStatus, }; pub struct CliGitHandler; @@ -43,6 +42,13 @@ struct HookCommandOutput { hooks: Vec<(String, String, Option)>, } +#[derive(Clone, Copy)] +enum PullIntegrationKind { + Auto, + Merge, + Rebase, +} + #[derive(Debug, Clone)] struct ConfiguredSubmodule { name: String, @@ -781,8 +787,14 @@ impl CliGitHandler { let Ok(event) = serde_json::from_str::(line) else { continue; }; - let session_id = event.get("sid").and_then(serde_json::Value::as_str); - if session_id != Some(root_session_id.as_str()) { + let Some(session_id) = event.get("sid").and_then(serde_json::Value::as_str) else { + continue; + }; + if session_id != root_session_id + && !session_id + .strip_prefix(root_session_id.as_str()) + .is_some_and(|suffix| suffix.starts_with('/')) + { continue; } let child_id = event.get("child_id").and_then(serde_json::Value::as_u64); @@ -795,7 +807,7 @@ impl CliGitHandler { child_id, event.get("hook_name").and_then(serde_json::Value::as_str), ) { - hooks.push((child_id, hook_name.to_string())); + hooks.push((format!("{session_id}:{child_id}"), hook_name.to_string())); } } Some("child_exit") => { @@ -803,7 +815,7 @@ impl CliGitHandler { child_id, event.get("code").and_then(serde_json::Value::as_i64), ) { - exit_codes.insert(child_id, code as i32); + exit_codes.insert(format!("{session_id}:{child_id}"), code as i32); } } _ => {} @@ -811,13 +823,7 @@ impl CliGitHandler { } hooks .into_iter() - .map(|(child_id, hook_name)| { - ( - format!("{root_session_id}:{child_id}"), - hook_name, - exit_codes.get(&child_id).copied(), - ) - }) + .map(|(key, hook_name)| (key.clone(), hook_name, exit_codes.get(&key).copied())) .collect() } @@ -1891,6 +1897,107 @@ impl CliGitHandler { } } + fn execute_pull_command_with_progress( + &self, + repo_path: &Path, + mut args: Vec, + success_message: &str, + conflict_message: &str, + integration_kind: PullIntegrationKind, + skip_hooks: bool, + on_progress: Arc, + ) -> GitResult> { + if skip_hooks { + args.push("--no-verify".to_string()); + } + + let outcome = Self::run_git_with_hook_progress(repo_path, &args, on_progress)?; + let is_rebase_pull = matches!(integration_kind, PullIntegrationKind::Rebase) + || matches!(integration_kind, PullIntegrationKind::Auto) + && (outcome + .hooks + .iter() + .any(|(name, _, _)| name == "pre-rebase") + || Self::is_rebase_in_progress(repo_path)); + + if !is_rebase_pull + && let Some(failure) = Self::latest_hook_failure( + &outcome, + &["pre-merge-commit", "prepare-commit-msg", "commit-msg"], + &["pre-merge-commit", "commit-msg"], + ) + { + Self::rollback_merge_after_hook_failure(repo_path, &failure)?; + return Ok(GitHookAttemptResult::HookRejected { + hook_name: failure.hook_name, + exit_status: failure.exit_status, + output: failure.output, + output_truncated: failure.output_truncated, + bypass_supported: failure.bypass_supported, + }); + } + + if is_rebase_pull + && !outcome.status.success() + && let Some(failure) = Self::latest_hook_failure( + &outcome, + &[ + "pre-rebase", + "prepare-commit-msg", + "commit-msg", + "applypatch-msg", + "pre-applypatch", + ], + &[], + ) + { + return Ok(GitHookAttemptResult::HookRejected { + hook_name: failure.hook_name, + exit_status: failure.exit_status, + output: failure.output, + output_truncated: failure.output_truncated, + bypass_supported: false, + }); + } + + let hook_warning = + Self::latest_hook_failure(&outcome, &["post-merge", "post-rewrite"], &[]); + if outcome.status.success() || hook_warning.is_some() { + return Ok(GitHookAttemptResult::Completed { + result: OperationResult { + message: success_message.to_string(), + output: outcome.output, + repo_path: Some(Self::path_to_string(repo_path)), + backend_used: "git-cli".to_string(), + interpreted_error: None, + }, + hook_warning, + output_truncated: outcome.output_truncated, + }); + } + + let status = self.get_repo_status(&Self::repo_request(repo_path))?; + if status.merge_in_progress || status.rebase_in_progress { + return Ok(GitHookAttemptResult::Completed { + result: OperationResult { + message: conflict_message.to_string(), + output: outcome.output, + repo_path: Some(Self::path_to_string(repo_path)), + backend_used: "git-cli".to_string(), + interpreted_error: None, + }, + hook_warning: None, + output_truncated: outcome.output_truncated, + }); + } + + Err(GitError::CommandFailed { + command: format!("git {}", args.join(" ")), + stderr: outcome.output.unwrap_or_default(), + exit_code: outcome.status.code(), + }) + } + fn try_rev_parse(repo_path: &Path, rev: &str) -> Option { Self::run_git(&["rev-parse", rev], Some(repo_path)) .ok() @@ -2684,6 +2791,65 @@ impl CliGitHandler { }) } + fn latest_hook_failure( + outcome: &HookCommandOutput, + hook_names: &[&str], + bypassable_hook_names: &[&str], + ) -> Option { + outcome + .hooks + .iter() + .rev() + .find(|(_, name, status)| { + hook_names.contains(&name.as_str()) && status.is_some_and(|code| code != 0) + }) + .map(|(_, name, status)| GitHookFailure { + hook_name: name.clone(), + exit_status: *status, + output: outcome.output.clone(), + output_truncated: outcome.output_truncated, + bypass_supported: bypassable_hook_names.contains(&name.as_str()), + }) + } + + fn rollback_merge_after_hook_failure( + repo_path: &Path, + failure: &GitHookFailure, + ) -> GitResult<()> { + if !Self::is_merge_in_progress(repo_path) { + return Ok(()); + } + + if let Err(rollback_error) = Self::run_git(&["merge", "--abort"], Some(repo_path)) { + return Err(GitError::CommandFailed { + command: "git merge --abort".to_string(), + stderr: format!( + "GITMUN_MERGE_HOOK_ROLLBACK_FAILED: {}\nHook output:\n{}\nRollback failure:\n{}", + failure.hook_name, + failure + .output + .as_deref() + .unwrap_or("No hook output was captured."), + rollback_error + ), + exit_code: None, + }); + } + + if Self::is_merge_in_progress(repo_path) { + return Err(GitError::CommandFailed { + command: "git merge --abort".to_string(), + stderr: format!( + "GITMUN_MERGE_HOOK_ROLLBACK_INCOMPLETE: {}", + failure.hook_name + ), + exit_code: None, + }); + } + + Ok(()) + } + fn completed_hook_operation( repo_path: &Path, args: Vec, @@ -3011,6 +3177,299 @@ impl CliGitHandler { output_truncated: outcome.output_truncated, }) } + + pub fn pull_changes_with_progress( + &self, + request: &RepoRequest, + skip_hooks: bool, + on_progress: Arc, + ) -> GitResult> { + let repo_path = Self::normalise_repo_path(&request.repo_path)?; + self.execute_pull_command_with_progress( + &repo_path, + vec!["pull".to_string()], + &format!("Pulled latest changes in {}", repo_path.display()), + "Pull started a conflict resolution flow. Resolve the conflicts, then continue or complete the operation.", + PullIntegrationKind::Auto, + skip_hooks, + on_progress, + ) + } + + pub fn pull_with_strategy_with_progress( + &self, + request: &PullStrategyRequest, + skip_hooks: bool, + on_progress: Arc, + ) -> GitResult> { + let repo_path = Self::normalise_repo_path(&request.repo_path)?; + let analysis = self.build_pull_analysis(&repo_path)?; + let (args, success_message, conflict_message, integration_kind) = match request.strategy { + PullStrategy::FfOnly => { + if !matches!(analysis.state, PullState::BehindOnly) { + return Err(GitError::InvalidInput( + "Fast-forward pull is only available when the branch is behind its upstream." + .to_string(), + )); + } + ( + vec!["pull".to_string(), "--ff-only".to_string()], + "Fast-forward pull complete.", + "Pull started a conflict resolution flow. Resolve the conflicts, then continue or complete the operation.", + PullIntegrationKind::Merge, + ) + } + PullStrategy::Rebase => { + if !matches!(analysis.state, PullState::BehindOnly | PullState::Divergent) { + return Err(GitError::InvalidInput( + "Rebase pull is only available when remote changes need to be integrated." + .to_string(), + )); + } + ( + vec!["pull".to_string(), "--rebase".to_string()], + "Rebase pull complete.", + "Rebase started and needs conflict resolution. Resolve the conflicts, then continue the rebase.", + PullIntegrationKind::Rebase, + ) + } + PullStrategy::Merge => { + if !matches!(analysis.state, PullState::BehindOnly | PullState::Divergent) { + return Err(GitError::InvalidInput( + "Merge pull is only available when remote changes need to be integrated." + .to_string(), + )); + } + ( + vec!["pull".to_string(), "--no-rebase".to_string()], + "Merge pull complete.", + "Merge started and needs conflict resolution. Resolve the conflicts, then complete or abort the merge.", + PullIntegrationKind::Merge, + ) + } + }; + self.execute_pull_command_with_progress( + &repo_path, + args, + success_message, + conflict_message, + integration_kind, + skip_hooks, + on_progress, + ) + } + + pub fn merge_branch_with_progress( + &self, + request: &MergeRequest, + skip_hooks: bool, + on_progress: Arc, + ) -> GitResult> { + let repo_path = Self::normalise_repo_path(&request.repo_path)?; + if Self::is_merge_in_progress(&repo_path) { + return Err(GitError::InvalidInput( + "Cannot start merge while another merge is in progress".to_string(), + )); + } + if Self::is_rebase_in_progress(&repo_path) { + return Err(GitError::InvalidInput( + "Cannot start merge while a rebase is in progress".to_string(), + )); + } + if Self::is_cherry_pick_in_progress(&repo_path) { + return Err(GitError::InvalidInput( + "Cannot start merge while a cherry-pick is in progress".to_string(), + )); + } + let mut args = vec!["merge".to_string()]; + if skip_hooks { + args.push("--no-verify".to_string()); + } + if request.no_ff == Some(true) { + args.push("--no-ff".to_string()); + } + if request.ff_only == Some(true) { + args.push("--ff-only".to_string()); + } + if let Some(message) = &request.message { + args.push("-m".to_string()); + args.push(message.clone()); + } + args.push(request.branch_name.clone()); + let outcome = Self::run_git_with_hook_progress(&repo_path, &args, on_progress)?; + if let Some(failure) = Self::latest_hook_failure( + &outcome, + &["pre-merge-commit", "prepare-commit-msg", "commit-msg"], + &["pre-merge-commit", "commit-msg"], + ) { + Self::rollback_merge_after_hook_failure(&repo_path, &failure)?; + return Ok(GitHookAttemptResult::HookRejected { + hook_name: failure.hook_name, + exit_status: failure.exit_status, + output: failure.output, + output_truncated: failure.output_truncated, + bypass_supported: failure.bypass_supported, + }); + } + let hook_warning = Self::hook_failure(&outcome, "post-merge", false); + let has_conflicts = Self::is_merge_in_progress(&repo_path); + if !outcome.status.success() && !has_conflicts && hook_warning.is_none() { + return Err(GitError::CommandFailed { + command: format!("git {}", args.join(" ")), + stderr: outcome.output.unwrap_or_default(), + exit_code: outcome.status.code(), + }); + } + let conflicted_files = if has_conflicts { + Self::get_conflicted_files(&repo_path) + } else { + vec![] + }; + let result = MergeResult { + message: if has_conflicts { + format!( + "Merge conflicts in {} file(s) - resolve and commit", + conflicted_files.len() + ) + } else { + format!("Merged '{}' into current branch", request.branch_name) + }, + output: outcome.output.clone(), + repo_path: Some(Self::path_to_string(&repo_path)), + backend_used: "git-cli".to_string(), + interpreted_error: None, + success: !has_conflicts, + has_conflicts, + conflicted_files, + }; + Ok(GitHookAttemptResult::Completed { + result, + hook_warning, + output_truncated: outcome.output_truncated, + }) + } + + fn rebase_hook_attempt_result( + repo_path: &Path, + args: &[String], + outcome: HookCommandOutput, + complete_message: String, + in_progress_message: &str, + ) -> GitResult> { + if !outcome.status.success() + && let Some(failure) = Self::latest_hook_failure( + &outcome, + &[ + "pre-rebase", + "pre-commit", + "prepare-commit-msg", + "commit-msg", + "applypatch-msg", + "pre-applypatch", + ], + &[], + ) + { + return Ok(GitHookAttemptResult::HookRejected { + hook_name: failure.hook_name, + exit_status: failure.exit_status, + output: failure.output, + output_truncated: failure.output_truncated, + bypass_supported: false, + }); + } + let hook_warning = Self::hook_failure(&outcome, "post-rewrite", false); + let rebase_in_progress = Self::is_rebase_in_progress(repo_path); + if !outcome.status.success() && outcome.status.code() != Some(1) && hook_warning.is_none() { + return Err(GitError::CommandFailed { + command: format!("git {}", args.join(" ")), + stderr: outcome.output.unwrap_or_default(), + exit_code: outcome.status.code(), + }); + } + let conflicted_files = if rebase_in_progress { + Self::get_conflicted_files(repo_path) + } else { + vec![] + }; + let has_conflicts = !conflicted_files.is_empty(); + let result = RebaseResult { + message: if has_conflicts { + format!( + "Rebase conflicts in {} file(s) - resolve and continue", + conflicted_files.len() + ) + } else if rebase_in_progress { + in_progress_message.to_string() + } else { + complete_message + }, + output: outcome.output, + repo_path: Some(Self::path_to_string(repo_path)), + backend_used: "git-cli".to_string(), + interpreted_error: None, + success: !has_conflicts, + has_conflicts, + conflicted_files, + rebase_in_progress, + }; + Ok(GitHookAttemptResult::Completed { + result, + hook_warning, + output_truncated: outcome.output_truncated, + }) + } + + pub fn rebase_start_with_progress( + &self, + request: &RebaseRequest, + on_progress: Arc, + ) -> GitResult> { + let repo_path = Self::normalise_repo_path(&request.repo_path)?; + Self::ensure_no_active_branch_operation(&repo_path, "start a rebase")?; + let onto = request.onto.trim(); + if onto.is_empty() { + return Err(GitError::InvalidInput( + "Rebase target cannot be empty".to_string(), + )); + } + let args = vec!["rebase".to_string(), onto.to_string()]; + let outcome = Self::run_git_with_hook_progress(&repo_path, &args, on_progress)?; + Self::rebase_hook_attempt_result( + &repo_path, + &args, + outcome, + format!("Rebased current branch onto '{onto}'"), + "Rebase continued", + ) + } + + pub fn rebase_continue_with_progress( + &self, + request: &RepoRequest, + on_progress: Arc, + ) -> GitResult> { + let repo_path = Self::normalise_repo_path(&request.repo_path)?; + if !Self::is_rebase_in_progress(&repo_path) { + return Err(GitError::InvalidInput( + "No rebase in progress to continue".to_string(), + )); + } + let args = vec![ + "-c".to_string(), + "core.editor=true".to_string(), + "rebase".to_string(), + "--continue".to_string(), + ]; + let outcome = Self::run_git_with_hook_progress(&repo_path, &args, on_progress)?; + Self::rebase_hook_attempt_result( + &repo_path, + &args, + outcome, + "Rebase complete".to_string(), + "Rebase continued", + ) + } } impl GitOperationHandler for CliGitHandler { diff --git a/src-tauri/src/git/handler.rs b/src-tauri/src/git/handler.rs index 71c8d89..ee1747b 100644 --- a/src-tauri/src/git/handler.rs +++ b/src-tauri/src/git/handler.rs @@ -670,6 +670,54 @@ impl GitService { .push_changes_with_progress(&request, skip_hooks, on_progress) } + pub fn pull_changes_with_progress( + &self, + request: RepoRequest, + skip_hooks: bool, + on_progress: Arc, + ) -> GitResult> { + self.cli_handler + .pull_changes_with_progress(&request, skip_hooks, on_progress) + } + + pub fn pull_with_strategy_with_progress( + &self, + request: PullStrategyRequest, + skip_hooks: bool, + on_progress: Arc, + ) -> GitResult> { + self.cli_handler + .pull_with_strategy_with_progress(&request, skip_hooks, on_progress) + } + + pub fn merge_branch_with_progress( + &self, + request: MergeRequest, + skip_hooks: bool, + on_progress: Arc, + ) -> GitResult> { + self.cli_handler + .merge_branch_with_progress(&request, skip_hooks, on_progress) + } + + pub fn rebase_start_with_progress( + &self, + request: RebaseRequest, + on_progress: Arc, + ) -> GitResult> { + self.cli_handler + .rebase_start_with_progress(&request, on_progress) + } + + pub fn rebase_continue_with_progress( + &self, + request: RepoRequest, + on_progress: Arc, + ) -> GitResult> { + self.cli_handler + .rebase_continue_with_progress(&request, on_progress) + } + pub fn switch_branch_with_progress( &self, request: BranchRequest, diff --git a/src-tauri/tests/git.rs b/src-tauri/tests/git.rs index 05ea52c..e5bb924 100644 --- a/src-tauri/tests/git.rs +++ b/src-tauri/tests/git.rs @@ -13,9 +13,10 @@ use gitmun_lib::git::types::{ CommitProgressEvent, CommitRefKind, CommitRequest, CreateBranchRequest, DeleteBranchRequest, ExportCommitPatchRequest, ExportPatchFileSelection, ExportPatchRequest, ExportPatchScope, FileRequest, GitHookAttemptResult, IdentityRequest, IdentityScope, ImportPatchRequest, - PushFailureKind, PushRequest, RepoRequest, RepoStatus, ResetMode, ResetRequest, - SetBranchUpstreamRequest, SetIdentityRequest, SshAllowedSignerReason, StageFilesRequest, - SubmoduleActionRequest, SubmoduleState, UnversionedItemKind, + MergeRequest, PullStrategy, PullStrategyRequest, PushFailureKind, PushRequest, RebaseRequest, + RepoRequest, RepoStatus, ResetMode, ResetRequest, SetBranchUpstreamRequest, SetIdentityRequest, + SshAllowedSignerReason, StageFilesRequest, SubmoduleActionRequest, SubmoduleState, + UnversionedItemKind, }; fn init_repo() -> TempDir { @@ -74,6 +75,19 @@ fn write_file(repo: &Path, name: &str, content: &str) { fs::write(repo.join(name), content).expect("write file"); } +#[cfg(unix)] +fn install_hook(repo: &Path, hook_name: &str, script: &str) { + use std::os::unix::fs::PermissionsExt; + + let hook_path = repo.join(".git/hooks").join(hook_name); + fs::write(&hook_path, script).expect("write hook"); + let mut permissions = fs::metadata(&hook_path) + .expect("hook metadata") + .permissions(); + permissions.set_mode(0o755); + fs::set_permissions(hook_path, permissions).expect("make hook executable"); +} + fn make_index_entry_stale(repo: &Path, file_path: &str) -> (std::path::PathBuf, Vec) { let raw_index_path = git_stdout(repo, &["rev-parse", "--git-path", "index"]); let index_path = { @@ -165,6 +179,22 @@ fn init_remote_with_clone() -> (TempDir, TempDir) { (remote, local) } +fn clone_test_remote(remote: &TempDir) -> TempDir { + let clone = TempDir::new().expect("create clone dir"); + git( + Path::new("."), + &[ + "clone", + remote.path().to_str().unwrap(), + clone.path().to_str().unwrap(), + ], + ); + git(clone.path(), &["config", "user.email", "peer@gitmun.test"]); + git(clone.path(), &["config", "user.name", "Gitmun Test Peer"]); + git(clone.path(), &["config", "commit.gpgsign", "false"]); + clone +} + fn init_submodule_source() -> TempDir { let dir = init_repo(); write_file(dir.path(), "lib.txt", "v1"); @@ -1792,6 +1822,433 @@ fn post_checkout_failure_reports_warning_without_repeating_checkout() { ); } +#[cfg(unix)] +#[test] +fn merge_hook_rejections_roll_back_and_only_supported_hooks_can_be_bypassed() { + for (installed_hook_name, bypass_supported) in [ + ("pre-merge-commit", true), + ("prepare-commit-msg", false), + ("commit-msg", true), + ] { + let dir = init_repo(); + git(dir.path(), &["switch", "-c", "feature/hook-check"]); + write_file(dir.path(), "feature.txt", installed_hook_name); + git(dir.path(), &["add", "feature.txt"]); + git(dir.path(), &["commit", "-m", "feature change"]); + git(dir.path(), &["switch", "main"]); + install_hook( + dir.path(), + installed_hook_name, + "#!/bin/sh\necho merge hook rejected >&2\nexit 1\n", + ); + let head_before = head_hash(dir.path()); + let request = MergeRequest { + repo_path: dir.path().to_string_lossy().into_owned(), + branch_name: "feature/hook-check".to_string(), + no_ff: Some(true), + ff_only: None, + message: None, + }; + + let rejected = handler() + .merge_branch_with_progress(&request, false, Arc::new(|_| {})) + .expect("merge hook rejection"); + assert!(matches!( + rejected, + GitHookAttemptResult::HookRejected { + ref hook_name, + bypass_supported: actual_bypass, + .. + } if hook_name == installed_hook_name && actual_bypass == bypass_supported + )); + assert_eq!(head_hash(dir.path()), head_before); + assert!(!dir.path().join(".git/MERGE_HEAD").exists()); + assert!(git_stdout(dir.path(), &["status", "--porcelain"]).is_empty()); + + if bypass_supported { + let bypassed = handler() + .merge_branch_with_progress(&request, true, Arc::new(|_| {})) + .expect("merge hook bypass"); + assert!(matches!( + bypassed, + GitHookAttemptResult::Completed { ref result, .. } if result.success + )); + assert_ne!(head_hash(dir.path()), head_before); + } + } +} + +#[cfg(unix)] +#[test] +fn clean_merge_emits_merge_message_and_post_merge_hook_progress() { + let dir = init_repo(); + git(dir.path(), &["switch", "-c", "feature/hook-progress"]); + write_file(dir.path(), "feature.txt", "feature change"); + git(dir.path(), &["add", "feature.txt"]); + git(dir.path(), &["commit", "-m", "feature change"]); + git(dir.path(), &["switch", "main"]); + for hook_name in [ + "pre-merge-commit", + "prepare-commit-msg", + "commit-msg", + "post-merge", + ] { + install_hook(dir.path(), hook_name, "#!/bin/sh\nexit 0\n"); + } + let events = Arc::new(Mutex::new(Vec::new())); + let recorded_events = Arc::clone(&events); + + let result = handler() + .merge_branch_with_progress( + &MergeRequest { + repo_path: dir.path().to_string_lossy().into_owned(), + branch_name: "feature/hook-progress".to_string(), + no_ff: Some(true), + ff_only: None, + message: None, + }, + false, + Arc::new(move |event| { + recorded_events.lock().expect("event lock").push(event); + }), + ) + .expect("clean merge"); + + assert!(matches!( + result, + GitHookAttemptResult::Completed { ref result, hook_warning: None, .. } if result.success + )); + let events = events.lock().expect("event lock"); + for expected_hook in [ + "pre-merge-commit", + "prepare-commit-msg", + "commit-msg", + "post-merge", + ] { + assert!(events.iter().any(|event| matches!( + event, + CommitProgressEvent::HookStarted { hook_name } if hook_name == expected_hook + ))); + } +} + +#[cfg(unix)] +#[test] +fn fast_forward_pull_reports_post_merge_failure_after_updating_head() { + let (remote, local) = init_remote_with_clone(); + let peer = clone_test_remote(&remote); + write_file(peer.path(), "remote.txt", "remote change"); + git(peer.path(), &["add", "remote.txt"]); + git(peer.path(), &["commit", "-m", "remote change"]); + git(peer.path(), &["push", "origin", "main"]); + git(local.path(), &["fetch", "origin"]); + install_hook( + local.path(), + "post-merge", + "#!/bin/sh\necho post-merge warning >&2\nexit 1\n", + ); + let events = Arc::new(Mutex::new(Vec::new())); + let recorded_events = Arc::clone(&events); + + let result = handler() + .pull_with_strategy_with_progress( + &PullStrategyRequest { + repo_path: local.path().to_string_lossy().into_owned(), + strategy: PullStrategy::FfOnly, + }, + false, + Arc::new(move |event| { + recorded_events.lock().expect("event lock").push(event); + }), + ) + .expect("fast-forward pull result"); + + assert!(matches!( + result, + GitHookAttemptResult::Completed { hook_warning: Some(ref warning), .. } + if warning.hook_name == "post-merge" + )); + assert_eq!(head_hash(local.path()), head_hash(peer.path())); + assert!( + events + .lock() + .expect("event lock") + .iter() + .any(|event| matches!( + event, + CommitProgressEvent::HookStarted { hook_name } if hook_name == "post-merge" + )) + ); +} + +#[cfg(unix)] +#[test] +fn merge_pull_rejection_rolls_back_before_bypass_retry() { + let (remote, local) = init_remote_with_clone(); + let peer = clone_test_remote(&remote); + write_file(local.path(), "local.txt", "local change"); + git(local.path(), &["add", "local.txt"]); + git(local.path(), &["commit", "-m", "local change"]); + write_file(peer.path(), "remote.txt", "remote change"); + git(peer.path(), &["add", "remote.txt"]); + git(peer.path(), &["commit", "-m", "remote change"]); + git(peer.path(), &["push", "origin", "main"]); + git(local.path(), &["fetch", "origin"]); + install_hook( + local.path(), + "pre-merge-commit", + "#!/bin/sh\necho pull merge rejected >&2\nexit 1\n", + ); + let head_before = head_hash(local.path()); + let request = PullStrategyRequest { + repo_path: local.path().to_string_lossy().into_owned(), + strategy: PullStrategy::Merge, + }; + + let rejected = handler() + .pull_with_strategy_with_progress(&request, false, Arc::new(|_| {})) + .expect("pull merge rejection"); + assert!(matches!( + rejected, + GitHookAttemptResult::HookRejected { + bypass_supported: true, + .. + } + )); + assert_eq!(head_hash(local.path()), head_before); + assert!(!local.path().join(".git/MERGE_HEAD").exists()); + + let bypassed = handler() + .pull_with_strategy_with_progress(&request, true, Arc::new(|_| {})) + .expect("pull merge bypass"); + assert!(matches!(bypassed, GitHookAttemptResult::Completed { .. })); + assert_ne!(head_hash(local.path()), head_before); +} + +#[cfg(unix)] +#[test] +fn merge_hook_rollback_failure_reports_both_failures() { + let dir = init_repo(); + git(dir.path(), &["switch", "-c", "feature/rollback-failure"]); + write_file(dir.path(), "feature.txt", "feature change"); + git(dir.path(), &["add", "feature.txt"]); + git(dir.path(), &["commit", "-m", "feature change"]); + git(dir.path(), &["switch", "main"]); + install_hook( + dir.path(), + "pre-merge-commit", + "#!/bin/sh\ntouch .git/index.lock\necho merge hook rejected >&2\nexit 1\n", + ); + + let error = handler() + .merge_branch_with_progress( + &MergeRequest { + repo_path: dir.path().to_string_lossy().into_owned(), + branch_name: "feature/rollback-failure".to_string(), + no_ff: Some(true), + ff_only: None, + message: None, + }, + false, + Arc::new(|_| {}), + ) + .expect_err("rollback should fail while the index is locked"); + + let error_message = error.to_string(); + assert!(error_message.contains("GITMUN_MERGE_HOOK_ROLLBACK_FAILED")); + assert!(error_message.contains("pre-merge-commit")); + assert!(dir.path().join(".git/MERGE_HEAD").exists()); + fs::remove_file(dir.path().join(".git/index.lock")).expect("remove index lock"); +} + +#[cfg(unix)] +#[test] +fn pre_rebase_rejects_pull_without_changing_head_or_offering_bypass() { + let (remote, local) = init_remote_with_clone(); + let peer = clone_test_remote(&remote); + write_file(local.path(), "local.txt", "local change"); + git(local.path(), &["add", "local.txt"]); + git(local.path(), &["commit", "-m", "local change"]); + write_file(peer.path(), "remote.txt", "remote change"); + git(peer.path(), &["add", "remote.txt"]); + git(peer.path(), &["commit", "-m", "remote change"]); + git(peer.path(), &["push", "origin", "main"]); + git(local.path(), &["fetch", "origin"]); + install_hook( + local.path(), + "pre-rebase", + "#!/bin/sh\necho rebase rejected >&2\nexit 1\n", + ); + let head_before = head_hash(local.path()); + + let rejected = handler() + .pull_with_strategy_with_progress( + &PullStrategyRequest { + repo_path: local.path().to_string_lossy().into_owned(), + strategy: PullStrategy::Rebase, + }, + false, + Arc::new(|_| {}), + ) + .expect("pre-rebase rejection"); + + assert!(matches!( + rejected, + GitHookAttemptResult::HookRejected { + ref hook_name, + bypass_supported: false, + .. + } if hook_name == "pre-rebase" + )); + assert_eq!(head_hash(local.path()), head_before); + assert!(!local.path().join(".git/rebase-merge").exists()); + assert!(!local.path().join(".git/rebase-apply").exists()); +} + +#[cfg(unix)] +#[test] +fn rebase_reports_post_rewrite_failure_after_rewriting_history() { + let dir = init_repo(); + git(dir.path(), &["switch", "-c", "feature/rebase-hook"]); + write_file(dir.path(), "feature.txt", "feature change"); + git(dir.path(), &["add", "feature.txt"]); + git(dir.path(), &["commit", "-m", "feature change"]); + let head_before = head_hash(dir.path()); + git(dir.path(), &["switch", "main"]); + write_file(dir.path(), "main.txt", "main change"); + git(dir.path(), &["add", "main.txt"]); + git(dir.path(), &["commit", "-m", "main change"]); + git(dir.path(), &["switch", "feature/rebase-hook"]); + install_hook( + dir.path(), + "post-rewrite", + "#!/bin/sh\necho post-rewrite warning >&2\nexit 1\n", + ); + + let result = handler() + .rebase_start_with_progress( + &RebaseRequest { + repo_path: dir.path().to_string_lossy().into_owned(), + onto: "main".to_string(), + }, + Arc::new(|_| {}), + ) + .expect("rebase result"); + + assert!(matches!( + result, + GitHookAttemptResult::Completed { hook_warning: Some(ref warning), ref result, .. } + if warning.hook_name == "post-rewrite" && result.success + )); + assert_ne!(head_hash(dir.path()), head_before); +} + +#[cfg(unix)] +#[test] +fn successful_rebase_emits_pre_rebase_and_post_rewrite_progress() { + let dir = init_repo(); + git(dir.path(), &["switch", "-c", "feature/rebase-progress"]); + write_file(dir.path(), "feature.txt", "feature change"); + git(dir.path(), &["add", "feature.txt"]); + git(dir.path(), &["commit", "-m", "feature change"]); + git(dir.path(), &["switch", "main"]); + write_file(dir.path(), "main.txt", "main change"); + git(dir.path(), &["add", "main.txt"]); + git(dir.path(), &["commit", "-m", "main change"]); + git(dir.path(), &["switch", "feature/rebase-progress"]); + install_hook(dir.path(), "pre-rebase", "#!/bin/sh\nexit 0\n"); + install_hook(dir.path(), "post-rewrite", "#!/bin/sh\nexit 0\n"); + let events = Arc::new(Mutex::new(Vec::new())); + let recorded_events = Arc::clone(&events); + + let result = handler() + .rebase_start_with_progress( + &RebaseRequest { + repo_path: dir.path().to_string_lossy().into_owned(), + onto: "main".to_string(), + }, + Arc::new(move |event| { + recorded_events.lock().expect("event lock").push(event); + }), + ) + .expect("successful rebase"); + + assert!(matches!( + result, + GitHookAttemptResult::Completed { ref result, hook_warning: None, .. } if result.success + )); + let events = events.lock().expect("event lock"); + for expected_hook in ["pre-rebase", "post-rewrite"] { + assert!(events.iter().any(|event| matches!( + event, + CommitProgressEvent::HookStarted { hook_name } if hook_name == expected_hook + ))); + } +} + +#[cfg(unix)] +#[test] +fn rebase_continue_preserves_conflict_state_and_reports_final_post_rewrite() { + let dir = init_repo(); + write_file(dir.path(), "shared.txt", "base"); + git(dir.path(), &["add", "shared.txt"]); + git(dir.path(), &["commit", "-m", "shared base"]); + git(dir.path(), &["switch", "-c", "feature/rebase-conflict"]); + write_file(dir.path(), "shared.txt", "feature"); + git(dir.path(), &["add", "shared.txt"]); + git(dir.path(), &["commit", "-m", "feature change"]); + git(dir.path(), &["switch", "main"]); + write_file(dir.path(), "shared.txt", "main"); + git(dir.path(), &["add", "shared.txt"]); + git(dir.path(), &["commit", "-m", "main change"]); + git(dir.path(), &["switch", "feature/rebase-conflict"]); + install_hook(dir.path(), "post-rewrite", "#!/bin/sh\nexit 0\n"); + + let started = handler() + .rebase_start_with_progress( + &RebaseRequest { + repo_path: dir.path().to_string_lossy().into_owned(), + onto: "main".to_string(), + }, + Arc::new(|_| {}), + ) + .expect("start conflicting rebase"); + assert!(matches!( + started, + GitHookAttemptResult::Completed { ref result, .. } + if result.has_conflicts && result.rebase_in_progress + )); + + write_file(dir.path(), "shared.txt", "resolved"); + git(dir.path(), &["add", "shared.txt"]); + let events = Arc::new(Mutex::new(Vec::new())); + let recorded_events = Arc::clone(&events); + let continued = handler() + .rebase_continue_with_progress( + &repo_request(&dir), + Arc::new(move |event| { + recorded_events.lock().expect("event lock").push(event); + }), + ) + .expect("continue rebase"); + + assert!(matches!( + continued, + GitHookAttemptResult::Completed { ref result, hook_warning: None, .. } + if result.success && !result.has_conflicts && !result.rebase_in_progress + )); + assert!( + events + .lock() + .expect("event lock") + .iter() + .any(|event| matches!( + event, + CommitProgressEvent::HookStarted { hook_name } if hook_name == "post-rewrite" + )) + ); +} + #[test] fn commit_message_recovery_reads_commit_editmsg() { let dir = init_repo(); diff --git a/src/api/commands.ts b/src/api/commands.ts index 0acbde7..c985a90 100644 --- a/src/api/commands.ts +++ b/src/api/commands.ts @@ -310,16 +310,25 @@ export function fetchRemote(repoPath: string, remote?: string): Promise("fetch_remote", {request: {repoPath, remote}}); } -export function pullChanges(repoPath: string): Promise { - return invoke("pull_changes", {request: {repoPath}}); +export function pullChanges( + repoPath: string, + onProgress: Channel, + skipHooks = false, +): Promise> { + return invoke>("pull_changes", {request: {repoPath}, onProgress, skipHooks}); } export function analyzePull(repoPath: string): Promise { return invoke("analyze_pull", {request: {repoPath}}); } -export function pullWithStrategy(repoPath: string, strategy: PullStrategy): Promise { - return invoke("pull_with_strategy", {request: {repoPath, strategy}}); +export function pullWithStrategy( + repoPath: string, + strategy: PullStrategy, + onProgress: Channel, + skipHooks = false, +): Promise> { + return invoke>("pull_with_strategy", {request: {repoPath, strategy}, onProgress, skipHooks}); } export function pushChanges(request: PushRequest, onProgress: Channel, skipHooks = false): Promise> { @@ -358,10 +367,14 @@ export function stashDrop(repoPath: string, stashIndex: number): Promise { - return invoke("merge_branch", { + options: { noFf?: boolean; ffOnly?: boolean; message?: string } | undefined, + onProgress: Channel, + skipHooks = false, +): Promise> { + return invoke>("merge_branch", { request: {repoPath, branchName, ...options}, + onProgress, + skipHooks, }); } @@ -369,12 +382,18 @@ export function mergeAbort(repoPath: string): Promise { return invoke("merge_abort", {request: {repoPath}}); } -export function rebaseStart(request: RebaseRequest): Promise { - return invoke("rebase_start", {request}); +export function rebaseStart( + request: RebaseRequest, + onProgress: Channel, +): Promise> { + return invoke>("rebase_start", {request, onProgress}); } -export function rebaseContinue(repoPath: string): Promise { - return invoke("rebase_continue", {request: {repoPath}}); +export function rebaseContinue( + repoPath: string, + onProgress: Channel, +): Promise> { + return invoke>("rebase_continue", {request: {repoPath}, onProgress}); } export function rebaseAbort(repoPath: string): Promise { diff --git a/src/components/ProjectView.tsx b/src/components/ProjectView.tsx index 13c007d..ea69f3b 100644 --- a/src/components/ProjectView.tsx +++ b/src/components/ProjectView.tsx @@ -60,6 +60,7 @@ import type { GitIdentity, GitHookAttemptResult, GitHookFailure, + GitHookOperation, GitHookProgressEvent, GitHookProgressState, ImportPatchRequest, @@ -143,6 +144,8 @@ const PATCH_EXPORT_ERROR_CODES = [ ] as const; const PATCH_IMPORT_APPLIED = "GITMUN_PATCH_IMPORT_APPLIED"; const PATCH_IMPORT_CONFLICTS = "GITMUN_PATCH_IMPORT_CONFLICTS"; +const MERGE_HOOK_ROLLBACK_FAILED = "GITMUN_MERGE_HOOK_ROLLBACK_FAILED"; +const MERGE_HOOK_ROLLBACK_INCOMPLETE = "GITMUN_MERGE_HOOK_ROLLBACK_INCOMPLETE"; const PATCH_IMPORT_MESSAGE_CODES = [ PATCH_IMPORT_APPLIED, PATCH_IMPORT_CONFLICTS, @@ -170,6 +173,13 @@ function localisePatchImportMessage(message: string, t: TFunction<"projectView"> return code ? t(`patch.import.${code}`) : message; } +function localiseMergeHookRollbackError(error: unknown, t: TFunction<"projectView">): string { + const message = String(error); + if (message.includes(MERGE_HOOK_ROLLBACK_FAILED)) return t("toast.mergeHookRollbackFailed"); + if (message.includes(MERGE_HOOK_ROLLBACK_INCOMPLETE)) return t("toast.mergeHookRollbackIncomplete"); + return message; +} + export function buildStashDropPrompt( stash: Pick, t: TFunction<"projectView">, @@ -428,7 +438,7 @@ export function ProjectView({ const operationLockRef = useRef(null); const nextOperationIdRef = useRef(1); const [hookProgress, setHookProgress] = useState(null); - const [hookRejection, setHookRejection] = useState<(GitHookFailure & {operation: "commit" | "push"}) | null>(null); + const [hookRejection, setHookRejection] = useState<(GitHookFailure & {operation: GitHookOperation}) | null>(null); const hookDecisionRef = useRef<((skipHooks: boolean) => void) | null>(null); useEffect(() => () => { @@ -731,7 +741,7 @@ export function ProjectView({ await Promise.all([refreshStatus(), refreshBranches(), refreshTags(), refreshRemotes(), refreshLog(), refreshStashes()]); }, [refreshStatus, refreshBranches, refreshTags, refreshRemotes, refreshLog, refreshStashes]); - const createHookProgressChannel = useCallback((operation: "commit" | "push" | "checkout") => { + const createHookProgressChannel = useCallback((operation: GitHookOperation) => { const progress = new Channel(); setHookProgress({operation, startedAt: Date.now(), phase: "running", hookName: null, output: "", outputTruncated: false, expanded: false}); progress.onmessage = event => { @@ -747,33 +757,54 @@ export function ProjectView({ return progress; }, []); - const runPushHookOperation = useCallback(async ( + const runBlockingHookOperation = useCallback(async ,>( + hookOperation: Exclude, operation: (progress: Channel, skipHooks: boolean) => Promise>, ): Promise => { let skipHooks = false; for (;;) { let attempt: GitHookAttemptResult; try { - attempt = await operation(createHookProgressChannel("push"), skipHooks); + attempt = await operation(createHookProgressChannel(hookOperation), skipHooks); } catch (error) { setHookProgress(null); throw error; } if (attempt.status === "completed") { - setHookProgress(null); - if (skipHooks) appendResultLog("info", t("log.pushHooksSkipped"), "git-cli"); + await refreshAll().catch(() => undefined); + if (attempt.hookWarning) { + setHookProgress(current => ({ + operation: hookOperation, + startedAt: current?.startedAt ?? Date.now(), + phase: "warning", + hookName: attempt.hookWarning?.hookName ?? null, + output: attempt.hookWarning?.output ?? current?.output ?? "", + outputTruncated: attempt.hookWarning?.outputTruncated ?? false, + expanded: true, + })); + showToast(t("toast.hookWarning", {operation: t(`hookOperationNames.${hookOperation}`)}), "info"); + appendResultLog("error", t("log.hookWarning", {operation: t(`hookOperationNames.${hookOperation}`), hook: attempt.hookWarning.hookName}), attempt.result.backendUsed, undefined, attempt.hookWarning.output ?? undefined); + } else { + setHookProgress(null); + } + if (skipHooks) appendResultLog("info", t("log.hooksSkipped", {operation: t(`hookOperationNames.${hookOperation}`)}), "git-cli"); return attempt.result; } setHookProgress(current => current ? {...current, phase: "awaitingDecision", hookName: attempt.hookName, output: attempt.output ?? current.output, outputTruncated: attempt.outputTruncated, expanded: true} : current); - setHookRejection({...attempt, operation: "push"}); + await refreshAll().catch(() => undefined); + setHookRejection({...attempt, operation: hookOperation}); skipHooks = await new Promise(resolve => { hookDecisionRef.current = resolve; }); if (!skipHooks) { - appendResultLog("error", t("log.pushHookRejected", {hook: attempt.hookName}), "git-cli", undefined, attempt.output ?? undefined); + appendResultLog("error", t("log.hookRejected", {operation: t(`hookOperationNames.${hookOperation}`), hook: attempt.hookName}), "git-cli", undefined, attempt.output ?? undefined); setHookProgress(null); return null; } } - }, [createHookProgressChannel, t]); + }, [createHookProgressChannel, refreshAll, showToast, t]); + + const runPushHookOperation = useCallback(>( + operation: (progress: Channel, skipHooks: boolean) => Promise>, + ) => runBlockingHookOperation("push", operation), [runBlockingHookOperation]); const runCheckoutHookOperation = useCallback(async ( operation: (progress: Channel) => Promise>, @@ -840,7 +871,8 @@ export function ProjectView({ showToast, onForcePushComplete: handleForcePushComplete, onFetchAttemptComplete, - pushChanges: request => runPushHookOperation((progress, skipHooks) => api.pushChanges(request, progress, skipHooks)), + pushChanges: request => runBlockingHookOperation("push", (progress, skipHooks) => api.pushChanges(request, progress, skipHooks)), + pullWithStrategy: strategy => runBlockingHookOperation("pull", (progress, skipHooks) => api.pullWithStrategy(repoPath!, strategy, progress, skipHooks)), }); useEffect(() => { @@ -1907,7 +1939,8 @@ export function ProjectView({ setIsRebaseActionRunning(true); try { - const result = await api.rebaseStart({ repoPath, onto: ontoBranch }); + const result = await runBlockingHookOperation("rebase", progress => api.rebaseStart({ repoPath, onto: ontoBranch }, progress)); + if (!result) return; if (result.hasConflicts) { showToast(t("toast.rebaseConflicts", { count: result.conflictedFiles.length }), "error"); appendResultLog("error", result.message, result.backendUsed); @@ -1919,18 +1952,20 @@ export function ProjectView({ } await refreshAll(); } catch (e) { + await refreshAll().catch(() => undefined); showToast(String(e), "error"); appendResultLog("error", t("log.rebaseFailed", { message: String(e) }), "unknown"); } finally { setIsRebaseActionRunning(false); } - }, [repoPath, cherryPickInProgress, mergeInProgress, rebaseInProgress, currentBranch, hasWorkingTreeChanges, refreshAll, showToast, t]); + }, [repoPath, cherryPickInProgress, mergeInProgress, rebaseInProgress, currentBranch, hasWorkingTreeChanges, refreshAll, runBlockingHookOperation, showToast, t]); const handleRebaseContinue = useCallback(async () => { if (!repoPath || !rebaseInProgress) return; setIsRebaseActionRunning(true); try { - const result = await api.rebaseContinue(repoPath); + const result = await runBlockingHookOperation("rebase", progress => api.rebaseContinue(repoPath, progress)); + if (!result) return; if (result.hasConflicts) { showToast(t("toast.rebaseConflicts", { count: result.conflictedFiles.length }), "error"); appendResultLog("error", result.message, result.backendUsed); @@ -1944,12 +1979,13 @@ export function ProjectView({ } await refreshAll(); } catch (e) { + await refreshAll().catch(() => undefined); showToast(String(e), "error"); appendResultLog("error", t("log.rebaseContinueFailed", { message: String(e) }), "unknown"); } finally { setIsRebaseActionRunning(false); } - }, [repoPath, rebaseInProgress, currentBranch, refreshAll, showToast, t]); + }, [repoPath, rebaseInProgress, currentBranch, refreshAll, runBlockingHookOperation, showToast, t]); const handleRebaseAbort = useCallback(async () => { if (!repoPath || !rebaseInProgress) return; @@ -2163,7 +2199,8 @@ export function ProjectView({ noFf: strategy === "no-ff", ffOnly: strategy === "ff-only", }; - const result = await api.mergeBranch(repoPath, mergePendingBranch, options); + const result = await runBlockingHookOperation("merge", (progress, skipHooks) => api.mergeBranch(repoPath, mergePendingBranch, options, progress, skipHooks)); + if (!result) return; if (result.hasConflicts) { showToast(t("toast.mergeConflicts", { count: result.conflictedFiles.length }), "error"); appendResultLog("error", result.message, result.backendUsed); @@ -2174,10 +2211,11 @@ export function ProjectView({ await refreshAll(); setCentreTab("changes"); } catch (e) { - showToast(String(e), "error"); + await refreshAll().catch(() => undefined); + showToast(localiseMergeHookRollbackError(e, t), "error"); appendResultLog("error", t("log.mergeFailed", { message: String(e) }), "unknown"); } - }, [repoPath, mergePendingBranch, refreshAll, showToast, t]); + }, [repoPath, mergePendingBranch, refreshAll, runBlockingHookOperation, showToast, t]); const handleMergeAbort = useCallback(async () => { if (!repoPath) return; diff --git a/src/components/centre/CentrePanel.test.tsx b/src/components/centre/CentrePanel.test.tsx index 1c9b17c..922b439 100644 --- a/src/components/centre/CentrePanel.test.tsx +++ b/src/components/centre/CentrePanel.test.tsx @@ -433,6 +433,52 @@ describe("CentrePanel hook feedback", () => { expect(onBypass).toHaveBeenCalledOnce(); }); + it("offers the pull bypass action when the backend permits it", () => { + renderCentrePanel({ + hookRejection: { + operation: "pull", + hookName: "pre-merge-commit", + exitStatus: 1, + output: null, + outputTruncated: false, + bypassSupported: true, + }, + }); + + expect(screen.getByRole("button", {name: "Pull without hooks"})).toBeInTheDocument(); + }); + + it("offers the merge bypass action when the backend permits it", () => { + renderCentrePanel({ + hookRejection: { + operation: "merge", + hookName: "commit-msg", + exitStatus: 1, + output: null, + outputTruncated: false, + bypassSupported: true, + }, + }); + + expect(screen.getByRole("button", {name: "Merge without hooks"})).toBeInTheDocument(); + }); + + it("does not offer a bypass for a rejected rebase hook", () => { + renderCentrePanel({ + hookRejection: { + operation: "rebase", + hookName: "pre-rebase", + exitStatus: 1, + output: null, + outputTruncated: false, + bypassSupported: false, + }, + }); + + expect(screen.getByRole("button", {name: "Close"})).toBeInTheDocument(); + expect(screen.queryByRole("button", {name: /without hooks/i})).not.toBeInTheDocument(); + }); + it("reports checkout completion with a dismissible warning", () => { const onDismiss = vi.fn(); renderCentrePanel({ diff --git a/src/components/centre/CentrePanel.tsx b/src/components/centre/CentrePanel.tsx index 01d329d..f44b288 100644 --- a/src/components/centre/CentrePanel.tsx +++ b/src/components/centre/CentrePanel.tsx @@ -129,7 +129,7 @@ type CentrePanelProps = { stagingOperation: StagingOperation | null; operationLock: LongRunningOperation | null; hookProgress?: GitHookProgressState | null; - hookRejection?: (GitHookFailure & {operation: "commit" | "push"}) | null; + hookRejection?: (GitHookFailure & {operation: GitHookProgressState["operation"]}) | null; onHookRejectionClose?: () => void; onHookRejectionBypass?: () => void; isCommitting: boolean; @@ -159,7 +159,7 @@ function HookProgressBanner({progress, onDismiss}: {progress: GitHookProgressSta return () => window.clearInterval(timer); }, [progress.startedAt]); const title = progress.phase === "warning" - ? t("gitHooks.checkoutWarningTitle") + ? t("gitHooks.warningTitle", {operation: t(`gitHooks.operations.${progress.operation}`)}) : progress.phase === "awaitingDecision" ? t("gitHooks.failedTitle", {operation: t(`gitHooks.operations.${progress.operation}`)}) : progress.hookName @@ -170,7 +170,7 @@ function HookProgressBanner({progress, onDismiss}: {progress: GitHookProgressSta {progress.phase === "running" ? ; } -function HookFailureDialog({failure, onClose, onBypass}: {failure: GitHookFailure & {operation: "commit" | "push"}; onClose: () => void; onBypass: () => void}) { +function HookFailureDialog({failure, onClose, onBypass}: {failure: GitHookFailure & {operation: GitHookProgressState["operation"]}; onClose: () => void; onBypass: () => void}) { const {t} = useTranslation("centre"); const closeButtonRef = React.useRef(null); React.useEffect(() => { closeButtonRef.current?.focus(); }, []); diff --git a/src/hooks/useRemoteOperations.test.tsx b/src/hooks/useRemoteOperations.test.tsx index 875b1d4..0f51c40 100644 --- a/src/hooks/useRemoteOperations.test.tsx +++ b/src/hooks/useRemoteOperations.test.tsx @@ -45,6 +45,7 @@ describe("useRemoteOperations", () => { onForcePushComplete: vi.fn(), onFetchAttemptComplete, pushChanges: mocks.pushChanges, + pullWithStrategy: mocks.pullWithStrategy, })); await act(async () => { @@ -75,6 +76,7 @@ describe("useRemoteOperations", () => { onForcePushComplete: vi.fn(), onFetchAttemptComplete, pushChanges: mocks.pushChanges, + pullWithStrategy: mocks.pullWithStrategy, })); await act(async () => { @@ -101,6 +103,7 @@ describe("useRemoteOperations", () => { onForcePushComplete: vi.fn(), onFetchAttemptComplete, pushChanges: mocks.pushChanges, + pullWithStrategy: mocks.pullWithStrategy, })); let autoFetchPromise!: Promise; @@ -143,6 +146,7 @@ describe("useRemoteOperations", () => { onForcePushComplete: vi.fn(), onFetchAttemptComplete, pushChanges: mocks.pushChanges, + pullWithStrategy: mocks.pullWithStrategy, })); await act(async () => { @@ -173,11 +177,13 @@ describe("useRemoteOperations", () => { onForcePushComplete: vi.fn(), onFetchAttemptComplete, pushChanges: mocks.pushChanges, + pullWithStrategy: mocks.pullWithStrategy, })); await act(async () => { await result.current.pull(); }); + expect(mocks.pullWithStrategy).toHaveBeenCalledWith("ff-only"); expect(onFetchAttemptComplete).toHaveBeenCalledWith("/repo"); onFetchAttemptComplete.mockClear(); diff --git a/src/hooks/useRemoteOperations.ts b/src/hooks/useRemoteOperations.ts index 89dcf23..721ff2f 100644 --- a/src/hooks/useRemoteOperations.ts +++ b/src/hooks/useRemoteOperations.ts @@ -1,8 +1,10 @@ import {useCallback, useState} from "react"; +import type {TFunction} from "i18next"; import {useTranslation} from "react-i18next"; import * as api from "../api/commands"; import type { BranchInfo, + OperationResult, PullAnalysis, PullStrategy, PushRejectionAnalysis, @@ -15,6 +17,15 @@ import {appendResultLog} from "../utils/resultLog"; import type {ToastType} from "./useToast"; const AUTO_FETCH_TIMEOUT_MS = 90_000; +const MERGE_HOOK_ROLLBACK_FAILED = "GITMUN_MERGE_HOOK_ROLLBACK_FAILED"; +const MERGE_HOOK_ROLLBACK_INCOMPLETE = "GITMUN_MERGE_HOOK_ROLLBACK_INCOMPLETE"; + +function localiseMergeHookRollbackError(error: unknown, t: TFunction<"projectView">): string { + const message = String(error); + if (message.includes(MERGE_HOOK_ROLLBACK_FAILED)) return t("toast.mergeHookRollbackFailed"); + if (message.includes(MERGE_HOOK_ROLLBACK_INCOMPLETE)) return t("toast.mergeHookRollbackIncomplete"); + return message; +} type UpstreamDialogMode = "publish" | "repair" | "change"; @@ -30,6 +41,7 @@ type UseRemoteOperationsOptions = { onForcePushComplete: () => void; onFetchAttemptComplete: (repoPath: string) => void; pushChanges: (request: PushRequest) => Promise; + pullWithStrategy: (strategy: PullStrategy) => Promise | null>; }; export function buildPushRequestForCurrentBranch( @@ -63,6 +75,7 @@ export function useRemoteOperations({ onForcePushComplete, onFetchAttemptComplete, pushChanges, + pullWithStrategy, }: UseRemoteOperationsOptions) { const {t} = useTranslation("projectView"); const {t: tGitAdvice} = useTranslation("gitAdvice"); @@ -134,7 +147,8 @@ export function useRemoteOperations({ if (!repoPath || remoteOp) return; setRemoteOp("pull"); try { - const result = await api.pullWithStrategy(repoPath, strategy); + const result = await pullWithStrategy(strategy); + if (!result) return; const conflictStarted = /conflict resolution flow|needs conflict resolution/i.test(result.message); if (conflictStarted) { showToast(result.message, "info"); @@ -148,13 +162,15 @@ export function useRemoteOperations({ } await refreshAll(); } catch (error) { - showToast(String(error), "error"); - appendResultLog("error", t("log.pullFailed", {message: String(error)}), "unknown"); + const message = String(error); + await refreshAll().catch(() => undefined); + showToast(localiseMergeHookRollbackError(error, t), "error"); + appendResultLog("error", t("log.pullFailed", {message}), "unknown"); } finally { onFetchAttemptComplete(repoPath); setRemoteOp(null); } - }, [onFetchAttemptComplete, repoPath, remoteOp, refreshAll, showToast, t]); + }, [onFetchAttemptComplete, pullWithStrategy, repoPath, remoteOp, refreshAll, showToast, t]); const startPullFlow = useCallback(async () => { if (!repoPath || remoteOp) return; diff --git a/src/i18n/locales/en/centre.json b/src/i18n/locales/en/centre.json index 8a08bcc..d6deb1f 100644 --- a/src/i18n/locales/en/centre.json +++ b/src/i18n/locales/en/centre.json @@ -190,14 +190,16 @@ "gitHooks": { "bypassAction": { "commit": "Commit without hooks", + "merge": "Merge without hooks", + "pull": "Pull without hooks", "push": "Push without hooks" }, "bypassWarning": { "commit": "Committing without hooks skips repository checks.", + "merge": "Merging without hooks skips repository checks.", + "pull": "Pulling without hooks skips repository checks.", "push": "Pushing without hooks skips repository checks." }, - "checkoutWarningMessage": "The checkout changed, but its post-checkout hook reported a failure.", - "checkoutWarningTitle": "Checkout completed with a warning", "close": "Close", "dismiss": "Dismiss", "elapsed": "Running for {{seconds}}s", @@ -207,6 +209,9 @@ "operations": { "checkout": "Checkout", "commit": "Commit", + "merge": "Merge", + "pull": "Pull", + "rebase": "Rebase", "push": "Push" }, "outputTruncated": "Output was truncated.", @@ -214,7 +219,9 @@ "runningHook": "Running {{hook}} hook", "runningOperation": "{{operation}} in progress", "unknownExitStatus": "an unknown status", - "viewOutput": "View output" + "viewOutput": "View output", + "warningMessage": "The operation completed, but its {{hook}} hook reported a failure.", + "warningTitle": "{{operation}} completed with a warning" }, "pushRejected": { "cancel": "Cancel", diff --git a/src/i18n/locales/en/projectView.json b/src/i18n/locales/en/projectView.json index ef01e98..ad93010 100644 --- a/src/i18n/locales/en/projectView.json +++ b/src/i18n/locales/en/projectView.json @@ -146,6 +146,9 @@ "forceDeleteBranchFailed": "Force delete branch failed: {{message}}", "mergeAbortFailed": "Merge abort failed: {{message}}", "mergeFailed": "Merge failed: {{message}}", + "hookRejected": "{{hook}} rejected the {{operation}}.", + "hookWarning": "{{operation}} completed, but the {{hook}} hook failed.", + "hooksSkipped": "{{operation}} ran without verification hooks.", "exportPatchFailed": "Export patch failed: {{message}}", "importPatchFailed": "Import patch failed: {{message}}", "noPatchChanges": "No changes available for patch export", @@ -238,6 +241,9 @@ "fetchComplete": "Fetch complete", "fetchedFrom": "Fetched from {{remote}}", "integrationComplete": "Integration complete. Push your branch to update the remote.", + "hookWarning": "{{operation}} completed with a hook warning", + "mergeHookRollbackFailed": "The merge hook failed and Gitmun could not roll back the merge. Review the repository state before continuing.", + "mergeHookRollbackIncomplete": "The merge hook failed and Gitmun could not confirm that the merge was rolled back. Review the repository state before continuing.", "mergeConflicts_one": "Merge conflict in {{count}} file - resolve in the Changes tab", "mergeConflicts_other": "Merge conflicts in {{count}} files - resolve in the Changes tab", "mergeBlockedByChanges": "Commit or stash changes before merging", @@ -279,6 +285,12 @@ "upstreamChanged": "Upstream changed", "upstreamRepaired": "Upstream repaired" }, + "hookOperationNames": { + "merge": "merge", + "pull": "pull", + "push": "push", + "rebase": "rebase" + }, "patch": { "errors": { "GITMUN_ERROR_PATCH_EXPORT_COMMIT_HASH_EMPTY": "Select a commit before exporting a patch.", diff --git a/src/types.ts b/src/types.ts index 2aea457..926131a 100644 --- a/src/types.ts +++ b/src/types.ts @@ -264,7 +264,7 @@ export type GitHookAttemptResult = | { status: "completed"; result: T; hookWarning: GitHookFailure | null; outputTruncated: boolean } | ({ status: "hookRejected" } & GitHookFailure); -export type GitHookOperation = "commit" | "push" | "checkout"; +export type GitHookOperation = "commit" | "push" | "checkout" | "pull" | "merge" | "rebase"; export type GitHookProgressState = { operation: GitHookOperation; From cac302f9fe1d9cfffb3b1ee8bc2e568391934c11 Mon Sep 17 00:00:00 2001 From: cst8t <1810150+cst8t@users.noreply.github.com> Date: Mon, 14 Sep 2026 22:40:37 +0100 Subject: [PATCH 2/5] feat(avatar): add GitLab provider Implement GitLab avatar fetching by resolving remote base URLs from Git configuration and querying the GitLab API. Improve the avatar service to try multiple applicable platform providers until an image is found. --- src-tauri/src/avatar/gitlab.rs | 285 +++++++++++++++++++++++++++++++++ src-tauri/src/avatar/mod.rs | 45 +++++- 2 files changed, 325 insertions(+), 5 deletions(-) create mode 100644 src-tauri/src/avatar/gitlab.rs diff --git a/src-tauri/src/avatar/gitlab.rs b/src-tauri/src/avatar/gitlab.rs new file mode 100644 index 0000000..54cec33 --- /dev/null +++ b/src-tauri/src/avatar/gitlab.rs @@ -0,0 +1,285 @@ +use super::conditional::ConditionalProvider; +use base64::{Engine, engine::general_purpose::STANDARD}; +use url::Url; + +pub struct GitLabProvider { + client: reqwest::blocking::Client, +} + +impl GitLabProvider { + pub fn new() -> Self { + Self { + client: reqwest::blocking::Client::builder() + .timeout(std::time::Duration::from_secs(5)) + .user_agent("gitmun/0.1") + .build() + .unwrap_or_else(|_| reqwest::blocking::Client::new()), + } + } + + fn remote_base_url(remote: &str) -> Option { + let mut url = if remote.contains("://") { + Url::parse(remote).ok()? + } else { + let (authority, path) = remote.split_once(':')?; + if authority.contains('/') || remote.contains('\\') || path.is_empty() { + return None; + } + Url::parse(&format!("ssh://{authority}/{path}")).ok()? + }; + match url.scheme() { + "http" | "https" => {} + "ssh" => { + // The SSH port is unrelated to the instance's web port. + url.set_port(None).ok()?; + url = Url::parse(&format!("https://{}/", url.host()?)).ok()?; + } + _ => return None, + } + url.host_str()?; + url.set_username("").ok()?; + url.set_password(None).ok()?; + url.set_path("/"); + url.set_query(None); + url.set_fragment(None); + Some(url) + } + + fn base_urls(repo_path: &str) -> Vec { + let Ok(output) = crate::configured_git_command() + .args([ + "-C", + repo_path, + "config", + "--get-regexp", + r"^remote\..*\.url$", + ]) + .output() + else { + return Vec::new(); + }; + if !output.status.success() { + return Vec::new(); + } + let mut bases = Vec::new(); + for line in String::from_utf8_lossy(&output.stdout).lines() { + if let Some((_, remote)) = line.split_once(' ') { + if let Some(base) = Self::remote_base_url(remote.trim()) { + if !bases.contains(&base) { + bases.push(base); + } + } + } + } + bases + } + + fn fetch_from_instance(&self, email: &str, base: &Url) -> Option { + let mut endpoint = base.join("api/v4/avatar").ok()?; + endpoint + .query_pairs_mut() + .append_pair("email", email) + .append_pair("size", "64"); + let response = self + .client + .get(endpoint) + .send() + .ok()? + .error_for_status() + .ok()?; + let body: serde_json::Value = response.json().ok()?; + let raw = body.get("avatar_url")?.as_str()?.trim(); + if raw.is_empty() { + return None; + } + let avatar = base.join(raw).ok()?; + if !matches!(avatar.scheme(), "http" | "https") { + return None; + } + let response = self + .client + .get(avatar) + .send() + .ok()? + .error_for_status() + .ok()?; + let content_type = response + .headers() + .get("content-type")? + .to_str() + .ok()? + .split(';') + .next()? + .trim() + .to_string(); + if !content_type.starts_with("image/") { + return None; + } + let bytes = response.bytes().ok()?; + if bytes.is_empty() { + return None; + } + Some(format!( + "data:{content_type};base64,{}", + STANDARD.encode(&bytes) + )) + } +} + +impl ConditionalProvider for GitLabProvider { + fn applies_to(&self, repo_path: &str) -> bool { + !Self::base_urls(repo_path).is_empty() + } + + fn fetch(&self, email: &str, repo_path: &str) -> Option { + Self::base_urls(repo_path) + .iter() + .find_map(|base| self.fetch_from_instance(email, base)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::{BufRead, BufReader, Write}; + use std::net::TcpListener; + + #[test] + fn parses_self_hosted_remotes() { + for (remote, expected) in [ + ( + "https://user:secret@code.example.org:8443/team/sub/repo.git", + "https://code.example.org:8443/", + ), + ( + "http://code.example.org:8080/team/repo.git", + "http://code.example.org:8080/", + ), + ( + "git@code.example.org:team/sub/repo.git", + "https://code.example.org/", + ), + ( + "ssh://git@code.example.org:2222/team/repo.git", + "https://code.example.org/", + ), + ] { + assert_eq!( + GitLabProvider::remote_base_url(remote).unwrap().as_str(), + expected + ); + } + for remote in [ + "/tmp/repo", + "../repo", + "file:///tmp/repo", + "C:\\repo", + "git://code.example.org/repo", + ] { + assert!( + GitLabProvider::remote_base_url(remote).is_none(), + "{remote}" + ); + } + } + + #[test] + fn reads_remotes_through_a_git_directory_file_and_deduplicates_hosts() { + let directory = tempfile::tempdir().unwrap(); + let repository = directory.path().join("repository"); + let git_directory = directory.path().join("git-directory"); + let output = crate::configured_git_command() + .arg("init") + .arg("--separate-git-dir") + .arg(&git_directory) + .arg(&repository) + .output() + .unwrap(); + assert!(output.status.success()); + std::fs::write( + git_directory.join("config"), + "[remote \"origin\"]\nurl = https://code.example.org/team/repo.git\n\ + [remote \"upstream\"]\nurl = git@code.example.org:team/upstream.git\n\ + [remote \"mirror\"]\nurl = https://mirror.example.org/team/repo.git\n", + ) + .unwrap(); + assert_eq!( + GitLabProvider::base_urls(repository.to_str().unwrap()), + vec![ + Url::parse("https://code.example.org/").unwrap(), + Url::parse("https://mirror.example.org/").unwrap(), + ] + ); + } + + #[test] + fn downloads_uploaded_avatar_and_handles_missing_or_restricted_avatars() { + for (status, body, image_type, expected) in [ + ( + "200 OK", + r#"{"avatar_url":"/uploads/-/system/user/avatar/42/avatar.png"}"#, + "image/png", + Some("data:image/png;base64,YXZhdGFy"), + ), + ("200 OK", r#"{"avatar_url":null}"#, "", None), + ("200 OK", r#"{"avatar_url":""}"#, "", None), + ("403 Forbidden", r#"{"message":"403 Forbidden"}"#, "", None), + ( + "200 OK", + r#"{"avatar_url":"/users/sign_in"}"#, + "text/html", + None, + ), + ] { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let base = Url::parse(&format!("http://{}/", listener.local_addr().unwrap())).unwrap(); + let server = std::thread::spawn(move || { + for (index, (response_status, response_type, response_body)) in + std::iter::once((status, "application/json", body)) + .chain((!image_type.is_empty()).then_some(("200 OK", image_type, "avatar"))) + .enumerate() + { + let (mut stream, _) = listener.accept().unwrap(); + stream + .set_read_timeout(Some(std::time::Duration::from_secs(5))) + .unwrap(); + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut request = String::new(); + reader.read_line(&mut request).unwrap(); + if index == 0 { + assert_eq!( + request.trim(), + "GET /api/v4/avatar?email=author%2Bgit%40example.com&size=64 HTTP/1.1" + ); + } else { + assert!( + request.contains("/uploads/") || request.contains("/users/sign_in") + ); + } + loop { + let mut header = String::new(); + reader.read_line(&mut header).unwrap(); + if header == "\r\n" || header.is_empty() { + break; + } + } + write!(stream, "HTTP/1.1 {response_status}\r\nContent-Type: {response_type}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{response_body}", response_body.len()).unwrap(); + } + }); + let provider = GitLabProvider { + client: reqwest::blocking::Client::builder() + .no_proxy() + .timeout(std::time::Duration::from_secs(5)) + .build() + .unwrap(), + }; + assert_eq!( + provider + .fetch_from_instance("author+git@example.com", &base) + .as_deref(), + expected + ); + server.join().unwrap(); + } + } +} diff --git a/src-tauri/src/avatar/mod.rs b/src-tauri/src/avatar/mod.rs index bb62570..5b6dc82 100644 --- a/src-tauri/src/avatar/mod.rs +++ b/src-tauri/src/avatar/mod.rs @@ -1,6 +1,7 @@ mod conditional; mod forgejo; mod github; +mod gitlab; mod libravatar; mod provider; @@ -10,6 +11,7 @@ pub use provider::AvatarProvider; use crate::git::types::AvatarProviderMode; use forgejo::ForgejoProvider; use github::GitHubProvider; +use gitlab::GitLabProvider; use libravatar::LibravatarProvider; use std::collections::HashMap; use std::sync::{Mutex, MutexGuard}; @@ -48,9 +50,7 @@ impl AvatarService { conditional_providers: vec![ Box::new(GitHubProvider::new()), Box::new(ForgejoProvider::new()), - // Add further platform providers here, e.g.: - // Box::new(GitLabProvider::new()), - // Box::new(BitbucketProvider::new()), + Box::new(GitLabProvider::new()), ], try_platform_first: Mutex::new(try_platform_first), cache: Mutex::new(HashMap::new()), @@ -93,8 +93,8 @@ impl AvatarService { let conditional_result = self .conditional_providers .iter() - .find(|p| p.applies_to(repo_path)) - .and_then(|p| p.fetch(&key_email, repo_path)); + .filter(|p| p.applies_to(repo_path)) + .find_map(|p| p.fetch(&key_email, repo_path)); conditional_result.or_else(|| self.fetch_from_provider(&key_email)) } else { self.fetch_from_provider(&key_email) @@ -111,3 +111,38 @@ impl AvatarService { provider.as_ref().and_then(|p| p.fetch(email)) } } + +#[cfg(test)] +mod tests { + use super::*; + + struct PlatformAvatar(Option<&'static str>); + + impl ConditionalProvider for PlatformAvatar { + fn applies_to(&self, _repo_path: &str) -> bool { + true + } + + fn fetch(&self, _email: &str, _repo_path: &str) -> Option { + self.0.map(str::to_string) + } + } + + #[test] + fn tries_next_platform_when_first_has_no_avatar() { + let mut service = AvatarService::new(AvatarProviderMode::Off, true); + service.conditional_providers = vec![ + Box::new(PlatformAvatar(None)), + Box::new(PlatformAvatar(Some("data:image/png;base64,YXZhdGFy"))), + ]; + assert_eq!( + service.fetch("author@example.com", "/tmp/avatar-repository"), + Some("data:image/png;base64,YXZhdGFy".to_string()) + ); + service.set_try_platform_first(false); + assert_eq!( + service.fetch("author@example.com", "/tmp/avatar-repository"), + None + ); + } +} From b7869ae5031b9eabb4408e4e6aaf0a72b988bb7d Mon Sep 17 00:00:00 2001 From: cst8t <1810150+cst8t@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:42:48 +0100 Subject: [PATCH 3/5] feat(ui): make commit graph button permanently visible Remove the setting to toggle the commit graph button and make the button visible by default in the log view. --- src-tauri/config.example.toml | 3 -- src-tauri/src/commands/settings.rs | 10 ------ src-tauri/src/config_file.rs | 25 +-------------- src-tauri/src/git/handler.rs | 16 ---------- src-tauri/src/git/types.rs | 3 -- src-tauri/src/lib.rs | 1 - src/api/commands.ts | 4 --- src/components/ProjectView.tsx | 6 +--- src/components/centre/CentrePanel.test.tsx | 12 ------- src/components/centre/CentrePanel.tsx | 32 ++++++++----------- src/components/centre/LogView.test.tsx | 1 - .../settings/SettingsWindow.test.tsx | 20 ------------ src/components/settings/SettingsWindow.tsx | 23 ------------- src/i18n/locales/en/settings.json | 3 -- src/types.ts | 1 - 15 files changed, 16 insertions(+), 144 deletions(-) diff --git a/src-tauri/config.example.toml b/src-tauri/config.example.toml index 42e5d3d..73424c5 100644 --- a/src-tauri/config.example.toml +++ b/src-tauri/config.example.toml @@ -20,9 +20,6 @@ uiTextScale = 1.0 # Whether to wrap long lines in diff views. wrapDiffLines = false -# Experimental: show the commit graph toolbar button. -showCommitGraphButton = false - # Experimental: enable Local Copy in the Clone window and CLI. enableLocalCopy = false diff --git a/src-tauri/src/commands/settings.rs b/src-tauri/src/commands/settings.rs index 982b752..67f7704 100644 --- a/src-tauri/src/commands/settings.rs +++ b/src-tauri/src/commands/settings.rs @@ -232,16 +232,6 @@ pub fn set_row_striping(row_striping: RowStriping, state: tauri::State<'_, AppSt state.git_service.set_row_striping(row_striping) } -#[tauri::command] -pub fn set_show_commit_graph_button( - show_commit_graph_button: bool, - state: tauri::State<'_, AppState>, -) -> Settings { - state - .git_service - .set_show_commit_graph_button(show_commit_graph_button) -} - #[tauri::command] pub fn set_enable_local_copy( enable_local_copy: bool, diff --git a/src-tauri/src/config_file.rs b/src-tauri/src/config_file.rs index 165e209..a61ec43 100644 --- a/src-tauri/src/config_file.rs +++ b/src-tauri/src/config_file.rs @@ -554,7 +554,6 @@ mod tests { assert!(toml_text.contains("# Backend used for Git operations")); assert!(toml_text.contains("backendMode = \"Default\"")); assert!(toml_text.contains("uiTextScale = 1.0")); - assert!(toml_text.contains("showCommitGraphButton = false")); assert!(toml_text.contains("errorToastClearDelayMs = 8000")); assert!(toml_text.contains("commitMessageRecommendedLength = 72")); assert!(!toml_text.contains("enableUpdateWithMSStoreFlow")); @@ -632,10 +631,9 @@ mod tests { ); assert!(updated.contains("uiTextScale = 1.0")); assert!( - updated.contains("# Experimental: show the commit graph toolbar button."), + updated.contains("# Experimental: enable Local Copy in the Clone window and CLI."), "missing key gained its template comment" ); - assert!(updated.contains("showCommitGraphButton = false")); assert!(updated.contains("enableLocalCopy = false")); assert!(updated.contains("# Maximum context sent in each AI commit-message request")); assert!(updated.contains("commitContextLimitKib = 24")); @@ -652,13 +650,6 @@ mod tests { assert!(!updated.contains("enableUpdateWithMSStoreFlow")); } - #[test] - fn missing_commit_graph_button_defaults_to_false() { - let settings: Settings = toml::from_str("backendMode = \"Default\"\n").unwrap(); - - assert!(!settings.show_commit_graph_button); - } - #[test] fn missing_local_copy_setting_defaults_to_false() { let settings: Settings = toml::from_str("backendMode = \"Default\"\n").unwrap(); @@ -680,20 +671,6 @@ mod tests { assert!(updated.contains("enableLocalCopy = true")); } - #[test] - fn persist_writes_commit_graph_button() { - let dir = TempDir::new().unwrap(); - let toml_path = dir.path().join("config.toml"); - - let mut settings = Settings::default(); - settings.show_commit_graph_button = true; - - persist(&toml_path, &settings).unwrap(); - - let updated = std::fs::read_to_string(&toml_path).unwrap(); - assert!(updated.contains("showCommitGraphButton = true")); - } - #[test] fn persist_leaves_old_ms_store_update_flow_config_untouched() { let dir = TempDir::new().unwrap(); diff --git a/src-tauri/src/git/handler.rs b/src-tauri/src/git/handler.rs index ee1747b..6874224 100644 --- a/src-tauri/src/git/handler.rs +++ b/src-tauri/src/git/handler.rs @@ -271,12 +271,6 @@ impl GitService { }) } - pub fn set_show_commit_graph_button(&self, show_commit_graph_button: bool) -> Settings { - self.update_settings(|settings| { - settings.show_commit_graph_button = show_commit_graph_button; - }) - } - pub fn set_enable_local_copy(&self, enable_local_copy: bool) -> Settings { self.update_settings(|settings| { settings.enable_local_copy = enable_local_copy; @@ -867,16 +861,6 @@ impl GitService { mod tests { use super::GitService; - #[test] - fn set_show_commit_graph_button_updates_settings() { - let service = GitService::new(); - - let settings = service.set_show_commit_graph_button(true); - - assert!(settings.show_commit_graph_button); - assert!(service.get_settings().show_commit_graph_button); - } - #[test] fn set_enable_local_copy_updates_settings() { let service = GitService::new(); diff --git a/src-tauri/src/git/types.rs b/src-tauri/src/git/types.rs index 09c6dd9..1bdefbd 100644 --- a/src-tauri/src/git/types.rs +++ b/src-tauri/src/git/types.rs @@ -286,8 +286,6 @@ pub struct Settings { #[serde(default)] pub row_striping: RowStriping, #[serde(default)] - pub show_commit_graph_button: bool, - #[serde(default)] pub enable_local_copy: bool, #[serde(default)] pub persistent_error_toasts: bool, @@ -411,7 +409,6 @@ impl Default for Settings { ui_text_scale: default_ui_text_scale(), wrap_diff_lines: false, row_striping: RowStriping::Off, - show_commit_graph_button: false, enable_local_copy: false, persistent_error_toasts: false, error_toast_clear_delay_ms: DEFAULT_ERROR_TOAST_CLEAR_DELAY_MS, diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 51d83be..a25617e 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -1571,7 +1571,6 @@ pub fn run() { commands::settings::set_ui_text_scale, commands::settings::set_wrap_diff_lines, commands::settings::set_row_striping, - commands::settings::set_show_commit_graph_button, commands::settings::set_enable_local_copy, commands::settings::set_persistent_error_toasts, commands::settings::set_error_toast_clear_delay_ms, diff --git a/src/api/commands.ts b/src/api/commands.ts index c985a90..77e0b65 100644 --- a/src/api/commands.ts +++ b/src/api/commands.ts @@ -518,10 +518,6 @@ export function setRowStriping(rowStriping: RowStriping): Promise { return invoke("set_row_striping", {rowStriping}); } -export function setShowCommitGraphButton(showCommitGraphButton: boolean): Promise { - return invoke("set_show_commit_graph_button", {showCommitGraphButton}); -} - export function setPersistentErrorToasts(persistentErrorToasts: boolean): Promise { return invoke("set_persistent_error_toasts", {persistentErrorToasts}); } diff --git a/src/components/ProjectView.tsx b/src/components/ProjectView.tsx index ea69f3b..21c1acf 100644 --- a/src/components/ProjectView.tsx +++ b/src/components/ProjectView.tsx @@ -457,7 +457,6 @@ export function ProjectView({ const [rebasedBranchAwaitingPush, setRebasedBranchAwaitingPush] = useState(null); const [wrapDiffLines, setWrapDiffLines] = useState(false); const [rowStriping, setRowStriping] = useState("Off"); - const [showCommitGraphButton, setShowCommitGraphButton] = useState(false); const [showCommitGraph, setShowCommitGraph] = useState(readShowCommitGraphPreference); const [showAiWriting, setShowAiWriting] = useState(false); const [searchQuery, setSearchQuery] = useState(""); @@ -519,7 +518,7 @@ export function ProjectView({ loadMoreError: logLoadMoreError, pageSize: logPageSize, refresh: refreshLog, - } = useGitLog(repoPath, logScope, windowFocused, showCommitGraphButton && showCommitGraph); + } = useGitLog(repoPath, logScope, windowFocused, showCommitGraph); const searching = deferredSearchQuery.length > 0; const visibleCommits = useMemo(() => { if (!searching) return commits; @@ -704,7 +703,6 @@ export function ProjectView({ setPushFollowTags(settings.pushFollowTags ?? false); setWrapDiffLines(settings.wrapDiffLines ?? false); setRowStriping(settings.rowStriping ?? "Off"); - setShowCommitGraphButton(settings.showCommitGraphButton ?? false); } }) .catch(() => { @@ -714,7 +712,6 @@ export function ProjectView({ setPushFollowTags(false); setWrapDiffLines(false); setRowStriping("Off"); - setShowCommitGraphButton(false); } }); @@ -2583,7 +2580,6 @@ export function ProjectView({ commitMarkers={commitMarkers} logScope={logScope} rowStriping={rowStriping} - showCommitGraphButton={showCommitGraphButton} onCommitGraphVisibilityChange={handleCommitGraphVisibilityChange} onLogScopeChange={setLogScope} detachedHead={status?.detachedHead ?? false} diff --git a/src/components/centre/CentrePanel.test.tsx b/src/components/centre/CentrePanel.test.tsx index 922b439..3791684 100644 --- a/src/components/centre/CentrePanel.test.tsx +++ b/src/components/centre/CentrePanel.test.tsx @@ -88,7 +88,6 @@ function renderCentrePanel(overrides: Partial { expect(screen.getByLabelText("Hide commit graph")).toBeInTheDocument(); }); - it("hides the graph button and forces the graph hidden when the setting is off", () => { - localStorage.setItem("gitmun.showCommitGraph", "true"); - - const { container } = renderCentrePanel({ showCommitGraphButton: false }); - - expect(screen.queryByLabelText("Hide commit graph")).not.toBeInTheDocument(); - expect(screen.queryByLabelText("Show commit graph")).not.toBeInTheDocument(); - expect(container.querySelector(".log-view__graph")).toBeNull(); - expect(localStorage.getItem("gitmun.showCommitGraph")).toBe("true"); - }); - it("disables the commit graph while searching without changing the saved preference", () => { localStorage.setItem("gitmun.showCommitGraph", "true"); const onCommitGraphVisibilityChange = vi.fn(); diff --git a/src/components/centre/CentrePanel.tsx b/src/components/centre/CentrePanel.tsx index f44b288..5c836c9 100644 --- a/src/components/centre/CentrePanel.tsx +++ b/src/components/centre/CentrePanel.tsx @@ -70,7 +70,6 @@ type CentrePanelProps = { commitMarkers: CommitMarkers; logScope: CommitLogScope; rowStriping: RowStriping; - showCommitGraphButton: boolean; onCommitGraphVisibilityChange?: (visible: boolean) => void; onLogScopeChange: (scope: CommitLogScope) => void; detachedHead: boolean; @@ -273,8 +272,7 @@ function getOperationContent( export function CentrePanel(props: CentrePanelProps) { const { t } = useTranslation("centre"); const [showCommitGraph, setShowCommitGraph] = React.useState(readShowCommitGraphPreference); - const preferredShowCommitGraph = props.showCommitGraphButton && showCommitGraph; - const effectiveShowCommitGraph = preferredShowCommitGraph && !props.searching; + const effectiveShowCommitGraph = showCommitGraph && !props.searching; const tab = props.activeTab; const operationContent = getOperationContent(props.operationLock, t); const operationFeedback = useDelayedOperationFeedback(props.operationLock); @@ -286,8 +284,8 @@ export function CentrePanel(props: CentrePanelProps) { const totalChanges = props.stagedFiles.length + props.unstagedFiles.length + props.unversionedFiles.length + submoduleChanges; React.useEffect(() => { - props.onCommitGraphVisibilityChange?.(preferredShowCommitGraph); - }, [preferredShowCommitGraph, props.onCommitGraphVisibilityChange]); + props.onCommitGraphVisibilityChange?.(showCommitGraph); + }, [showCommitGraph, props.onCommitGraphVisibilityChange]); const handleToggleCommitGraph = () => { setShowCommitGraph(previous => { @@ -370,19 +368,17 @@ export function CentrePanel(props: CentrePanelProps) {
{tab === "log" && (
- {props.showCommitGraphButton && ( - - )} +