From 06e366e91413aacb3686f94b51efd16626034bb4 Mon Sep 17 00:00:00 2001 From: Roope Airinen Date: Wed, 8 Apr 2026 17:48:55 +0300 Subject: [PATCH 1/6] removed strict GIT_ALLOW_PROTOCOL=file to allow for https and ssh submodule imports --- .../gitcomet-git-gix/src/repo/submodules.rs | 46 +++++++++++++++---- crates/gitcomet-git-gix/src/util.rs | 2 +- .../tests/submodules_integration.rs | 28 +++++++++++ 3 files changed, 65 insertions(+), 11 deletions(-) diff --git a/crates/gitcomet-git-gix/src/repo/submodules.rs b/crates/gitcomet-git-gix/src/repo/submodules.rs index 1b1fc603..1dd7ad3d 100644 --- a/crates/gitcomet-git-gix/src/repo/submodules.rs +++ b/crates/gitcomet-git-gix/src/repo/submodules.rs @@ -6,6 +6,14 @@ use gitcomet_core::error::{Error, ErrorKind}; use gitcomet_core::services::{CommandOutput, Result}; use std::collections::BTreeMap; use std::path::{Path, PathBuf}; +use std::process::Command; + +fn allow_file_submodule_transport(cmd: &mut Command) { + // `git submodule` blocks local-path remotes unless `protocol.file.allow` is enabled. + // Use per-command config so local workflows keep working without disabling `https`/`ssh`. + cmd.arg("-c").arg("protocol.file.allow=always"); +} + impl GixRepo { pub(super) fn list_submodules_impl(&self) -> Result> { let repo = self.reopen_repo()?; @@ -21,24 +29,18 @@ impl GixRepo { path: &Path, ) -> Result { let mut cmd = self.git_workdir_cmd(); - cmd.arg("submodule") - .arg("add") - .arg(url) - .arg(path) - // Local-path submodule URLs are used in tests and supported workflows. - // Explicitly allow `file` transport for this command. - .env("GIT_ALLOW_PROTOCOL", "file"); + allow_file_submodule_transport(&mut cmd); + cmd.arg("submodule").arg("add").arg(url).arg(path); run_git_with_output(cmd, &format!("git submodule add {url} {}", path.display())) } pub(super) fn update_submodules_with_output_impl(&self) -> Result { let mut cmd = self.git_workdir_cmd(); + allow_file_submodule_transport(&mut cmd); cmd.arg("submodule") .arg("update") .arg("--init") - .arg("--recursive") - // Keep behavior consistent with add: allow local-path submodule URLs. - .env("GIT_ALLOW_PROTOCOL", "file"); + .arg("--recursive"); run_git_with_output(cmd, "git submodule update --init --recursive") } @@ -255,3 +257,27 @@ fn pathbuf_from_gix_path(path: &gix::bstr::BStr) -> Result { fn object_id_to_commit_id(id: gix::ObjectId) -> CommitId { CommitId(id.to_string().into()) } + +#[cfg(test)] +mod tests { + use super::allow_file_submodule_transport; + use std::ffi::OsStr; + use std::process::Command; + + #[test] + fn allow_file_submodule_transport_uses_git_config_not_protocol_allowlist() { + let mut cmd = Command::new("git"); + + allow_file_submodule_transport(&mut cmd); + + let args = cmd + .get_args() + .map(|arg| arg.to_string_lossy().into_owned()) + .collect::>(); + assert_eq!(args, ["-c", "protocol.file.allow=always"]); + assert!( + !cmd.get_envs() + .any(|(key, _)| key == OsStr::new("GIT_ALLOW_PROTOCOL")) + ); + } +} diff --git a/crates/gitcomet-git-gix/src/util.rs b/crates/gitcomet-git-gix/src/util.rs index c8c4acbb..5b91d047 100644 --- a/crates/gitcomet-git-gix/src/util.rs +++ b/crates/gitcomet-git-gix/src/util.rs @@ -169,7 +169,7 @@ fn apply_test_git_command_environment(cmd: &mut Command) { cmd.env("HOME", &env.home_dir); cmd.env("XDG_CONFIG_HOME", &env.xdg_config_home); cmd.env("GNUPGHOME", &env.gnupg_home); - cmd.env("GIT_ALLOW_PROTOCOL", "file"); + cmd.arg("-c").arg("protocol.file.allow=always"); } pub(crate) fn git_workdir_cmd_for(workdir: &Path) -> Command { diff --git a/crates/gitcomet-git-gix/tests/submodules_integration.rs b/crates/gitcomet-git-gix/tests/submodules_integration.rs index 72c61b6b..ffc6eb0b 100644 --- a/crates/gitcomet-git-gix/tests/submodules_integration.rs +++ b/crates/gitcomet-git-gix/tests/submodules_integration.rs @@ -568,3 +568,31 @@ fn submodule_add_update_remove_round_trip() { assert!(listed_after_remove.is_empty()); assert!(!parent_repo.join("mods/sub-one").exists()); } + +#[test] +fn add_submodule_does_not_restrict_https_or_ssh_transports() { + if !require_git_shell_for_submodule_tests() { + return; + } + let dir = tempfile::tempdir().expect("create tempdir"); + let repo = dir.path().join("parent"); + fs::create_dir_all(&repo).expect("create parent repository directory"); + init_repo_with_seed(&repo, "seed.txt", "seed\n", "seed parent"); + + let backend = GixBackend; + let opened = backend.open(&repo).expect("open parent repository"); + + for (url, blocked_transport) in [ + ("https://127.0.0.1:1/repo.git", "https"), + ("ssh://git@127.0.0.1:1/repo.git", "ssh"), + ] { + let err = opened + .add_submodule_with_output(url, Path::new("mods/sub-one")) + .expect_err("dummy remote should fail without a reachable server"); + let rendered = err.to_string(); + assert!( + !rendered.contains(&format!("transport '{blocked_transport}' not allowed")), + "unexpected protocol allowlist failure for {url}: {rendered}" + ); + } +} From e28d98ff82552b0a5e2b7ec2ce5baa2d52e2d9d1 Mon Sep 17 00:00:00 2001 From: Roope Airinen Date: Fri, 10 Apr 2026 10:58:34 +0300 Subject: [PATCH 2/6] changed file protocol to be trust gated and added a popup for GUI. this better aligns with cve-2022-39253 --- crates/gitcomet-core/src/services.rs | 37 +- crates/gitcomet-git-gix/src/repo/mod.rs | 30 +- .../gitcomet-git-gix/src/repo/submodules.rs | 432 +++++++++++++++++- .../tests/submodules_integration.rs | 73 ++- .../tests/support/test_git_env.rs | 20 + crates/gitcomet-git/src/noop_backend.rs | 6 +- crates/gitcomet-state/src/model.rs | 16 +- crates/gitcomet-state/src/msg/effect.rs | 14 +- crates/gitcomet-state/src/msg/message.rs | 27 +- .../gitcomet-state/src/msg/message_debug.rs | 17 + .../src/msg/repo_command_kind.rs | 9 +- crates/gitcomet-state/src/store/effects.rs | 26 +- .../src/store/effects/repo_commands.rs | 57 ++- crates/gitcomet-state/src/store/reducer.rs | 105 ++++- .../src/store/reducer/actions_emit_effects.rs | 18 +- .../gitcomet-state/src/store/reducer/util.rs | 4 +- .../src/store/tests/actions_emit_effects.rs | 147 +++++- .../gitcomet-state/src/store/tests/effects.rs | 2 + crates/gitcomet-ui-gpui/src/view/mod.rs | 14 +- .../gitcomet-ui-gpui/src/view/mod_helpers.rs | 3 + .../src/view/panels/popover.rs | 9 + .../src/view/panels/popover/fingerprint.rs | 32 ++ .../panels/popover/submodule_trust_confirm.rs | 159 +++++++ .../gitcomet-ui-gpui/src/view/state_apply.rs | 4 + 24 files changed, 1194 insertions(+), 67 deletions(-) create mode 100644 crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs diff --git a/crates/gitcomet-core/src/services.rs b/crates/gitcomet-core/src/services.rs index ae0d2088..241d7786 100644 --- a/crates/gitcomet-core/src/services.rs +++ b/crates/gitcomet-core/src/services.rs @@ -113,6 +113,19 @@ pub enum RemoteUrlKind { Push, } +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct SubmoduleTrustTarget { + pub submodule_path: PathBuf, + pub display_source: String, + pub local_source_path: PathBuf, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum SubmoduleTrustDecision { + Proceed, + Prompt { sources: Vec }, +} + #[derive(Clone, Debug, Eq, PartialEq)] pub struct BlameLine { pub commit_id: Arc, @@ -526,13 +539,33 @@ pub trait GitRepository: Send + Sync { ))) } - fn add_submodule_with_output(&self, _url: &str, _path: &Path) -> Result { + fn check_submodule_add_trust(&self, _url: &str, _path: &Path) -> Result { + Err(Error::new(ErrorKind::Unsupported( + "submodule trust checks are not implemented for this backend", + ))) + } + + fn check_submodule_update_trust(&self) -> Result { + Err(Error::new(ErrorKind::Unsupported( + "submodule trust checks are not implemented for this backend", + ))) + } + + fn add_submodule_with_output( + &self, + _url: &str, + _path: &Path, + _approved_sources: &[SubmoduleTrustTarget], + ) -> Result { Err(Error::new(ErrorKind::Unsupported( "submodule add is not implemented for this backend", ))) } - fn update_submodules_with_output(&self) -> Result { + fn update_submodules_with_output( + &self, + _approved_sources: &[SubmoduleTrustTarget], + ) -> Result { Err(Error::new(ErrorKind::Unsupported( "submodule update is not implemented for this backend", ))) diff --git a/crates/gitcomet-git-gix/src/repo/mod.rs b/crates/gitcomet-git-gix/src/repo/mod.rs index 08dab017..943d4b3b 100644 --- a/crates/gitcomet-git-gix/src/repo/mod.rs +++ b/crates/gitcomet-git-gix/src/repo/mod.rs @@ -9,7 +9,7 @@ use gitcomet_core::error::{Error, ErrorKind}; use gitcomet_core::git_ops_trace::{self, GitOpTraceKind}; use gitcomet_core::services::{ BlameLine, CommandOutput, ConflictFileStages, ConflictSide, GitRepository, MergetoolResult, - PullMode, RemoteUrlKind, ResetMode, Result, + PullMode, RemoteUrlKind, ResetMode, Result, SubmoduleTrustDecision, SubmoduleTrustTarget, }; use std::path::{Path, PathBuf}; use std::process::Command; @@ -511,12 +511,32 @@ impl GitRepository for GixRepo { self.list_submodules_impl() } - fn add_submodule_with_output(&self, url: &str, path: &Path) -> Result { - self.add_submodule_with_output_impl(url, path) + fn check_submodule_add_trust( + &self, + url: &str, + path: &Path, + ) -> Result { + self.check_submodule_add_trust_impl(url, path) } - fn update_submodules_with_output(&self) -> Result { - self.update_submodules_with_output_impl() + fn check_submodule_update_trust(&self) -> Result { + self.check_submodule_update_trust_impl() + } + + fn add_submodule_with_output( + &self, + url: &str, + path: &Path, + approved_sources: &[SubmoduleTrustTarget], + ) -> Result { + self.add_submodule_with_output_impl(url, path, approved_sources) + } + + fn update_submodules_with_output( + &self, + approved_sources: &[SubmoduleTrustTarget], + ) -> Result { + self.update_submodules_with_output_impl(approved_sources) } fn remove_submodule_with_output(&self, path: &Path) -> Result { diff --git a/crates/gitcomet-git-gix/src/repo/submodules.rs b/crates/gitcomet-git-gix/src/repo/submodules.rs index 1dd7ad3d..0f77781f 100644 --- a/crates/gitcomet-git-gix/src/repo/submodules.rs +++ b/crates/gitcomet-git-gix/src/repo/submodules.rs @@ -1,9 +1,15 @@ use super::GixRepo; use super::history::gix_head_id_or_none; -use crate::util::run_git_with_output; +use crate::util::{ + bytes_to_text_preserving_utf8, git_workdir_cmd_for, run_git_simple, run_git_with_output, +}; use gitcomet_core::domain::{CommitId, Submodule, SubmoduleStatus}; use gitcomet_core::error::{Error, ErrorKind}; -use gitcomet_core::services::{CommandOutput, Result}; +use gitcomet_core::path_utils::canonicalize_or_original; +use gitcomet_core::services::{ + CommandOutput, Result, SubmoduleTrustDecision, SubmoduleTrustTarget, +}; +use gix::bstr::ByteSlice as _; use std::collections::BTreeMap; use std::path::{Path, PathBuf}; use std::process::Command; @@ -23,25 +29,90 @@ impl GixRepo { Ok(submodules) } + pub(super) fn check_submodule_add_trust_impl( + &self, + url: &str, + path: &Path, + ) -> Result { + let repo = self.reopen_repo()?; + let Some(target) = trust_target_from_raw_source( + repo_workdir_for_submodule_trust(&repo), + path, + url, + )? + else { + return Ok(SubmoduleTrustDecision::Proceed); + }; + + if submodule_source_trusted(repo_workdir_for_submodule_trust(&repo), &target)? { + Ok(SubmoduleTrustDecision::Proceed) + } else { + Ok(SubmoduleTrustDecision::Prompt { + sources: vec![target], + }) + } + } + + pub(super) fn check_submodule_update_trust_impl(&self) -> Result { + let repo = self.reopen_repo()?; + let trust_root = repo_workdir_for_submodule_trust(&repo); + let mut sources = BTreeMap::new(); + collect_repo_untrusted_submodule_sources(&repo, trust_root, Path::new(""), &mut sources)?; + if sources.is_empty() { + Ok(SubmoduleTrustDecision::Proceed) + } else { + Ok(SubmoduleTrustDecision::Prompt { + sources: sources.into_values().collect(), + }) + } + } + pub(super) fn add_submodule_with_output_impl( &self, url: &str, path: &Path, + approved_sources: &[SubmoduleTrustTarget], ) -> Result { + let repo = self.reopen_repo()?; + let trust_root = repo_workdir_for_submodule_trust(&repo); + persist_submodule_trust_approvals(trust_root, approved_sources)?; + let mut cmd = self.git_workdir_cmd(); - allow_file_submodule_transport(&mut cmd); + if let Some(target) = trust_target_from_raw_source(trust_root, path, url)? { + if !submodule_source_trusted(trust_root, &target)? { + return Err(untrusted_local_submodule_error(&target, "add")); + } + allow_file_submodule_transport(&mut cmd); + } + cmd.arg("submodule").arg("add").arg(url).arg(path); run_git_with_output(cmd, &format!("git submodule add {url} {}", path.display())) } - pub(super) fn update_submodules_with_output_impl(&self) -> Result { - let mut cmd = self.git_workdir_cmd(); - allow_file_submodule_transport(&mut cmd); - cmd.arg("submodule") - .arg("update") - .arg("--init") - .arg("--recursive"); - run_git_with_output(cmd, "git submodule update --init --recursive") + pub(super) fn update_submodules_with_output_impl( + &self, + approved_sources: &[SubmoduleTrustTarget], + ) -> Result { + let repo = self.reopen_repo()?; + let trust_root = repo_workdir_for_submodule_trust(&repo).to_path_buf(); + persist_submodule_trust_approvals(&trust_root, approved_sources)?; + + let mut outputs = Vec::new(); + update_repo_submodules_recursive( + &repo, + &trust_root, + Path::new(""), + approved_sources, + &mut outputs, + )?; + + if outputs.is_empty() { + Ok(CommandOutput::empty_success( + "git submodule update --init --recursive", + )) + } else { + Ok(combine_submodule_update_outputs(outputs)) + } } pub(super) fn remove_submodule_with_output_impl(&self, path: &Path) -> Result { @@ -144,6 +215,98 @@ fn collect_repo_submodules( Ok(()) } +fn collect_repo_untrusted_submodule_sources( + repo: &gix::Repository, + trust_root: &Path, + prefix: &Path, + out: &mut BTreeMap, +) -> Result<()> { + let Some(submodules) = repo + .submodules() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodules: {e}"))))? + else { + return Ok(()); + }; + + let current_workdir = repo_workdir_for_submodule_trust(repo); + for submodule in submodules { + let relative_path = submodule + .path() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule path: {e}")))) + .and_then(|path| pathbuf_from_gix_path(path.as_ref()))?; + let full_path = prefix.join(&relative_path); + + if let Some(target) = trust_target_from_submodule(current_workdir, &full_path, &submodule)? { + if !submodule_source_trusted(trust_root, &target)? { + out.insert(full_path.clone(), target); + } + } + + if let Some(nested_repo) = open_configured_submodule_repo(&submodule)? { + collect_repo_untrusted_submodule_sources(&nested_repo, trust_root, &full_path, out)?; + } + } + + Ok(()) +} + +fn update_repo_submodules_recursive( + repo: &gix::Repository, + trust_root: &Path, + prefix: &Path, + approved_sources: &[SubmoduleTrustTarget], + outputs: &mut Vec, +) -> Result<()> { + let Some(submodules) = repo + .submodules() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodules: {e}"))))? + else { + return Ok(()); + }; + + let current_workdir = repo_workdir_for_submodule_trust(repo); + for submodule in submodules { + let relative_path = submodule + .path() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule path: {e}")))) + .and_then(|path| pathbuf_from_gix_path(path.as_ref()))?; + let full_path = prefix.join(&relative_path); + + let local_target = + trust_target_from_submodule(current_workdir, &full_path, &submodule)?; + + let mut cmd = git_workdir_cmd_for(current_workdir); + if let Some(target) = local_target.as_ref() { + if !submodule_source_trusted(trust_root, target)? { + return Err(untrusted_local_submodule_error(target, "update")); + } + allow_file_submodule_transport(&mut cmd); + } + + cmd.arg("submodule") + .arg("update") + .arg("--init") + .arg("--") + .arg(&relative_path); + outputs.push(run_git_with_output( + cmd, + &format!("git submodule update --init -- {}", full_path.display()), + )?); + + if let Some(nested_repo) = open_gitlink_repo(repo, &relative_path)? { + update_repo_submodules_recursive( + &nested_repo, + trust_root, + &full_path, + approved_sources, + outputs, + )?; + } + } + + Ok(()) +} + fn configured_submodule_row( repo: &gix::Repository, submodule: gix::Submodule<'_>, @@ -161,16 +324,7 @@ fn configured_submodule_row( )); } - let state = submodule - .state() - .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule state: {e}"))))?; - let nested_repo = if state.repository_exists && state.worktree_checkout { - submodule - .open() - .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule open: {e}"))))? - } else { - None - }; + let nested_repo = open_configured_submodule_repo(&submodule)?; let Some(nested_repo) = nested_repo else { return Ok(( Submodule { @@ -248,6 +402,221 @@ fn open_gitlink_repo( } } +fn open_configured_submodule_repo(submodule: &gix::Submodule<'_>) -> Result> { + let state = submodule + .state() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule state: {e}"))))?; + if !(state.repository_exists && state.worktree_checkout) { + return Ok(None); + } + submodule + .open() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule open: {e}")))) +} + +fn trust_target_from_submodule( + current_repo_workdir: &Path, + full_submodule_path: &Path, + submodule: &gix::Submodule<'_>, +) -> Result> { + let url = submodule + .url() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule url: {e}"))))?; + trust_target_from_url(current_repo_workdir, full_submodule_path, &url) +} + +fn trust_target_from_raw_source( + current_repo_workdir: &Path, + submodule_path: &Path, + raw_source: &str, +) -> Result> { + let url = gix::url::parse(raw_source.as_bytes().as_bstr()).map_err(|e| { + Error::new(ErrorKind::Backend(format!( + "invalid submodule source {raw_source:?}: {e}" + ))) + })?; + let display_source = raw_source.trim().to_string(); + trust_target_from_parsed_url(current_repo_workdir, submodule_path, &url, display_source) +} + +fn trust_target_from_url( + current_repo_workdir: &Path, + submodule_path: &Path, + url: &gix::Url, +) -> Result> { + let display_source = bytes_to_text_preserving_utf8(url.to_bstring().as_ref()); + trust_target_from_parsed_url(current_repo_workdir, submodule_path, url, display_source) +} + +fn trust_target_from_parsed_url( + current_repo_workdir: &Path, + submodule_path: &Path, + url: &gix::Url, + display_source: String, +) -> Result> { + if url.scheme != gix::url::Scheme::File { + return Ok(None); + } + + let local_source_path = + canonicalize_or_original(resolve_local_file_transport_path(current_repo_workdir, url)?); + Ok(Some(SubmoduleTrustTarget { + submodule_path: submodule_path.to_path_buf(), + display_source, + local_source_path, + })) +} + +fn resolve_local_file_transport_path(current_repo_workdir: &Path, url: &gix::Url) -> Result { + let mut path = pathbuf_from_gix_path(url.path.as_ref())?; + if let Some(host) = url.host.as_deref() { + if !host.eq_ignore_ascii_case("localhost") { + let host_path = PathBuf::from(format!("//{host}")).join(&path); + path = host_path; + } + } + if path.is_relative() { + path = current_repo_workdir.join(path); + } + Ok(path) +} + +fn persist_submodule_trust_approvals( + trust_root: &Path, + approved_sources: &[SubmoduleTrustTarget], +) -> Result<()> { + for source in approved_sources { + let key = submodule_file_transport_consent_key(trust_root, &source.local_source_path); + if git_config_get_bool_global(trust_root, &key)?.unwrap_or(false) { + continue; + } + + let mut cmd = git_workdir_cmd_for(trust_root); + cmd.arg("config").arg("--global").arg(&key).arg("true"); + run_git_simple(cmd, &format!("git config --global {key} true"))?; + } + Ok(()) +} + +fn submodule_source_trusted( + trust_root: &Path, + source: &SubmoduleTrustTarget, +) -> Result { + let key = submodule_file_transport_consent_key(trust_root, &source.local_source_path); + Ok(git_config_get_bool_global(trust_root, &key)?.unwrap_or(false)) +} + +fn untrusted_local_submodule_error(source: &SubmoduleTrustTarget, action: &str) -> Error { + Error::new(ErrorKind::Backend(format!( + "Refusing to {action} local submodule '{}' from '{}'. Explicit trust is required before enabling file transport.", + source.submodule_path.display(), + source.display_source + ))) +} + +fn combine_submodule_update_outputs(outputs: Vec) -> CommandOutput { + CommandOutput { + command: "git submodule update --init --recursive".to_string(), + stdout: outputs + .iter() + .map(|output| output.stdout.trim_end()) + .filter(|text| !text.is_empty()) + .collect::>() + .join("\n"), + stderr: outputs + .iter() + .map(|output| output.stderr.trim_end()) + .filter(|text| !text.is_empty()) + .collect::>() + .join("\n"), + exit_code: Some(0), + } +} + +fn repo_workdir_for_submodule_trust(repo: &gix::Repository) -> &Path { + repo.workdir().unwrap_or_else(|| repo.git_dir()) +} + +fn submodule_file_transport_consent_key(trust_root: &Path, source_path: &Path) -> String { + let root = canonicalize_or_original(trust_root.to_path_buf()); + let source = canonicalize_or_original(source_path.to_path_buf()); + + let mut bytes = stable_path_bytes(&root); + bytes.push(0); + bytes.extend_from_slice(&stable_path_bytes(&source)); + format!("gitcomet.submodule.allowfiletransport-{:016x}", fnv1a_64(&bytes)) +} + +fn git_config_get_bool_global(trust_root: &Path, key: &str) -> Result> { + let mut cmd = git_workdir_cmd_for(trust_root); + cmd.arg("config") + .arg("--global") + .arg("--type=bool") + .arg("--get") + .arg(key); + + let output = cmd + .output() + .map_err(|err| Error::new(ErrorKind::Io(err.kind())))?; + + if output.status.success() { + let value = bytes_to_text_preserving_utf8(&output.stdout); + return match value.trim() { + "true" => Ok(Some(true)), + "false" => Ok(Some(false)), + other => Err(Error::new(ErrorKind::Backend(format!( + "Invalid boolean value for git config {key}: {:?}. Expected true or false.", + other + )))), + }; + } + + if output.status.code() == Some(1) { + return Ok(None); + } + + Err(Error::new(ErrorKind::Backend(format!( + "git config --global --type=bool --get {key} failed: {}", + bytes_to_text_preserving_utf8(&output.stderr).trim() + )))) +} + +fn stable_path_bytes(path: &Path) -> Vec { + #[cfg(unix)] + { + use std::os::unix::ffi::OsStrExt as _; + + path.as_os_str().as_bytes().to_vec() + } + + #[cfg(windows)] + { + use std::os::windows::ffi::OsStrExt as _; + + let mut bytes = Vec::new(); + for unit in path.as_os_str().encode_wide() { + bytes.extend_from_slice(&unit.to_le_bytes()); + } + bytes + } + + #[cfg(not(any(unix, windows)))] + { + path.to_str() + .map(|text| text.as_bytes().to_vec()) + .unwrap_or_else(|| format!("{path:?}").into_bytes()) + } +} + +fn fnv1a_64(bytes: &[u8]) -> u64 { + let mut hash = 0xcbf29ce484222325u64; + for byte in bytes { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x100000001b3); + } + hash +} + fn pathbuf_from_gix_path(path: &gix::bstr::BStr) -> Result { gix::path::try_from_bstr(path) .map(|path| path.into_owned()) @@ -261,7 +630,9 @@ fn object_id_to_commit_id(id: gix::ObjectId) -> CommitId { #[cfg(test)] mod tests { use super::allow_file_submodule_transport; + use super::submodule_file_transport_consent_key; use std::ffi::OsStr; + use std::path::Path; use std::process::Command; #[test] @@ -280,4 +651,23 @@ mod tests { .any(|(key, _)| key == OsStr::new("GIT_ALLOW_PROTOCOL")) ); } + + #[test] + fn consent_key_depends_on_root_and_source_path() { + let a = submodule_file_transport_consent_key( + Path::new("/repo-a"), + Path::new("/sources/local-one"), + ); + let b = submodule_file_transport_consent_key( + Path::new("/repo-a"), + Path::new("/sources/local-two"), + ); + let c = submodule_file_transport_consent_key( + Path::new("/repo-b"), + Path::new("/sources/local-one"), + ); + + assert_ne!(a, b); + assert_ne!(a, c); + } } diff --git a/crates/gitcomet-git-gix/tests/submodules_integration.rs b/crates/gitcomet-git-gix/tests/submodules_integration.rs index ffc6eb0b..9e0767fa 100644 --- a/crates/gitcomet-git-gix/tests/submodules_integration.rs +++ b/crates/gitcomet-git-gix/tests/submodules_integration.rs @@ -1,5 +1,5 @@ use gitcomet_core::domain::SubmoduleStatus; -use gitcomet_core::services::GitBackend; +use gitcomet_core::services::{GitBackend, SubmoduleTrustDecision}; use gitcomet_git_gix::GixBackend; #[path = "support/test_git_env.rs"] mod test_git_env; @@ -520,14 +520,24 @@ fn submodule_add_update_remove_round_trip() { init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); - run_git(&parent_repo, &["config", "protocol.file.allow", "always"]); let backend = GixBackend; let opened = backend.open(&parent_repo).expect("open parent repository"); let submodule_path = Path::new("mods/sub-one"); + let approved_sources = match opened + .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), submodule_path) + .expect("check local submodule trust") + { + SubmoduleTrustDecision::Prompt { sources } => sources, + other => panic!("expected trust prompt for local submodule, got {other:?}"), + }; let add_output = opened - .add_submodule_with_output(sub_repo.to_string_lossy().as_ref(), submodule_path) + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + &approved_sources, + ) .expect("add submodule"); assert_eq!(add_output.exit_code, Some(0)); @@ -551,8 +561,15 @@ fn submodule_add_update_remove_round_trip() { ); assert_eq!(listed[0].head.as_ref().len(), 40); + assert_eq!( + opened + .check_submodule_update_trust() + .expect("check update trust after approval"), + SubmoduleTrustDecision::Proceed + ); + let update_output = opened - .update_submodules_with_output() + .update_submodules_with_output(&[]) .expect("update submodules"); assert_eq!(update_output.exit_code, Some(0)); @@ -587,7 +604,7 @@ fn add_submodule_does_not_restrict_https_or_ssh_transports() { ("ssh://git@127.0.0.1:1/repo.git", "ssh"), ] { let err = opened - .add_submodule_with_output(url, Path::new("mods/sub-one")) + .add_submodule_with_output(url, Path::new("mods/sub-one"), &[]) .expect_err("dummy remote should fail without a reachable server"); let rendered = err.to_string(); assert!( @@ -596,3 +613,49 @@ fn add_submodule_does_not_restrict_https_or_ssh_transports() { ); } } + +#[test] +fn add_local_submodule_requires_explicit_trust() { + if !require_git_shell_for_submodule_tests() { + return; + } + let dir = tempfile::tempdir().expect("create tempdir"); + let root = dir.path(); + + let sub_repo = root.join("sub"); + let parent_repo = root.join("parent"); + fs::create_dir_all(&sub_repo).expect("create sub repository directory"); + fs::create_dir_all(&parent_repo).expect("create parent repository directory"); + + init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); + init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + + let backend = GixBackend; + let opened = backend.open(&parent_repo).expect("open parent repository"); + + let submodule_path = Path::new("mods/sub"); + let trust = opened + .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), submodule_path) + .expect("check local submodule trust"); + let approved_sources = match trust { + SubmoduleTrustDecision::Prompt { sources } => sources, + other => panic!("expected trust prompt for local submodule, got {other:?}"), + }; + + let err = opened + .add_submodule_with_output(sub_repo.to_string_lossy().as_ref(), submodule_path, &[]) + .expect_err("local submodule should fail without trust"); + assert!( + err.to_string().contains("Explicit trust is required"), + "unexpected error: {err}" + ); + + let add_output = opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + &approved_sources, + ) + .expect("add trusted local submodule"); + assert_eq!(add_output.exit_code, Some(0)); +} diff --git a/crates/gitcomet-git-gix/tests/support/test_git_env.rs b/crates/gitcomet-git-gix/tests/support/test_git_env.rs index a00923c7..a2e1ec18 100644 --- a/crates/gitcomet-git-gix/tests/support/test_git_env.rs +++ b/crates/gitcomet-git-gix/tests/support/test_git_env.rs @@ -10,6 +10,7 @@ struct IsolatedGitConfigEnv { home_dir: PathBuf, xdg_config_home: PathBuf, global_config: PathBuf, + gnupg_home: PathBuf, } fn isolated_git_config_env() -> &'static IsolatedGitConfigEnv { @@ -19,16 +20,34 @@ fn isolated_git_config_env() -> &'static IsolatedGitConfigEnv { let home_dir = root.path().join("home"); let xdg_config_home = root.path().join("xdg"); let global_config = root.path().join("global.gitconfig"); + let gnupg_home = root.path().join("gnupg"); fs::create_dir_all(&home_dir).expect("create isolated HOME directory"); fs::create_dir_all(&xdg_config_home).expect("create isolated XDG_CONFIG_HOME directory"); + fs::create_dir_all(&gnupg_home).expect("create isolated GNUPGHOME directory"); fs::write(&global_config, "").expect("create isolated global git config file"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + + fs::set_permissions(&gnupg_home, fs::Permissions::from_mode(0o700)) + .expect("set isolated GNUPGHOME permissions"); + } + + gitcomet_git_gix::install_test_git_command_environment( + global_config.clone(), + home_dir.clone(), + xdg_config_home.clone(), + gnupg_home.clone(), + ); + IsolatedGitConfigEnv { _root: root, home_dir, xdg_config_home, global_config, + gnupg_home, } }) } @@ -39,5 +58,6 @@ pub(crate) fn apply(cmd: &mut Command) { .env("GIT_CONFIG_GLOBAL", &env.global_config) .env("HOME", &env.home_dir) .env("XDG_CONFIG_HOME", &env.xdg_config_home) + .env("GNUPGHOME", &env.gnupg_home) .env_remove("GIT_CONFIG_SYSTEM"); } diff --git a/crates/gitcomet-git/src/noop_backend.rs b/crates/gitcomet-git/src/noop_backend.rs index 58c84c83..c0caa9a5 100644 --- a/crates/gitcomet-git/src/noop_backend.rs +++ b/crates/gitcomet-git/src/noop_backend.rs @@ -290,8 +290,10 @@ mod tests { assert_unsupported(repo.remove_worktree_with_output(path)); assert_unsupported(repo.force_remove_worktree_with_output(path)); assert_unsupported(repo.list_submodules()); - assert_unsupported(repo.add_submodule_with_output("https://example.com/repo.git", path)); - assert_unsupported(repo.update_submodules_with_output()); + assert_unsupported(repo.check_submodule_add_trust("https://example.com/repo.git", path)); + assert_unsupported(repo.check_submodule_update_trust()); + assert_unsupported(repo.add_submodule_with_output("https://example.com/repo.git", path, &[])); + assert_unsupported(repo.update_submodules_with_output(&[])); assert_unsupported(repo.remove_submodule_with_output(path)); } } diff --git a/crates/gitcomet-state/src/model.rs b/crates/gitcomet-state/src/model.rs index 39d692b5..a7b80a34 100644 --- a/crates/gitcomet-state/src/model.rs +++ b/crates/gitcomet-state/src/model.rs @@ -5,7 +5,7 @@ use gitcomet_core::conflict_session::{ ConflictPayload, ConflictSession, ConflictStageParts, canonicalize_stage_parts, }; use gitcomet_core::domain::*; -use gitcomet_core::services::BlameLine; +use gitcomet_core::services::{BlameLine, SubmoduleTrustTarget}; use std::collections::VecDeque; use std::path::PathBuf; use std::sync::Arc; @@ -230,6 +230,7 @@ pub struct AppState { pub notifications: Vec, pub banner_error: Option, pub auth_prompt: Option, + pub submodule_trust_prompt: Option, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -275,6 +276,19 @@ pub struct AuthPromptState { pub operation: AuthRetryOperation, } +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum SubmoduleTrustPromptOperation { + Add { url: String, path: PathBuf }, + Update, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct SubmoduleTrustPromptState { + pub repo_id: RepoId, + pub operation: SubmoduleTrustPromptOperation, + pub sources: Vec, +} + #[derive(Clone, Debug, Eq, PartialEq)] pub struct AppNotification { pub time: SystemTime, diff --git a/crates/gitcomet-state/src/msg/effect.rs b/crates/gitcomet-state/src/msg/effect.rs index 98db4b43..cb494313 100644 --- a/crates/gitcomet-state/src/msg/effect.rs +++ b/crates/gitcomet-state/src/msg/effect.rs @@ -1,7 +1,9 @@ use crate::model::{ConflictFileLoadMode, RepoId}; use gitcomet_core::auth::StagedGitAuth; use gitcomet_core::domain::*; -use gitcomet_core::services::{ConflictSide, PullMode, RemoteUrlKind, ResetMode}; +use gitcomet_core::services::{ + ConflictSide, PullMode, RemoteUrlKind, ResetMode, SubmoduleTrustTarget, +}; use std::path::PathBuf; use super::RepoPathList; @@ -183,14 +185,24 @@ pub enum Effect { repo_id: RepoId, path: PathBuf, }, + CheckSubmoduleAddTrust { + repo_id: RepoId, + url: String, + path: PathBuf, + }, + CheckSubmoduleUpdateTrust { + repo_id: RepoId, + }, AddSubmodule { repo_id: RepoId, url: String, path: PathBuf, + approved_sources: Vec, auth: Option, }, UpdateSubmodules { repo_id: RepoId, + approved_sources: Vec, auth: Option, }, RemoveSubmodule { diff --git a/crates/gitcomet-state/src/msg/message.rs b/crates/gitcomet-state/src/msg/message.rs index fecf71ea..0c3e979b 100644 --- a/crates/gitcomet-state/src/msg/message.rs +++ b/crates/gitcomet-state/src/msg/message.rs @@ -3,7 +3,10 @@ use gitcomet_core::conflict_session::ConflictSession; use gitcomet_core::domain::*; use gitcomet_core::error::Error; use gitcomet_core::services::GitRepository; -use gitcomet_core::services::{CommandOutput, ConflictSide, PullMode, RemoteUrlKind, ResetMode}; +use gitcomet_core::services::{ + CommandOutput, ConflictSide, PullMode, RemoteUrlKind, ResetMode, SubmoduleTrustDecision, + SubmoduleTrustTarget, +}; use std::path::PathBuf; use std::sync::Arc; @@ -245,9 +248,21 @@ pub enum Msg { url: String, path: PathBuf, }, + AddSubmoduleTrusted { + repo_id: RepoId, + url: String, + path: PathBuf, + approved_sources: Vec, + }, UpdateSubmodules { repo_id: RepoId, }, + UpdateSubmodulesTrusted { + repo_id: RepoId, + approved_sources: Vec, + }, + ConfirmSubmoduleTrustPrompt, + CancelSubmoduleTrustPrompt, RemoveSubmodule { repo_id: RepoId, path: PathBuf, @@ -575,6 +590,16 @@ pub enum InternalMsg { repo_id: RepoId, result: Result, Error>, }, + SubmoduleAddTrustChecked { + repo_id: RepoId, + url: String, + path: PathBuf, + result: Result, + }, + SubmoduleUpdateTrustChecked { + repo_id: RepoId, + result: Result, + }, CommitDetailsLoaded { repo_id: RepoId, commit_id: CommitId, diff --git a/crates/gitcomet-state/src/msg/message_debug.rs b/crates/gitcomet-state/src/msg/message_debug.rs index 497d9cc7..7a269708 100644 --- a/crates/gitcomet-state/src/msg/message_debug.rs +++ b/crates/gitcomet-state/src/msg/message_debug.rs @@ -156,6 +156,23 @@ impl std::fmt::Debug for InternalMsg { .field("repo_id", repo_id) .field("result", result) .finish(), + InternalMsg::SubmoduleAddTrustChecked { + repo_id, + url, + path, + result, + } => f + .debug_struct("SubmoduleAddTrustChecked") + .field("repo_id", repo_id) + .field("url", url) + .field("path", path) + .field("result", result) + .finish(), + InternalMsg::SubmoduleUpdateTrustChecked { repo_id, result } => f + .debug_struct("SubmoduleUpdateTrustChecked") + .field("repo_id", repo_id) + .field("result", result) + .finish(), InternalMsg::CommitDetailsLoaded { repo_id, commit_id, diff --git a/crates/gitcomet-state/src/msg/repo_command_kind.rs b/crates/gitcomet-state/src/msg/repo_command_kind.rs index d268a7de..13e35435 100644 --- a/crates/gitcomet-state/src/msg/repo_command_kind.rs +++ b/crates/gitcomet-state/src/msg/repo_command_kind.rs @@ -1,5 +1,7 @@ use gitcomet_core::domain::CommitId; -use gitcomet_core::services::{ConflictSide, PullMode, RemoteUrlKind, ResetMode}; +use gitcomet_core::services::{ + ConflictSide, PullMode, RemoteUrlKind, ResetMode, SubmoduleTrustTarget, +}; use std::path::PathBuf; #[derive(Clone, Debug, Eq, PartialEq)] @@ -111,8 +113,11 @@ pub enum RepoCommandKind { AddSubmodule { url: String, path: PathBuf, + approved_sources: Vec, + }, + UpdateSubmodules { + approved_sources: Vec, }, - UpdateSubmodules, RemoveSubmodule { path: PathBuf, }, diff --git a/crates/gitcomet-state/src/store/effects.rs b/crates/gitcomet-state/src/store/effects.rs index 0c8ae30f..77df2027 100644 --- a/crates/gitcomet-state/src/store/effects.rs +++ b/crates/gitcomet-state/src/store/effects.rs @@ -255,18 +255,38 @@ pub(super) fn schedule_effect( Effect::ForceRemoveWorktree { repo_id, path } => { repo_commands::schedule_force_remove_worktree(executor, repos, msg_tx, repo_id, path); } + Effect::CheckSubmoduleAddTrust { repo_id, url, path } => { + repo_commands::schedule_check_submodule_add_trust( + executor, repos, msg_tx, repo_id, url, path, + ); + } + Effect::CheckSubmoduleUpdateTrust { repo_id } => { + repo_commands::schedule_check_submodule_update_trust(executor, repos, msg_tx, repo_id); + } Effect::AddSubmodule { repo_id, url, path, + approved_sources, auth, } => { repo_commands::schedule_add_submodule( - executor, repos, msg_tx, repo_id, url, path, auth, + executor, repos, msg_tx, repo_id, url, path, approved_sources, auth, ); } - Effect::UpdateSubmodules { repo_id, auth } => { - repo_commands::schedule_update_submodules(executor, repos, msg_tx, repo_id, auth); + Effect::UpdateSubmodules { + repo_id, + approved_sources, + auth, + } => { + repo_commands::schedule_update_submodules( + executor, + repos, + msg_tx, + repo_id, + approved_sources, + auth, + ); } Effect::RemoveSubmodule { repo_id, path } => { repo_commands::schedule_remove_submodule(executor, repos, msg_tx, repo_id, path); diff --git a/crates/gitcomet-state/src/store/effects/repo_commands.rs b/crates/gitcomet-state/src/store/effects/repo_commands.rs index 435e956d..95ffb7d5 100644 --- a/crates/gitcomet-state/src/store/effects/repo_commands.rs +++ b/crates/gitcomet-state/src/store/effects/repo_commands.rs @@ -5,6 +5,7 @@ use gitcomet_core::auth::{ use gitcomet_core::error::{Error, ErrorKind}; use gitcomet_core::services::{ CommandOutput, ConflictSide, GitRepository, PullMode, RemoteUrlKind, ResetMode, + SubmoduleTrustTarget, }; use std::path::{Component, Path, PathBuf}; use std::sync::{Arc, mpsc}; @@ -232,10 +233,12 @@ pub(super) fn schedule_add_submodule( repo_id: RepoId, url: String, path: PathBuf, + approved_sources: Vec, auth: Option, ) { let command_url = url.clone(); let command_path = path.clone(); + let command_sources = approved_sources.clone(); schedule_repo_command( executor, repos, @@ -244,8 +247,13 @@ pub(super) fn schedule_add_submodule( RepoCommandKind::AddSubmodule { url: command_url, path: command_path, + approved_sources: command_sources, + }, + move |repo| { + run_with_git_auth(auth, || { + repo.add_submodule_with_output(&url, &path, &approved_sources) + }) }, - move |repo| run_with_git_auth(auth, || repo.add_submodule_with_output(&url, &path)), ); } @@ -254,18 +262,61 @@ pub(super) fn schedule_update_submodules( repos: &RepoMap, msg_tx: mpsc::Sender, repo_id: RepoId, + approved_sources: Vec, auth: Option, ) { + let command_sources = approved_sources.clone(); schedule_repo_command( executor, repos, msg_tx, repo_id, - RepoCommandKind::UpdateSubmodules, - move |repo| run_with_git_auth(auth, || repo.update_submodules_with_output()), + RepoCommandKind::UpdateSubmodules { + approved_sources: command_sources, + }, + move |repo| { + run_with_git_auth(auth, || repo.update_submodules_with_output(&approved_sources)) + }, ); } +pub(super) fn schedule_check_submodule_add_trust( + executor: &TaskExecutor, + repos: &RepoMap, + msg_tx: mpsc::Sender, + repo_id: RepoId, + url: String, + path: PathBuf, +) { + spawn_with_repo(executor, repos, repo_id, msg_tx, move |repo, msg_tx| { + let result = repo.check_submodule_add_trust(&url, &path); + send_or_log( + &msg_tx, + Msg::Internal(crate::msg::InternalMsg::SubmoduleAddTrustChecked { + repo_id, + url, + path, + result, + }), + ); + }); +} + +pub(super) fn schedule_check_submodule_update_trust( + executor: &TaskExecutor, + repos: &RepoMap, + msg_tx: mpsc::Sender, + repo_id: RepoId, +) { + spawn_with_repo(executor, repos, repo_id, msg_tx, move |repo, msg_tx| { + let result = repo.check_submodule_update_trust(); + send_or_log( + &msg_tx, + Msg::Internal(crate::msg::InternalMsg::SubmoduleUpdateTrustChecked { repo_id, result }), + ); + }); +} + pub(super) fn schedule_remove_submodule( executor: &TaskExecutor, repos: &RepoMap, diff --git a/crates/gitcomet-state/src/store/reducer.rs b/crates/gitcomet-state/src/store/reducer.rs index 7e460afd..3dc922de 100644 --- a/crates/gitcomet-state/src/store/reducer.rs +++ b/crates/gitcomet-state/src/store/reducer.rs @@ -8,6 +8,7 @@ mod util; use crate::model::{ AppState, AuthPromptState, AuthRetryOperation, BannerErrorState, PendingCommitRetry, RepoId, + SubmoduleTrustPromptOperation, SubmoduleTrustPromptState, }; use crate::msg::{ConflictRegionChoice, Effect, Msg, RepoCommandKind, RepoPath, RepoPathList}; use gitcomet_core::auth::StagedGitAuth; @@ -249,8 +250,20 @@ fn retry_msg_for_repo_command(repo_id: RepoId, command: RepoCommandKind) -> Opti }, RepoCommandKind::RemoveWorktree { path } => Msg::RemoveWorktree { repo_id, path }, RepoCommandKind::ForceRemoveWorktree { path } => Msg::ForceRemoveWorktree { repo_id, path }, - RepoCommandKind::AddSubmodule { url, path } => Msg::AddSubmodule { repo_id, url, path }, - RepoCommandKind::UpdateSubmodules => Msg::UpdateSubmodules { repo_id }, + RepoCommandKind::AddSubmodule { + url, + path, + approved_sources, + } => Msg::AddSubmoduleTrusted { + repo_id, + url, + path, + approved_sources, + }, + RepoCommandKind::UpdateSubmodules { approved_sources } => Msg::UpdateSubmodulesTrusted { + repo_id, + approved_sources, + }, RepoCommandKind::RemoveSubmodule { path } => Msg::RemoveSubmodule { repo_id, path }, // Not replayable because command metadata does not retain original content. RepoCommandKind::SaveWorktreeFile { .. } @@ -652,12 +665,47 @@ pub(super) fn reduce( actions_emit_effects::force_remove_worktree(repo_id, normalized_path) } Msg::AddSubmodule { repo_id, url, path } => { + state.submodule_trust_prompt = None; + vec![Effect::CheckSubmoduleAddTrust { repo_id, url, path }] + } + Msg::AddSubmoduleTrusted { + repo_id, + url, + path, + approved_sources, + } => { begin_local_action(state, repo_id); - actions_emit_effects::add_submodule(repo_id, url, path) + actions_emit_effects::add_submodule(repo_id, url, path, approved_sources) } Msg::UpdateSubmodules { repo_id } => { + state.submodule_trust_prompt = None; + vec![Effect::CheckSubmoduleUpdateTrust { repo_id }] + } + Msg::UpdateSubmodulesTrusted { + repo_id, + approved_sources, + } => { begin_local_action(state, repo_id); - actions_emit_effects::update_submodules(repo_id) + actions_emit_effects::update_submodules(repo_id, approved_sources) + } + Msg::ConfirmSubmoduleTrustPrompt => { + let Some(prompt) = state.submodule_trust_prompt.take() else { + return Vec::new(); + }; + match prompt.operation { + SubmoduleTrustPromptOperation::Add { url, path } => { + begin_local_action(state, prompt.repo_id); + actions_emit_effects::add_submodule(prompt.repo_id, url, path, prompt.sources) + } + SubmoduleTrustPromptOperation::Update => { + begin_local_action(state, prompt.repo_id); + actions_emit_effects::update_submodules(prompt.repo_id, prompt.sources) + } + } + } + Msg::CancelSubmoduleTrustPrompt => { + state.submodule_trust_prompt = None; + Vec::new() } Msg::RemoveSubmodule { repo_id, path } => { begin_local_action(state, repo_id); @@ -1000,6 +1048,55 @@ pub(super) fn reduce( Msg::Internal(crate::msg::InternalMsg::SubmodulesLoaded { repo_id, result }) => { effects::submodules_loaded(state, repo_id, result) } + Msg::Internal(crate::msg::InternalMsg::SubmoduleAddTrustChecked { + repo_id, + url, + path, + result, + }) => match result { + Ok(gitcomet_core::services::SubmoduleTrustDecision::Proceed) => { + begin_local_action(state, repo_id); + actions_emit_effects::add_submodule(repo_id, url, path, Vec::new()) + } + Ok(gitcomet_core::services::SubmoduleTrustDecision::Prompt { sources }) => { + state.submodule_trust_prompt = Some(SubmoduleTrustPromptState { + repo_id, + operation: SubmoduleTrustPromptOperation::Add { url, path }, + sources, + }); + Vec::new() + } + Err(error) => { + state.banner_error = Some(BannerErrorState { + repo_id: Some(repo_id), + message: util::format_failure_summary("Submodule trust check", &error), + }); + Vec::new() + } + }, + Msg::Internal(crate::msg::InternalMsg::SubmoduleUpdateTrustChecked { repo_id, result }) => { + match result { + Ok(gitcomet_core::services::SubmoduleTrustDecision::Proceed) => { + begin_local_action(state, repo_id); + actions_emit_effects::update_submodules(repo_id, Vec::new()) + } + Ok(gitcomet_core::services::SubmoduleTrustDecision::Prompt { sources }) => { + state.submodule_trust_prompt = Some(SubmoduleTrustPromptState { + repo_id, + operation: SubmoduleTrustPromptOperation::Update, + sources, + }); + Vec::new() + } + Err(error) => { + state.banner_error = Some(BannerErrorState { + repo_id: Some(repo_id), + message: util::format_failure_summary("Submodule trust check", &error), + }); + Vec::new() + } + } + } Msg::Internal(crate::msg::InternalMsg::CommitDetailsLoaded { repo_id, commit_id, diff --git a/crates/gitcomet-state/src/store/reducer/actions_emit_effects.rs b/crates/gitcomet-state/src/store/reducer/actions_emit_effects.rs index 460e9b6e..6932f3ec 100644 --- a/crates/gitcomet-state/src/store/reducer/actions_emit_effects.rs +++ b/crates/gitcomet-state/src/store/reducer/actions_emit_effects.rs @@ -117,18 +117,28 @@ pub(super) fn force_remove_worktree(repo_id: RepoId, path: PathBuf) -> Vec Vec { +pub(super) fn add_submodule( + repo_id: RepoId, + url: String, + path: PathBuf, + approved_sources: Vec, +) -> Vec { vec![Effect::AddSubmodule { repo_id, url, path, + approved_sources, auth: None, }] } -pub(super) fn update_submodules(repo_id: RepoId) -> Vec { +pub(super) fn update_submodules( + repo_id: RepoId, + approved_sources: Vec, +) -> Vec { vec![Effect::UpdateSubmodules { repo_id, + approved_sources, auth: None, }] } @@ -621,7 +631,7 @@ fn tracks_local_actions_in_flight(command: &RepoCommandKind) -> bool { | RepoCommandKind::ExportPatch { .. } | RepoCommandKind::ApplyPatch { .. } | RepoCommandKind::AddSubmodule { .. } - | RepoCommandKind::UpdateSubmodules + | RepoCommandKind::UpdateSubmodules { .. } | RepoCommandKind::RemoveSubmodule { .. } | RepoCommandKind::StageHunk | RepoCommandKind::UnstageHunk @@ -644,7 +654,7 @@ pub(super) fn repo_command_finished( let refresh_submodules = matches!( &command, RepoCommandKind::AddSubmodule { .. } - | RepoCommandKind::UpdateSubmodules + | RepoCommandKind::UpdateSubmodules { .. } | RepoCommandKind::RemoveSubmodule { .. } ) && result.is_ok(); let command_succeeded = result.is_ok(); diff --git a/crates/gitcomet-state/src/store/reducer/util.rs b/crates/gitcomet-state/src/store/reducer/util.rs index a8097ecc..f2424405 100644 --- a/crates/gitcomet-state/src/store/reducer/util.rs +++ b/crates/gitcomet-state/src/store/reducer/util.rs @@ -705,7 +705,7 @@ fn summarize_command( | RepoCommandKind::RemoveWorktree { .. } | RepoCommandKind::ForceRemoveWorktree { .. } => "Worktree", RepoCommandKind::AddSubmodule { .. } - | RepoCommandKind::UpdateSubmodules + | RepoCommandKind::UpdateSubmodules { .. } | RepoCommandKind::RemoveSubmodule { .. } => "Submodule", RepoCommandKind::StageHunk | RepoCommandKind::UnstageHunk => "Hunk", RepoCommandKind::ApplyWorktreePatch { reverse } => { @@ -897,7 +897,7 @@ fn summarize_command( RepoCommandKind::AddSubmodule { path, .. } => { format!("Submodule added → {}", path.display()) } - RepoCommandKind::UpdateSubmodules => "Submodules: Updated".to_string(), + RepoCommandKind::UpdateSubmodules { .. } => "Submodules: Updated".to_string(), RepoCommandKind::RemoveSubmodule { path } => { format!("Submodule removed → {}", path.display()) } diff --git a/crates/gitcomet-state/src/store/tests/actions_emit_effects.rs b/crates/gitcomet-state/src/store/tests/actions_emit_effects.rs index a3055600..604e8e9c 100644 --- a/crates/gitcomet-state/src/store/tests/actions_emit_effects.rs +++ b/crates/gitcomet-state/src/store/tests/actions_emit_effects.rs @@ -652,6 +652,7 @@ fn submodule_commands_reload_submodules_on_success() { command: RepoCommandKind::AddSubmodule { url: "https://example.com/sub.git".to_string(), path: PathBuf::from("submodule"), + approved_sources: Vec::new(), }, result: Ok(CommandOutput::empty_success("git submodule add")), }), @@ -671,7 +672,9 @@ fn submodule_commands_reload_submodules_on_success() { &mut state, Msg::Internal(crate::msg::InternalMsg::RepoCommandFinished { repo_id, - command: RepoCommandKind::UpdateSubmodules, + command: RepoCommandKind::UpdateSubmodules { + approved_sources: Vec::new(), + }, result: Ok(CommandOutput::empty_success( "git submodule update --init --recursive", )), @@ -2214,10 +2217,16 @@ fn repo_command_finished_error_summaries_cover_additional_labels() { RepoCommandKind::AddSubmodule { url: "https://example.com/sub.git".to_string(), path: PathBuf::from("mods/sub"), + approved_sources: Vec::new(), + }, + "Submodule", + ), + ( + RepoCommandKind::UpdateSubmodules { + approved_sources: Vec::new(), }, "Submodule", ), - (RepoCommandKind::UpdateSubmodules, "Submodule"), ( RepoCommandKind::RemoveSubmodule { path: PathBuf::from("mods/sub"), @@ -2327,10 +2336,16 @@ fn repo_command_finished_success_summaries_cover_additional_commands() { RepoCommandKind::AddSubmodule { url: "https://example.com/sub.git".to_string(), path: PathBuf::from("mods/sub"), + approved_sources: Vec::new(), }, "Submodule added → mods/sub", ), - (RepoCommandKind::UpdateSubmodules, "Submodules: Updated"), + ( + RepoCommandKind::UpdateSubmodules { + approved_sources: Vec::new(), + }, + "Submodules: Updated", + ), ( RepoCommandKind::RemoveSubmodule { path: PathBuf::from("mods/sub"), @@ -2485,11 +2500,11 @@ fn checkout_branch_and_submodule_messages_emit_effects() { ); assert!(matches!( add_submodule.as_slice(), - [Effect::AddSubmodule { + [Effect::CheckSubmoduleAddTrust { repo_id: RepoId(1), + url, path, - .. - }] if path == &PathBuf::from("mods/sub") + }] if url == "https://example.com/sub.git" && path == &PathBuf::from("mods/sub") )); let update_submodules = reduce( @@ -2500,10 +2515,7 @@ fn checkout_branch_and_submodule_messages_emit_effects() { ); assert!(matches!( update_submodules.as_slice(), - [Effect::UpdateSubmodules { - repo_id: RepoId(1), - .. - }] + [Effect::CheckSubmoduleUpdateTrust { repo_id: RepoId(1) }] )); let remove_submodule = reduce( @@ -2524,6 +2536,121 @@ fn checkout_branch_and_submodule_messages_emit_effects() { )); } +#[test] +fn local_submodule_add_trust_prompt_confirms_into_add_effect() { + let mut repos: HashMap> = HashMap::default(); + let id_alloc = AtomicU64::new(1); + let mut state = AppState::default(); + let repo_id = RepoId(1); + state.repos.push(RepoState::new_opening( + repo_id, + RepoSpec { + workdir: PathBuf::from("/tmp/repo"), + }, + )); + + let source = gitcomet_core::services::SubmoduleTrustTarget { + submodule_path: PathBuf::from("mods/sub"), + display_source: "../local-sub".to_string(), + local_source_path: PathBuf::from("/tmp/local-sub"), + }; + + let effects = reduce( + &mut repos, + &id_alloc, + &mut state, + Msg::Internal(crate::msg::InternalMsg::SubmoduleAddTrustChecked { + repo_id, + url: "../local-sub".to_string(), + path: PathBuf::from("mods/sub"), + result: Ok(gitcomet_core::services::SubmoduleTrustDecision::Prompt { + sources: vec![source.clone()], + }), + }), + ); + assert!(effects.is_empty()); + assert_eq!( + state.submodule_trust_prompt, + Some(crate::model::SubmoduleTrustPromptState { + repo_id, + operation: crate::model::SubmoduleTrustPromptOperation::Add { + url: "../local-sub".to_string(), + path: PathBuf::from("mods/sub"), + }, + sources: vec![source.clone()], + }) + ); + + let effects = reduce( + &mut repos, + &id_alloc, + &mut state, + Msg::ConfirmSubmoduleTrustPrompt, + ); + assert!(state.submodule_trust_prompt.is_none()); + assert!(matches!( + effects.as_slice(), + [Effect::AddSubmodule { + repo_id: RepoId(1), + url, + path, + approved_sources, + .. + }] if url == "../local-sub" + && path == &PathBuf::from("mods/sub") + && approved_sources == &vec![source] + )); +} + +#[test] +fn local_submodule_update_trust_prompt_cancels_cleanly() { + let mut repos: HashMap> = HashMap::default(); + let id_alloc = AtomicU64::new(1); + let mut state = AppState::default(); + let repo_id = RepoId(1); + state.repos.push(RepoState::new_opening( + repo_id, + RepoSpec { + workdir: PathBuf::from("/tmp/repo"), + }, + )); + + let source = gitcomet_core::services::SubmoduleTrustTarget { + submodule_path: PathBuf::from("mods/sub"), + display_source: "../local-sub".to_string(), + local_source_path: PathBuf::from("/tmp/local-sub"), + }; + + reduce( + &mut repos, + &id_alloc, + &mut state, + Msg::Internal(crate::msg::InternalMsg::SubmoduleUpdateTrustChecked { + repo_id, + result: Ok(gitcomet_core::services::SubmoduleTrustDecision::Prompt { + sources: vec![source], + }), + }), + ); + assert!(matches!( + state.submodule_trust_prompt, + Some(crate::model::SubmoduleTrustPromptState { + repo_id: RepoId(1), + operation: crate::model::SubmoduleTrustPromptOperation::Update, + .. + }) + )); + + let effects = reduce( + &mut repos, + &id_alloc, + &mut state, + Msg::CancelSubmoduleTrustPrompt, + ); + assert!(effects.is_empty()); + assert!(state.submodule_trust_prompt.is_none()); +} + #[test] fn pull_branch_and_push_variants_mark_in_flight_when_repo_is_opened() { let mut repos: HashMap> = HashMap::default(); diff --git a/crates/gitcomet-state/src/store/tests/effects.rs b/crates/gitcomet-state/src/store/tests/effects.rs index cfbe541f..d60683ec 100644 --- a/crates/gitcomet-state/src/store/tests/effects.rs +++ b/crates/gitcomet-state/src/store/tests/effects.rs @@ -3291,12 +3291,14 @@ fn schedule_effect_dispatches_many_variants_with_repo_present() { repo_id, url: "https://example.com/repo.git".to_string(), path: PathBuf::from("sub"), + approved_sources: Vec::new(), auth: None, }, 1, ), ( Effect::UpdateSubmodules { + approved_sources: Vec::new(), repo_id, auth: None, }, diff --git a/crates/gitcomet-ui-gpui/src/view/mod.rs b/crates/gitcomet-ui-gpui/src/view/mod.rs index 5461da3a..82ad06ab 100644 --- a/crates/gitcomet-ui-gpui/src/view/mod.rs +++ b/crates/gitcomet-ui-gpui/src/view/mod.rs @@ -8,7 +8,7 @@ use gitcomet_core::file_diff::FileDiffRow; use gitcomet_core::services::{PullMode, RemoteUrlKind, ResetMode}; use gitcomet_state::model::{ AppNotificationKind, AppState, AuthPromptKind, CloneOpState, CloneOpStatus, DiagnosticKind, - Loadable, RepoId, RepoState, + Loadable, RepoId, RepoState, SubmoduleTrustPromptOperation, }; use gitcomet_state::msg::{Msg, RepoExternalChange, StoreEvent}; use gitcomet_state::session; @@ -745,6 +745,7 @@ impl GitCometView { pending_pull_reconcile_prompt: None, pending_force_delete_branch_prompt: None, pending_force_remove_worktree_prompt: None, + pending_submodule_trust_prompt: None, pending_worktree_branch_removals: HashMap::default(), startup_crash_report, #[cfg(target_os = "macos")] @@ -1434,6 +1435,17 @@ impl Render for GitCometView { ); } + if let Some(prompt) = self.pending_submodule_trust_prompt.take() + && self.active_repo_id() == Some(prompt.repo_id) + { + self.open_popover_at( + PopoverKind::submodule(prompt.repo_id, SubmodulePopoverKind::TrustConfirm), + self.last_mouse_pos, + window, + cx, + ); + } + let decorations = window.window_decorations(); let (tiling, client_inset) = match decorations { Decorations::Client { tiling } => (Some(tiling), CLIENT_SIDE_DECORATION_INSET), diff --git a/crates/gitcomet-ui-gpui/src/view/mod_helpers.rs b/crates/gitcomet-ui-gpui/src/view/mod_helpers.rs index 6fbc1344..5bbaceff 100644 --- a/crates/gitcomet-ui-gpui/src/view/mod_helpers.rs +++ b/crates/gitcomet-ui-gpui/src/view/mod_helpers.rs @@ -2167,6 +2167,7 @@ pub(super) enum SubmodulePopoverKind { SectionMenu, Menu { path: std::path::PathBuf }, AddPrompt, + TrustConfirm, OpenPicker, RemovePicker, RemoveConfirm { path: std::path::PathBuf }, @@ -2606,6 +2607,8 @@ pub struct GitCometView { pub(super) pending_force_delete_branch_prompt: Option<(RepoId, String)>, pub(super) pending_force_remove_worktree_prompt: Option<(RepoId, std::path::PathBuf, Option)>, + pub(super) pending_submodule_trust_prompt: + Option, pub(super) pending_worktree_branch_removals: HashMap<(RepoId, std::path::PathBuf), String>, pub(super) startup_crash_report: Option, #[cfg(target_os = "macos")] diff --git a/crates/gitcomet-ui-gpui/src/view/panels/popover.rs b/crates/gitcomet-ui-gpui/src/view/panels/popover.rs index 81fffceb..6f39c2e4 100644 --- a/crates/gitcomet-ui-gpui/src/view/panels/popover.rs +++ b/crates/gitcomet-ui-gpui/src/view/panels/popover.rs @@ -30,6 +30,7 @@ mod search_inputs; mod stash_drop_confirm; mod stash_prompt; mod submodule_add_prompt; +mod submodule_trust_confirm; mod submodule_open_picker; mod submodule_remove_confirm; mod submodule_remove_picker; @@ -1040,6 +1041,10 @@ impl PopoverHost { .read_with(cx, |i, _| i.focus_handle()); window.focus(&focus); } + PopoverKind::Repo { + kind: RepoPopoverKind::Submodule(SubmodulePopoverKind::TrustConfirm), + .. + } => {} PopoverKind::Repo { repo_id, kind: @@ -1348,6 +1353,7 @@ impl PopoverHost { kind: RepoPopoverKind::Submodule( SubmodulePopoverKind::AddPrompt + | SubmodulePopoverKind::TrustConfirm | SubmodulePopoverKind::OpenPicker | SubmodulePopoverKind::RemovePicker | SubmodulePopoverKind::RemoveConfirm { .. }, @@ -1496,6 +1502,9 @@ impl PopoverHost { SubmodulePopoverKind::AddPrompt => { submodule_add_prompt::panel(self, repo_id, cx) } + SubmodulePopoverKind::TrustConfirm => { + submodule_trust_confirm::panel(self, repo_id, cx) + } SubmodulePopoverKind::OpenPicker => { submodule_open_picker::panel(self, repo_id, cx) } diff --git a/crates/gitcomet-ui-gpui/src/view/panels/popover/fingerprint.rs b/crates/gitcomet-ui-gpui/src/view/panels/popover/fingerprint.rs index 524293aa..8a2d26ae 100644 --- a/crates/gitcomet-ui-gpui/src/view/panels/popover/fingerprint.rs +++ b/crates/gitcomet-ui-gpui/src/view/panels/popover/fingerprint.rs @@ -48,6 +48,34 @@ pub(super) fn notify_fingerprint(state: &AppState, popover: &PopoverKind) -> u64 view_fingerprint::hash_loadable_kind(&repo.open, &mut hasher); } } + PopoverKind::Repo { + kind: RepoPopoverKind::Submodule(SubmodulePopoverKind::TrustConfirm), + .. + } => { + if let Some(repo) = repo_for_popover(state, popover) { + hash_repo_for_popover(repo, popover, &mut hasher); + } else { + state.active_repo.hash(&mut hasher); + } + if let Some(prompt) = state.submodule_trust_prompt.as_ref() { + prompt.repo_id.hash(&mut hasher); + match &prompt.operation { + SubmoduleTrustPromptOperation::Add { url, path } => { + 0u8.hash(&mut hasher); + url.hash(&mut hasher); + path.hash(&mut hasher); + } + SubmoduleTrustPromptOperation::Update => { + 1u8.hash(&mut hasher); + } + } + for source in &prompt.sources { + source.submodule_path.hash(&mut hasher); + source.display_source.hash(&mut hasher); + source.local_source_path.hash(&mut hasher); + } + } + } _ => { if let Some(repo) = repo_for_popover(state, popover) { hash_repo_for_popover(repo, popover, &mut hasher); @@ -546,6 +574,10 @@ fn hash_repo_popover_kind(repo_id: RepoId, kind: &RepoPopoverKind, ha 24u8.hash(hasher); repo_id.hash(hasher); } + SubmodulePopoverKind::TrustConfirm => { + 28u8.hash(hasher); + repo_id.hash(hasher); + } SubmodulePopoverKind::OpenPicker => { 25u8.hash(hasher); repo_id.hash(hasher); diff --git a/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs b/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs new file mode 100644 index 00000000..df32796e --- /dev/null +++ b/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs @@ -0,0 +1,159 @@ +use super::*; + +const SUBMODULE_TRUST_CVE_URL: &str = + "https://github.blog/open-source/git/git-security-vulnerabilities-announced/#cve-2022-39253"; + +pub(super) fn panel( + this: &mut PopoverHost, + repo_id: RepoId, + cx: &mut gpui::Context, +) -> gpui::Div { + let theme = this.theme; + let Some(prompt) = this + .state + .submodule_trust_prompt + .as_ref() + .filter(|prompt| prompt.repo_id == repo_id) + .cloned() + else { + return div(); + }; + + let (title, confirm_label, cancel_label) = match &prompt.operation { + SubmoduleTrustPromptOperation::Add { .. } => { + ("Trust local submodule?", "Trust and add", "Back") + } + SubmoduleTrustPromptOperation::Update => { + ("Trust local submodule sources?", "Trust and update", "Cancel") + } + }; + let sources = prompt.sources.clone(); + let operation = prompt.operation.clone(); + + div() + .flex() + .flex_col() + .w(px(640.0)) + .child( + div() + .px_2() + .py_1() + .text_sm() + .font_weight(FontWeight::BOLD) + .child(title), + ) + .child(div().border_t_1().border_color(theme.colors.border)) + .child( + div() + .px_2() + .pt_1() + .text_sm() + .text_color(theme.colors.text_muted) + .child("Git blocks local file transport for submodules by default. Trusting these sources will allow GitComet to enable file transport only for this repo/source pair."), + ) + .child( + div() + .px_2() + .pb_1() + .child( + components::Button::new( + "submodule_trust_cve_link", + "Read about CVE-2022-39253", + ) + .style(components::ButtonStyle::Filled) + .borderless() + .no_hover_border() + .end_slot(div().font_family(UI_MONOSPACE_FONT_FAMILY).child("->")) + .on_click(theme, cx, |_this, _e, _window, cx| { + cx.open_url(SUBMODULE_TRUST_CVE_URL); + }), + ), + ) + .children(sources.iter().cloned().map(|source| { + div() + .px_2() + .pb_1() + .flex() + .flex_col() + .gap_0p5() + .child( + div() + .text_xs() + .text_color(theme.colors.text_muted) + .child(format!( + "Submodule: {}", + source.submodule_path.display() + )), + ) + .child( + div() + .text_sm() + .font_family(crate::font_preferences::EDITOR_MONOSPACE_FONT_FAMILY) + .child(source.display_source.clone()), + ) + .child( + div() + .text_xs() + .font_family(crate::font_preferences::EDITOR_MONOSPACE_FONT_FAMILY) + .text_color(theme.colors.text_muted) + .child(format!( + "Local path: {}", + source.local_source_path.display() + )), + ) + })) + .child(div().border_t_1().border_color(theme.colors.border)) + .child( + div() + .px_2() + .py_1() + .flex() + .items_center() + .justify_between() + .child( + components::Button::new("submodule_trust_cancel", cancel_label) + .style(components::ButtonStyle::Outlined) + .on_click(theme, cx, move |this, _e, window, cx| { + this.store.dispatch(Msg::CancelSubmoduleTrustPrompt); + match &operation { + SubmoduleTrustPromptOperation::Add { url, path } => { + let theme = this.theme; + this.submodule_url_input.update(cx, |input, cx| { + input.set_theme(theme, cx); + input.set_text(url, cx); + cx.notify(); + }); + this.submodule_path_input.update(cx, |input, cx| { + input.set_theme(theme, cx); + input.set_text(&path.display().to_string(), cx); + cx.notify(); + }); + this.popover = Some(PopoverKind::submodule( + repo_id, + SubmodulePopoverKind::AddPrompt, + )); + let focus = this + .submodule_url_input + .read_with(cx, |input, _| input.focus_handle()); + window.focus(&focus); + } + SubmoduleTrustPromptOperation::Update => { + this.popover = None; + this.popover_anchor = None; + } + } + cx.notify(); + }), + ) + .child( + components::Button::new("submodule_trust_confirm", confirm_label) + .style(components::ButtonStyle::Filled) + .on_click(theme, cx, |this, _e, _window, cx| { + this.store.dispatch(Msg::ConfirmSubmoduleTrustPrompt); + this.popover = None; + this.popover_anchor = None; + cx.notify(); + }), + ), + ) +} diff --git a/crates/gitcomet-ui-gpui/src/view/state_apply.rs b/crates/gitcomet-ui-gpui/src/view/state_apply.rs index 071e9b0c..c431568e 100644 --- a/crates/gitcomet-ui-gpui/src/view/state_apply.rs +++ b/crates/gitcomet-ui-gpui/src/view/state_apply.rs @@ -9,6 +9,7 @@ impl GitCometView { let prev_had_repos = !self.state.repos.is_empty(); let prev_banner_error = self.state.banner_error.clone(); let prev_auth_prompt = self.state.auth_prompt.clone(); + let prev_submodule_trust_prompt = self.state.submodule_trust_prompt.clone(); let next_banner_error = next.banner_error.clone(); let mut follow_up_msgs = Vec::new(); @@ -153,6 +154,9 @@ impl GitCometView { if prev_auth_prompt != self.state.auth_prompt { self.auth_prompt_key = None; } + if prev_submodule_trust_prompt != self.state.submodule_trust_prompt { + self.pending_submodule_trust_prompt = self.state.submodule_trust_prompt.clone(); + } if prev_had_repos && self.state.repos.is_empty() { self.popover_host .update(cx, |host, cx| host.close_popover(cx)); From 5c44505825b5b58d0f9409feef26407441d24bf7 Mon Sep 17 00:00:00 2001 From: Roope Airinen Date: Fri, 10 Apr 2026 12:28:09 +0300 Subject: [PATCH 3/6] add branch and advanced options (--force and --name) to submodule popup --- crates/gitcomet-core/src/services.rs | 9 +- crates/gitcomet-git-gix/src/repo/mod.rs | 11 +- .../gitcomet-git-gix/src/repo/submodules.rs | 60 +++-- .../tests/submodules_integration.rs | 242 +++++++++++++++++- crates/gitcomet-git/src/noop_backend.rs | 9 +- crates/gitcomet-state/src/model.rs | 8 +- crates/gitcomet-state/src/msg/effect.rs | 6 + crates/gitcomet-state/src/msg/message.rs | 9 + .../gitcomet-state/src/msg/message_debug.rs | 6 + .../src/msg/repo_command_kind.rs | 3 + crates/gitcomet-state/src/store/effects.rs | 26 +- .../src/store/effects/repo_commands.rs | 27 +- crates/gitcomet-state/src/store/reducer.rs | 76 +++++- .../src/store/reducer/actions_emit_effects.rs | 6 + .../src/store/tests/actions_emit_effects.rs | 26 +- .../gitcomet-state/src/store/tests/effects.rs | 3 + .../src/view/panels/popover.rs | 54 +++- .../src/view/panels/popover/fingerprint.rs | 11 +- .../panels/popover/submodule_add_prompt.rs | 128 +++++++++ .../panels/popover/submodule_trust_confirm.rs | 82 +++++- 20 files changed, 751 insertions(+), 51 deletions(-) diff --git a/crates/gitcomet-core/src/services.rs b/crates/gitcomet-core/src/services.rs index 241d7786..4b41ae6b 100644 --- a/crates/gitcomet-core/src/services.rs +++ b/crates/gitcomet-core/src/services.rs @@ -539,7 +539,11 @@ pub trait GitRepository: Send + Sync { ))) } - fn check_submodule_add_trust(&self, _url: &str, _path: &Path) -> Result { + fn check_submodule_add_trust( + &self, + _url: &str, + _path: &Path, + ) -> Result { Err(Error::new(ErrorKind::Unsupported( "submodule trust checks are not implemented for this backend", ))) @@ -555,6 +559,9 @@ pub trait GitRepository: Send + Sync { &self, _url: &str, _path: &Path, + _branch: Option<&str>, + _name: Option<&str>, + _force: bool, _approved_sources: &[SubmoduleTrustTarget], ) -> Result { Err(Error::new(ErrorKind::Unsupported( diff --git a/crates/gitcomet-git-gix/src/repo/mod.rs b/crates/gitcomet-git-gix/src/repo/mod.rs index 943d4b3b..84fec5e1 100644 --- a/crates/gitcomet-git-gix/src/repo/mod.rs +++ b/crates/gitcomet-git-gix/src/repo/mod.rs @@ -511,11 +511,7 @@ impl GitRepository for GixRepo { self.list_submodules_impl() } - fn check_submodule_add_trust( - &self, - url: &str, - path: &Path, - ) -> Result { + fn check_submodule_add_trust(&self, url: &str, path: &Path) -> Result { self.check_submodule_add_trust_impl(url, path) } @@ -527,9 +523,12 @@ impl GitRepository for GixRepo { &self, url: &str, path: &Path, + branch: Option<&str>, + name: Option<&str>, + force: bool, approved_sources: &[SubmoduleTrustTarget], ) -> Result { - self.add_submodule_with_output_impl(url, path, approved_sources) + self.add_submodule_with_output_impl(url, path, branch, name, force, approved_sources) } fn update_submodules_with_output( diff --git a/crates/gitcomet-git-gix/src/repo/submodules.rs b/crates/gitcomet-git-gix/src/repo/submodules.rs index 0f77781f..f38ba70b 100644 --- a/crates/gitcomet-git-gix/src/repo/submodules.rs +++ b/crates/gitcomet-git-gix/src/repo/submodules.rs @@ -35,11 +35,8 @@ impl GixRepo { path: &Path, ) -> Result { let repo = self.reopen_repo()?; - let Some(target) = trust_target_from_raw_source( - repo_workdir_for_submodule_trust(&repo), - path, - url, - )? + let Some(target) = + trust_target_from_raw_source(repo_workdir_for_submodule_trust(&repo), path, url)? else { return Ok(SubmoduleTrustDecision::Proceed); }; @@ -71,6 +68,9 @@ impl GixRepo { &self, url: &str, path: &Path, + branch: Option<&str>, + name: Option<&str>, + force: bool, approved_sources: &[SubmoduleTrustTarget], ) -> Result { let repo = self.reopen_repo()?; @@ -85,8 +85,23 @@ impl GixRepo { allow_file_submodule_transport(&mut cmd); } - cmd.arg("submodule").arg("add").arg(url).arg(path); - run_git_with_output(cmd, &format!("git submodule add {url} {}", path.display())) + cmd.arg("submodule").arg("add"); + let mut command = "git submodule add".to_string(); + if let Some(branch) = branch { + cmd.arg("--branch").arg(branch); + command.push_str(&format!(" --branch {branch}")); + } + if force { + cmd.arg("--force"); + command.push_str(" --force"); + } + if let Some(name) = name { + cmd.arg("--name").arg(name); + command.push_str(&format!(" --name {name}")); + } + cmd.arg(url).arg(path); + command.push_str(&format!(" {url} {}", path.display())); + run_git_with_output(cmd, &command) } pub(super) fn update_submodules_with_output_impl( @@ -236,7 +251,8 @@ fn collect_repo_untrusted_submodule_sources( .and_then(|path| pathbuf_from_gix_path(path.as_ref()))?; let full_path = prefix.join(&relative_path); - if let Some(target) = trust_target_from_submodule(current_workdir, &full_path, &submodule)? { + if let Some(target) = trust_target_from_submodule(current_workdir, &full_path, &submodule)? + { if !submodule_source_trusted(trust_root, &target)? { out.insert(full_path.clone(), target); } @@ -272,8 +288,7 @@ fn update_repo_submodules_recursive( .and_then(|path| pathbuf_from_gix_path(path.as_ref()))?; let full_path = prefix.join(&relative_path); - let local_target = - trust_target_from_submodule(current_workdir, &full_path, &submodule)?; + let local_target = trust_target_from_submodule(current_workdir, &full_path, &submodule)?; let mut cmd = git_workdir_cmd_for(current_workdir); if let Some(target) = local_target.as_ref() { @@ -402,7 +417,9 @@ fn open_gitlink_repo( } } -fn open_configured_submodule_repo(submodule: &gix::Submodule<'_>) -> Result> { +fn open_configured_submodule_repo( + submodule: &gix::Submodule<'_>, +) -> Result> { let state = submodule .state() .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule state: {e}"))))?; @@ -458,8 +475,10 @@ fn trust_target_from_parsed_url( return Ok(None); } - let local_source_path = - canonicalize_or_original(resolve_local_file_transport_path(current_repo_workdir, url)?); + let local_source_path = canonicalize_or_original(resolve_local_file_transport_path( + current_repo_workdir, + url, + )?); Ok(Some(SubmoduleTrustTarget { submodule_path: submodule_path.to_path_buf(), display_source, @@ -467,7 +486,10 @@ fn trust_target_from_parsed_url( })) } -fn resolve_local_file_transport_path(current_repo_workdir: &Path, url: &gix::Url) -> Result { +fn resolve_local_file_transport_path( + current_repo_workdir: &Path, + url: &gix::Url, +) -> Result { let mut path = pathbuf_from_gix_path(url.path.as_ref())?; if let Some(host) = url.host.as_deref() { if !host.eq_ignore_ascii_case("localhost") { @@ -498,10 +520,7 @@ fn persist_submodule_trust_approvals( Ok(()) } -fn submodule_source_trusted( - trust_root: &Path, - source: &SubmoduleTrustTarget, -) -> Result { +fn submodule_source_trusted(trust_root: &Path, source: &SubmoduleTrustTarget) -> Result { let key = submodule_file_transport_consent_key(trust_root, &source.local_source_path); Ok(git_config_get_bool_global(trust_root, &key)?.unwrap_or(false)) } @@ -544,7 +563,10 @@ fn submodule_file_transport_consent_key(trust_root: &Path, source_path: &Path) - let mut bytes = stable_path_bytes(&root); bytes.push(0); bytes.extend_from_slice(&stable_path_bytes(&source)); - format!("gitcomet.submodule.allowfiletransport-{:016x}", fnv1a_64(&bytes)) + format!( + "gitcomet.submodule.allowfiletransport-{:016x}", + fnv1a_64(&bytes) + ) } fn git_config_get_bool_global(trust_root: &Path, key: &str) -> Result> { diff --git a/crates/gitcomet-git-gix/tests/submodules_integration.rs b/crates/gitcomet-git-gix/tests/submodules_integration.rs index 9e0767fa..896a12c7 100644 --- a/crates/gitcomet-git-gix/tests/submodules_integration.rs +++ b/crates/gitcomet-git-gix/tests/submodules_integration.rs @@ -536,6 +536,9 @@ fn submodule_add_update_remove_round_trip() { .add_submodule_with_output( sub_repo.to_string_lossy().as_ref(), submodule_path, + None, + None, + false, &approved_sources, ) .expect("add submodule"); @@ -604,7 +607,7 @@ fn add_submodule_does_not_restrict_https_or_ssh_transports() { ("ssh://git@127.0.0.1:1/repo.git", "ssh"), ] { let err = opened - .add_submodule_with_output(url, Path::new("mods/sub-one"), &[]) + .add_submodule_with_output(url, Path::new("mods/sub-one"), None, None, false, &[]) .expect_err("dummy remote should fail without a reachable server"); let rendered = err.to_string(); assert!( @@ -643,7 +646,14 @@ fn add_local_submodule_requires_explicit_trust() { }; let err = opened - .add_submodule_with_output(sub_repo.to_string_lossy().as_ref(), submodule_path, &[]) + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + None, + None, + false, + &[], + ) .expect_err("local submodule should fail without trust"); assert!( err.to_string().contains("Explicit trust is required"), @@ -654,8 +664,236 @@ fn add_local_submodule_requires_explicit_trust() { .add_submodule_with_output( sub_repo.to_string_lossy().as_ref(), submodule_path, + None, + None, + false, &approved_sources, ) .expect("add trusted local submodule"); assert_eq!(add_output.exit_code, Some(0)); } + +#[test] +fn add_submodule_supports_branch_selection() { + if !require_git_shell_for_submodule_tests() { + return; + } + let dir = tempfile::tempdir().expect("create tempdir"); + let root = dir.path(); + + let sub_repo = root.join("sub"); + let parent_repo = root.join("parent"); + fs::create_dir_all(&sub_repo).expect("create sub repository directory"); + fs::create_dir_all(&parent_repo).expect("create parent repository directory"); + + init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); + run_git(&sub_repo, &["branch", "feature"]); + init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + + let backend = GixBackend; + let opened = backend.open(&parent_repo).expect("open parent repository"); + let submodule_path = Path::new("mods/sub"); + let approved_sources = match opened + .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), submodule_path) + .expect("check local submodule trust") + { + SubmoduleTrustDecision::Prompt { sources } => sources, + other => panic!("expected trust prompt for local submodule, got {other:?}"), + }; + + let add_output = opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + Some("feature"), + None, + false, + &approved_sources, + ) + .expect("add submodule with branch"); + assert_eq!(add_output.exit_code, Some(0)); + assert!(add_output.command.contains("--branch feature")); + + let gitmodules = fs::read_to_string(parent_repo.join(".gitmodules")).expect("read .gitmodules"); + assert!(gitmodules.contains("branch = feature")); + assert_eq!( + git_stdout(&parent_repo.join("mods/sub"), &["branch", "--show-current"]), + "feature" + ); +} + +#[test] +fn add_submodule_supports_custom_logical_name_for_local_git_dir_collision() { + if !require_git_shell_for_submodule_tests() { + return; + } + let dir = tempfile::tempdir().expect("create tempdir"); + let root = dir.path(); + + let sub_repo = root.join("sub"); + let parent_repo = root.join("parent"); + fs::create_dir_all(&sub_repo).expect("create sub repository directory"); + fs::create_dir_all(&parent_repo).expect("create parent repository directory"); + + init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); + init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + + let backend = GixBackend; + let opened = backend.open(&parent_repo).expect("open parent repository"); + let submodule_path = Path::new("sm"); + let approved_sources = match opened + .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), submodule_path) + .expect("check local submodule trust") + { + SubmoduleTrustDecision::Prompt { sources } => sources, + other => panic!("expected trust prompt for local submodule, got {other:?}"), + }; + + opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + None, + None, + false, + &approved_sources, + ) + .expect("add initial submodule"); + run_git( + &parent_repo, + &[ + "-c", + "commit.gpgsign=false", + "commit", + "-m", + "add submodule", + ], + ); + + opened + .remove_submodule_with_output(submodule_path) + .expect("remove submodule"); + run_git( + &parent_repo, + &[ + "-c", + "commit.gpgsign=false", + "commit", + "-m", + "remove submodule", + ], + ); + assert!( + parent_repo.join(".git/modules/sm").exists(), + "expected stale local submodule git dir" + ); + + let err = opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + None, + None, + false, + &[], + ) + .expect_err("add without custom name or force should fail"); + let rendered = err.to_string(); + assert!( + rendered.contains("If you want to reuse this local git directory") + || rendered.contains("use the '--force' option") + || rendered.contains("choose another name with the '--name' option"), + "unexpected collision error: {rendered}" + ); + + let add_output = opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + None, + Some("sm-renamed"), + false, + &[], + ) + .expect("add submodule with custom logical name"); + assert_eq!(add_output.exit_code, Some(0)); +} + +#[test] +fn add_submodule_supports_force_for_local_git_dir_collision() { + if !require_git_shell_for_submodule_tests() { + return; + } + let dir = tempfile::tempdir().expect("create tempdir"); + let root = dir.path(); + + let sub_repo = root.join("sub"); + let parent_repo = root.join("parent"); + fs::create_dir_all(&sub_repo).expect("create sub repository directory"); + fs::create_dir_all(&parent_repo).expect("create parent repository directory"); + + init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); + init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + + let backend = GixBackend; + let opened = backend.open(&parent_repo).expect("open parent repository"); + let submodule_path = Path::new("sm"); + let approved_sources = match opened + .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), submodule_path) + .expect("check local submodule trust") + { + SubmoduleTrustDecision::Prompt { sources } => sources, + other => panic!("expected trust prompt for local submodule, got {other:?}"), + }; + + opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + None, + None, + false, + &approved_sources, + ) + .expect("add initial submodule"); + run_git( + &parent_repo, + &[ + "-c", + "commit.gpgsign=false", + "commit", + "-m", + "add submodule", + ], + ); + + opened + .remove_submodule_with_output(submodule_path) + .expect("remove submodule"); + run_git( + &parent_repo, + &[ + "-c", + "commit.gpgsign=false", + "commit", + "-m", + "remove submodule", + ], + ); + assert!( + parent_repo.join(".git/modules/sm").exists(), + "expected stale local submodule git dir" + ); + + let add_output = opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + None, + None, + true, + &[], + ) + .expect("add submodule with force"); + assert_eq!(add_output.exit_code, Some(0)); +} diff --git a/crates/gitcomet-git/src/noop_backend.rs b/crates/gitcomet-git/src/noop_backend.rs index c0caa9a5..400836a3 100644 --- a/crates/gitcomet-git/src/noop_backend.rs +++ b/crates/gitcomet-git/src/noop_backend.rs @@ -292,7 +292,14 @@ mod tests { assert_unsupported(repo.list_submodules()); assert_unsupported(repo.check_submodule_add_trust("https://example.com/repo.git", path)); assert_unsupported(repo.check_submodule_update_trust()); - assert_unsupported(repo.add_submodule_with_output("https://example.com/repo.git", path, &[])); + assert_unsupported(repo.add_submodule_with_output( + "https://example.com/repo.git", + path, + None, + None, + false, + &[], + )); assert_unsupported(repo.update_submodules_with_output(&[])); assert_unsupported(repo.remove_submodule_with_output(path)); } diff --git a/crates/gitcomet-state/src/model.rs b/crates/gitcomet-state/src/model.rs index a7b80a34..4129715a 100644 --- a/crates/gitcomet-state/src/model.rs +++ b/crates/gitcomet-state/src/model.rs @@ -278,7 +278,13 @@ pub struct AuthPromptState { #[derive(Clone, Debug, Eq, PartialEq)] pub enum SubmoduleTrustPromptOperation { - Add { url: String, path: PathBuf }, + Add { + url: String, + path: PathBuf, + branch: Option, + name: Option, + force: bool, + }, Update, } diff --git a/crates/gitcomet-state/src/msg/effect.rs b/crates/gitcomet-state/src/msg/effect.rs index cb494313..67c5c98b 100644 --- a/crates/gitcomet-state/src/msg/effect.rs +++ b/crates/gitcomet-state/src/msg/effect.rs @@ -189,6 +189,9 @@ pub enum Effect { repo_id: RepoId, url: String, path: PathBuf, + branch: Option, + name: Option, + force: bool, }, CheckSubmoduleUpdateTrust { repo_id: RepoId, @@ -197,6 +200,9 @@ pub enum Effect { repo_id: RepoId, url: String, path: PathBuf, + branch: Option, + name: Option, + force: bool, approved_sources: Vec, auth: Option, }, diff --git a/crates/gitcomet-state/src/msg/message.rs b/crates/gitcomet-state/src/msg/message.rs index 0c3e979b..0d2a98a7 100644 --- a/crates/gitcomet-state/src/msg/message.rs +++ b/crates/gitcomet-state/src/msg/message.rs @@ -247,11 +247,17 @@ pub enum Msg { repo_id: RepoId, url: String, path: PathBuf, + branch: Option, + name: Option, + force: bool, }, AddSubmoduleTrusted { repo_id: RepoId, url: String, path: PathBuf, + branch: Option, + name: Option, + force: bool, approved_sources: Vec, }, UpdateSubmodules { @@ -594,6 +600,9 @@ pub enum InternalMsg { repo_id: RepoId, url: String, path: PathBuf, + branch: Option, + name: Option, + force: bool, result: Result, }, SubmoduleUpdateTrustChecked { diff --git a/crates/gitcomet-state/src/msg/message_debug.rs b/crates/gitcomet-state/src/msg/message_debug.rs index 7a269708..eaaeac8f 100644 --- a/crates/gitcomet-state/src/msg/message_debug.rs +++ b/crates/gitcomet-state/src/msg/message_debug.rs @@ -160,12 +160,18 @@ impl std::fmt::Debug for InternalMsg { repo_id, url, path, + branch, + name, + force, result, } => f .debug_struct("SubmoduleAddTrustChecked") .field("repo_id", repo_id) .field("url", url) .field("path", path) + .field("branch", branch) + .field("name", name) + .field("force", force) .field("result", result) .finish(), InternalMsg::SubmoduleUpdateTrustChecked { repo_id, result } => f diff --git a/crates/gitcomet-state/src/msg/repo_command_kind.rs b/crates/gitcomet-state/src/msg/repo_command_kind.rs index 13e35435..37b00a3c 100644 --- a/crates/gitcomet-state/src/msg/repo_command_kind.rs +++ b/crates/gitcomet-state/src/msg/repo_command_kind.rs @@ -113,6 +113,9 @@ pub enum RepoCommandKind { AddSubmodule { url: String, path: PathBuf, + branch: Option, + name: Option, + force: bool, approved_sources: Vec, }, UpdateSubmodules { diff --git a/crates/gitcomet-state/src/store/effects.rs b/crates/gitcomet-state/src/store/effects.rs index 77df2027..8836a308 100644 --- a/crates/gitcomet-state/src/store/effects.rs +++ b/crates/gitcomet-state/src/store/effects.rs @@ -255,9 +255,16 @@ pub(super) fn schedule_effect( Effect::ForceRemoveWorktree { repo_id, path } => { repo_commands::schedule_force_remove_worktree(executor, repos, msg_tx, repo_id, path); } - Effect::CheckSubmoduleAddTrust { repo_id, url, path } => { + Effect::CheckSubmoduleAddTrust { + repo_id, + url, + path, + branch, + name, + force, + } => { repo_commands::schedule_check_submodule_add_trust( - executor, repos, msg_tx, repo_id, url, path, + executor, repos, msg_tx, repo_id, url, path, branch, name, force, ); } Effect::CheckSubmoduleUpdateTrust { repo_id } => { @@ -267,11 +274,24 @@ pub(super) fn schedule_effect( repo_id, url, path, + branch, + name, + force, approved_sources, auth, } => { repo_commands::schedule_add_submodule( - executor, repos, msg_tx, repo_id, url, path, approved_sources, auth, + executor, + repos, + msg_tx, + repo_id, + url, + path, + branch, + name, + force, + approved_sources, + auth, ); } Effect::UpdateSubmodules { diff --git a/crates/gitcomet-state/src/store/effects/repo_commands.rs b/crates/gitcomet-state/src/store/effects/repo_commands.rs index 95ffb7d5..7f7b0275 100644 --- a/crates/gitcomet-state/src/store/effects/repo_commands.rs +++ b/crates/gitcomet-state/src/store/effects/repo_commands.rs @@ -233,11 +233,16 @@ pub(super) fn schedule_add_submodule( repo_id: RepoId, url: String, path: PathBuf, + branch: Option, + name: Option, + force: bool, approved_sources: Vec, auth: Option, ) { let command_url = url.clone(); let command_path = path.clone(); + let command_branch = branch.clone(); + let command_name = name.clone(); let command_sources = approved_sources.clone(); schedule_repo_command( executor, @@ -247,11 +252,21 @@ pub(super) fn schedule_add_submodule( RepoCommandKind::AddSubmodule { url: command_url, path: command_path, + branch: command_branch, + name: command_name, + force, approved_sources: command_sources, }, move |repo| { run_with_git_auth(auth, || { - repo.add_submodule_with_output(&url, &path, &approved_sources) + repo.add_submodule_with_output( + &url, + &path, + branch.as_deref(), + name.as_deref(), + force, + &approved_sources, + ) }) }, ); @@ -275,7 +290,9 @@ pub(super) fn schedule_update_submodules( approved_sources: command_sources, }, move |repo| { - run_with_git_auth(auth, || repo.update_submodules_with_output(&approved_sources)) + run_with_git_auth(auth, || { + repo.update_submodules_with_output(&approved_sources) + }) }, ); } @@ -287,6 +304,9 @@ pub(super) fn schedule_check_submodule_add_trust( repo_id: RepoId, url: String, path: PathBuf, + branch: Option, + name: Option, + force: bool, ) { spawn_with_repo(executor, repos, repo_id, msg_tx, move |repo, msg_tx| { let result = repo.check_submodule_add_trust(&url, &path); @@ -296,6 +316,9 @@ pub(super) fn schedule_check_submodule_add_trust( repo_id, url, path, + branch, + name, + force, result, }), ); diff --git a/crates/gitcomet-state/src/store/reducer.rs b/crates/gitcomet-state/src/store/reducer.rs index 3dc922de..e7a614f2 100644 --- a/crates/gitcomet-state/src/store/reducer.rs +++ b/crates/gitcomet-state/src/store/reducer.rs @@ -253,11 +253,17 @@ fn retry_msg_for_repo_command(repo_id: RepoId, command: RepoCommandKind) -> Opti RepoCommandKind::AddSubmodule { url, path, + branch, + name, + force, approved_sources, } => Msg::AddSubmoduleTrusted { repo_id, url, path, + branch, + name, + force, approved_sources, }, RepoCommandKind::UpdateSubmodules { approved_sources } => Msg::UpdateSubmodulesTrusted { @@ -664,18 +670,43 @@ pub(super) fn reduce( }; actions_emit_effects::force_remove_worktree(repo_id, normalized_path) } - Msg::AddSubmodule { repo_id, url, path } => { + Msg::AddSubmodule { + repo_id, + url, + path, + branch, + name, + force, + } => { state.submodule_trust_prompt = None; - vec![Effect::CheckSubmoduleAddTrust { repo_id, url, path }] + vec![Effect::CheckSubmoduleAddTrust { + repo_id, + url, + path, + branch, + name, + force, + }] } Msg::AddSubmoduleTrusted { repo_id, url, path, + branch, + name, + force, approved_sources, } => { begin_local_action(state, repo_id); - actions_emit_effects::add_submodule(repo_id, url, path, approved_sources) + actions_emit_effects::add_submodule( + repo_id, + url, + path, + branch, + name, + force, + approved_sources, + ) } Msg::UpdateSubmodules { repo_id } => { state.submodule_trust_prompt = None; @@ -693,9 +724,23 @@ pub(super) fn reduce( return Vec::new(); }; match prompt.operation { - SubmoduleTrustPromptOperation::Add { url, path } => { + SubmoduleTrustPromptOperation::Add { + url, + path, + branch, + name, + force, + } => { begin_local_action(state, prompt.repo_id); - actions_emit_effects::add_submodule(prompt.repo_id, url, path, prompt.sources) + actions_emit_effects::add_submodule( + prompt.repo_id, + url, + path, + branch, + name, + force, + prompt.sources, + ) } SubmoduleTrustPromptOperation::Update => { begin_local_action(state, prompt.repo_id); @@ -1052,16 +1097,33 @@ pub(super) fn reduce( repo_id, url, path, + branch, + name, + force, result, }) => match result { Ok(gitcomet_core::services::SubmoduleTrustDecision::Proceed) => { begin_local_action(state, repo_id); - actions_emit_effects::add_submodule(repo_id, url, path, Vec::new()) + actions_emit_effects::add_submodule( + repo_id, + url, + path, + branch, + name, + force, + Vec::new(), + ) } Ok(gitcomet_core::services::SubmoduleTrustDecision::Prompt { sources }) => { state.submodule_trust_prompt = Some(SubmoduleTrustPromptState { repo_id, - operation: SubmoduleTrustPromptOperation::Add { url, path }, + operation: SubmoduleTrustPromptOperation::Add { + url, + path, + branch, + name, + force, + }, sources, }); Vec::new() diff --git a/crates/gitcomet-state/src/store/reducer/actions_emit_effects.rs b/crates/gitcomet-state/src/store/reducer/actions_emit_effects.rs index 6932f3ec..5c1091fe 100644 --- a/crates/gitcomet-state/src/store/reducer/actions_emit_effects.rs +++ b/crates/gitcomet-state/src/store/reducer/actions_emit_effects.rs @@ -121,12 +121,18 @@ pub(super) fn add_submodule( repo_id: RepoId, url: String, path: PathBuf, + branch: Option, + name: Option, + force: bool, approved_sources: Vec, ) -> Vec { vec![Effect::AddSubmodule { repo_id, url, path, + branch, + name, + force, approved_sources, auth: None, }] diff --git a/crates/gitcomet-state/src/store/tests/actions_emit_effects.rs b/crates/gitcomet-state/src/store/tests/actions_emit_effects.rs index 604e8e9c..9c5dd1eb 100644 --- a/crates/gitcomet-state/src/store/tests/actions_emit_effects.rs +++ b/crates/gitcomet-state/src/store/tests/actions_emit_effects.rs @@ -652,6 +652,9 @@ fn submodule_commands_reload_submodules_on_success() { command: RepoCommandKind::AddSubmodule { url: "https://example.com/sub.git".to_string(), path: PathBuf::from("submodule"), + branch: None, + name: None, + force: false, approved_sources: Vec::new(), }, result: Ok(CommandOutput::empty_success("git submodule add")), @@ -2217,6 +2220,9 @@ fn repo_command_finished_error_summaries_cover_additional_labels() { RepoCommandKind::AddSubmodule { url: "https://example.com/sub.git".to_string(), path: PathBuf::from("mods/sub"), + branch: None, + name: None, + force: false, approved_sources: Vec::new(), }, "Submodule", @@ -2336,6 +2342,9 @@ fn repo_command_finished_success_summaries_cover_additional_commands() { RepoCommandKind::AddSubmodule { url: "https://example.com/sub.git".to_string(), path: PathBuf::from("mods/sub"), + branch: None, + name: None, + force: false, approved_sources: Vec::new(), }, "Submodule added → mods/sub", @@ -2496,6 +2505,9 @@ fn checkout_branch_and_submodule_messages_emit_effects() { repo_id: RepoId(1), url: "https://example.com/sub.git".to_string(), path: PathBuf::from("mods/sub"), + branch: Some("feature".to_string()), + name: None, + force: false, }, ); assert!(matches!( @@ -2504,7 +2516,11 @@ fn checkout_branch_and_submodule_messages_emit_effects() { repo_id: RepoId(1), url, path, - }] if url == "https://example.com/sub.git" && path == &PathBuf::from("mods/sub") + branch, + .. + }] if url == "https://example.com/sub.git" + && path == &PathBuf::from("mods/sub") + && branch.as_deref() == Some("feature") )); let update_submodules = reduce( @@ -2563,6 +2579,9 @@ fn local_submodule_add_trust_prompt_confirms_into_add_effect() { repo_id, url: "../local-sub".to_string(), path: PathBuf::from("mods/sub"), + branch: Some("feature".to_string()), + name: None, + force: false, result: Ok(gitcomet_core::services::SubmoduleTrustDecision::Prompt { sources: vec![source.clone()], }), @@ -2576,6 +2595,9 @@ fn local_submodule_add_trust_prompt_confirms_into_add_effect() { operation: crate::model::SubmoduleTrustPromptOperation::Add { url: "../local-sub".to_string(), path: PathBuf::from("mods/sub"), + branch: Some("feature".to_string()), + name: None, + force: false, }, sources: vec![source.clone()], }) @@ -2594,10 +2616,12 @@ fn local_submodule_add_trust_prompt_confirms_into_add_effect() { repo_id: RepoId(1), url, path, + branch, approved_sources, .. }] if url == "../local-sub" && path == &PathBuf::from("mods/sub") + && branch.as_deref() == Some("feature") && approved_sources == &vec![source] )); } diff --git a/crates/gitcomet-state/src/store/tests/effects.rs b/crates/gitcomet-state/src/store/tests/effects.rs index d60683ec..3e1db723 100644 --- a/crates/gitcomet-state/src/store/tests/effects.rs +++ b/crates/gitcomet-state/src/store/tests/effects.rs @@ -3291,6 +3291,9 @@ fn schedule_effect_dispatches_many_variants_with_repo_present() { repo_id, url: "https://example.com/repo.git".to_string(), path: PathBuf::from("sub"), + branch: None, + name: None, + force: false, approved_sources: Vec::new(), auth: None, }, diff --git a/crates/gitcomet-ui-gpui/src/view/panels/popover.rs b/crates/gitcomet-ui-gpui/src/view/panels/popover.rs index 6f39c2e4..18ebeb1a 100644 --- a/crates/gitcomet-ui-gpui/src/view/panels/popover.rs +++ b/crates/gitcomet-ui-gpui/src/view/panels/popover.rs @@ -30,10 +30,10 @@ mod search_inputs; mod stash_drop_confirm; mod stash_prompt; mod submodule_add_prompt; -mod submodule_trust_confirm; mod submodule_open_picker; mod submodule_remove_confirm; mod submodule_remove_picker; +mod submodule_trust_confirm; mod worktree_add_prompt; mod worktree_open_picker; mod worktree_remove_confirm; @@ -97,6 +97,10 @@ pub(in super::super) struct PopoverHost { worktree_ref_input: Entity, submodule_url_input: Entity, submodule_path_input: Entity, + submodule_branch_input: Entity, + submodule_name_input: Entity, + submodule_add_advanced_expanded: bool, + submodule_force_enabled: bool, } impl PopoverHost { @@ -462,6 +466,34 @@ impl PopoverHost { ) }); + let submodule_name_input = cx.new(|cx| { + components::TextInput::new( + components::TextInputOptions { + placeholder: "submodule-logical-name".into(), + multiline: false, + read_only: false, + chromeless: false, + soft_wrap: false, + }, + window, + cx, + ) + }); + + let submodule_branch_input = cx.new(|cx| { + components::TextInput::new( + components::TextInputOptions { + placeholder: "feature".into(), + multiline: false, + read_only: false, + chromeless: false, + soft_wrap: false, + }, + window, + cx, + ) + }); + let context_menu_focus_handle = cx.focus_handle().tab_index(0).tab_stop(false); Self { @@ -513,6 +545,10 @@ impl PopoverHost { worktree_ref_input, submodule_url_input, submodule_path_input, + submodule_branch_input, + submodule_name_input, + submodule_add_advanced_expanded: false, + submodule_force_enabled: false, } } @@ -547,6 +583,10 @@ impl PopoverHost { .update(cx, |input, cx| input.set_theme(theme, cx)); self.submodule_path_input .update(cx, |input, cx| input.set_theme(theme, cx)); + self.submodule_branch_input + .update(cx, |input, cx| input.set_theme(theme, cx)); + self.submodule_name_input + .update(cx, |input, cx| input.set_theme(theme, cx)); if let Some(input) = &self.repo_picker_search_input { input.update(cx, |input, cx| input.set_theme(theme, cx)); @@ -1026,6 +1066,8 @@ impl PopoverHost { .. } => { let theme = self.theme; + self.submodule_add_advanced_expanded = false; + self.submodule_force_enabled = false; self.submodule_url_input.update(cx, |input, cx| { input.set_theme(theme, cx); input.set_text("", cx); @@ -1036,6 +1078,16 @@ impl PopoverHost { input.set_text("", cx); cx.notify(); }); + self.submodule_branch_input.update(cx, |input, cx| { + input.set_theme(theme, cx); + input.set_text("", cx); + cx.notify(); + }); + self.submodule_name_input.update(cx, |input, cx| { + input.set_theme(theme, cx); + input.set_text("", cx); + cx.notify(); + }); let focus = self .submodule_url_input .read_with(cx, |i, _| i.focus_handle()); diff --git a/crates/gitcomet-ui-gpui/src/view/panels/popover/fingerprint.rs b/crates/gitcomet-ui-gpui/src/view/panels/popover/fingerprint.rs index 8a2d26ae..399a1e7c 100644 --- a/crates/gitcomet-ui-gpui/src/view/panels/popover/fingerprint.rs +++ b/crates/gitcomet-ui-gpui/src/view/panels/popover/fingerprint.rs @@ -60,10 +60,19 @@ pub(super) fn notify_fingerprint(state: &AppState, popover: &PopoverKind) -> u64 if let Some(prompt) = state.submodule_trust_prompt.as_ref() { prompt.repo_id.hash(&mut hasher); match &prompt.operation { - SubmoduleTrustPromptOperation::Add { url, path } => { + SubmoduleTrustPromptOperation::Add { + url, + path, + branch, + name, + force, + } => { 0u8.hash(&mut hasher); url.hash(&mut hasher); path.hash(&mut hasher); + branch.hash(&mut hasher); + name.hash(&mut hasher); + force.hash(&mut hasher); } SubmoduleTrustPromptOperation::Update => { 1u8.hash(&mut hasher); diff --git a/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_add_prompt.rs b/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_add_prompt.rs index 9d33ba77..14dd54e4 100644 --- a/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_add_prompt.rs +++ b/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_add_prompt.rs @@ -1,11 +1,68 @@ use super::*; +fn advanced_toggle(theme: AppTheme, expanded: bool) -> gpui::Stateful { + div() + .id("submodule_add_advanced_toggle") + .debug_selector(|| "submodule_add_advanced_toggle".to_string()) + .w_full() + .px_2() + .py_1() + .flex() + .items_center() + .justify_between() + .rounded(px(theme.radii.row)) + .cursor(CursorStyle::PointingHand) + .hover(move |s| s.bg(theme.colors.hover)) + .active(move |s| s.bg(theme.colors.active)) + .child(div().text_sm().child("Advanced")) + .child( + div() + .text_sm() + .font_family(UI_MONOSPACE_FONT_FAMILY) + .text_color(theme.colors.text_muted) + .child(if expanded { "^" } else { "v" }), + ) +} + +fn force_toggle(theme: AppTheme, enabled: bool) -> gpui::Stateful { + div() + .id("submodule_add_force_toggle") + .debug_selector(|| "submodule_add_force_toggle".to_string()) + .w_full() + .px_2() + .py_1() + .flex() + .items_center() + .justify_between() + .rounded(px(theme.radii.row)) + .cursor(CursorStyle::PointingHand) + .hover(move |s| s.bg(theme.colors.hover)) + .active(move |s| s.bg(theme.colors.active)) + .child( + div() + .text_sm() + .child("Force reuse / bypass collision checks"), + ) + .child( + div() + .text_sm() + .text_color(if enabled { + theme.colors.success + } else { + theme.colors.text_muted + }) + .child(if enabled { "On" } else { "Off" }), + ) +} + pub(super) fn panel( this: &mut PopoverHost, repo_id: RepoId, cx: &mut gpui::Context, ) -> gpui::Div { let theme = this.theme; + let advanced_expanded = this.submodule_add_advanced_expanded; + let force_enabled = this.submodule_force_enabled; div() .flex() @@ -52,6 +109,65 @@ pub(super) fn panel( .min_w(px(0.0)) .child(this.submodule_path_input.clone()), ) + .child( + div() + .px_2() + .py_1() + .text_xs() + .text_color(theme.colors.text_muted) + .child("Branch (optional)"), + ) + .child( + div() + .px_2() + .pb_1() + .w_full() + .min_w(px(0.0)) + .child(this.submodule_branch_input.clone()), + ) + .child( + advanced_toggle(theme, advanced_expanded).on_click(cx.listener(|this, _e: &ClickEvent, _w, cx| { + this.submodule_add_advanced_expanded = !this.submodule_add_advanced_expanded; + cx.notify(); + })), + ) + .when(advanced_expanded, |this_panel| { + this_panel + .child( + div() + .px_2() + .py_1() + .text_xs() + .text_color(theme.colors.text_muted) + .child("Logical name (optional)"), + ) + .child( + div() + .px_2() + .pb_1() + .w_full() + .min_w(px(0.0)) + .child(this.submodule_name_input.clone()), + ) + .child( + force_toggle(theme, force_enabled).on_click(cx.listener( + |this, _e: &ClickEvent, _w, cx| { + this.submodule_force_enabled = !this.submodule_force_enabled; + cx.notify(); + }, + )), + ) + .child( + div() + .px_2() + .pb_1() + .text_xs() + .text_color(theme.colors.text_muted) + .child( + "Force reuses an existing local submodule git dir or bypasses Git's normal collision refusal.", + ), + ) + }) .child(div().border_t_1().border_color(theme.colors.border)) .child( div() @@ -79,6 +195,15 @@ pub(super) fn panel( let path_text = this .submodule_path_input .read_with(cx, |i, _| i.text().trim().to_string()); + let branch = this.submodule_branch_input.read_with(cx, |i, _| { + let text = i.text().trim().to_string(); + if text.is_empty() { None } else { Some(text) } + }); + let name = this.submodule_name_input.read_with(cx, |i, _| { + let text = i.text().trim().to_string(); + if text.is_empty() { None } else { Some(text) } + }); + let force = this.submodule_force_enabled; if url.is_empty() || path_text.is_empty() { this.push_toast( components::ToastKind::Error, @@ -91,6 +216,9 @@ pub(super) fn panel( repo_id, url, path: std::path::PathBuf::from(path_text), + branch, + name, + force, }); this.popover = None; this.popover_anchor = None; diff --git a/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs b/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs index df32796e..bec6c0f6 100644 --- a/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs +++ b/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs @@ -23,9 +23,20 @@ pub(super) fn panel( SubmoduleTrustPromptOperation::Add { .. } => { ("Trust local submodule?", "Trust and add", "Back") } - SubmoduleTrustPromptOperation::Update => { - ("Trust local submodule sources?", "Trust and update", "Cancel") - } + SubmoduleTrustPromptOperation::Update => ( + "Trust local submodule sources?", + "Trust and update", + "Cancel", + ), + }; + let (add_branch, add_name, add_force) = match &prompt.operation { + SubmoduleTrustPromptOperation::Add { + branch, + name, + force, + .. + } => (branch.clone(), name.clone(), *force), + SubmoduleTrustPromptOperation::Update => (None, None, false), }; let sources = prompt.sources.clone(); let operation = prompt.operation.clone(); @@ -102,6 +113,42 @@ pub(super) fn panel( )), ) })) + .when(add_branch.is_some() || add_name.is_some() || add_force, |panel| { + panel.child( + div() + .px_2() + .pb_1() + .flex() + .flex_col() + .gap_0p5() + .when_some(add_branch.clone(), |details, branch| { + details.child( + div() + .text_xs() + .font_family(crate::font_preferences::EDITOR_MONOSPACE_FONT_FAMILY) + .text_color(theme.colors.text_muted) + .child(format!("Branch: {branch}")), + ) + }) + .when_some(add_name.clone(), |details, name| { + details.child( + div() + .text_xs() + .font_family(crate::font_preferences::EDITOR_MONOSPACE_FONT_FAMILY) + .text_color(theme.colors.text_muted) + .child(format!("Logical name: {name}")), + ) + }) + .when(add_force, |details| { + details.child( + div() + .text_xs() + .text_color(theme.colors.text_muted) + .child("Force: enabled"), + ) + }), + ) + }) .child(div().border_t_1().border_color(theme.colors.border)) .child( div() @@ -115,12 +162,22 @@ pub(super) fn panel( .style(components::ButtonStyle::Outlined) .on_click(theme, cx, move |this, _e, window, cx| { this.store.dispatch(Msg::CancelSubmoduleTrustPrompt); - match &operation { - SubmoduleTrustPromptOperation::Add { url, path } => { + match operation.clone() { + SubmoduleTrustPromptOperation::Add { + url, + path, + branch, + name, + force, + } => { let theme = this.theme; + let restored_branch = branch.unwrap_or_default(); + let restored_branch_for_input = restored_branch.clone(); + let restored_name = name.unwrap_or_default(); + let restored_name_for_input = restored_name.clone(); this.submodule_url_input.update(cx, |input, cx| { input.set_theme(theme, cx); - input.set_text(url, cx); + input.set_text(&url, cx); cx.notify(); }); this.submodule_path_input.update(cx, |input, cx| { @@ -128,6 +185,19 @@ pub(super) fn panel( input.set_text(&path.display().to_string(), cx); cx.notify(); }); + this.submodule_branch_input.update(cx, move |input, cx| { + input.set_theme(theme, cx); + input.set_text(&restored_branch_for_input, cx); + cx.notify(); + }); + this.submodule_name_input.update(cx, move |input, cx| { + input.set_theme(theme, cx); + input.set_text(&restored_name_for_input, cx); + cx.notify(); + }); + this.submodule_add_advanced_expanded = + !restored_name.is_empty() || force; + this.submodule_force_enabled = force; this.popover = Some(PopoverKind::submodule( repo_id, SubmodulePopoverKind::AddPrompt, From 85522dc3a2feed70824cf599856502614e2332e1 Mon Sep 17 00:00:00 2001 From: Roope Airinen Date: Fri, 10 Apr 2026 13:13:22 +0300 Subject: [PATCH 4/6] removing submodule now also cleans up associated metadata --- .../gitcomet-git-gix/src/repo/submodules.rs | 130 ++++++++++++ .../tests/submodules_integration.rs | 199 +++++++++++------- 2 files changed, 255 insertions(+), 74 deletions(-) diff --git a/crates/gitcomet-git-gix/src/repo/submodules.rs b/crates/gitcomet-git-gix/src/repo/submodules.rs index f38ba70b..c1b75e2f 100644 --- a/crates/gitcomet-git-gix/src/repo/submodules.rs +++ b/crates/gitcomet-git-gix/src/repo/submodules.rs @@ -11,6 +11,7 @@ use gitcomet_core::services::{ }; use gix::bstr::ByteSlice as _; use std::collections::BTreeMap; +use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; @@ -131,6 +132,12 @@ impl GixRepo { } pub(super) fn remove_submodule_with_output_impl(&self, path: &Path) -> Result { + let repo = self.reopen_repo()?; + let workdir = repo_workdir_for_submodule_trust(&repo).to_path_buf(); + let git_dir = repo.git_dir().to_path_buf(); + let logical_name = + resolve_submodule_logical_name(&repo, path)?.unwrap_or_else(|| path.to_path_buf()); + let mut cmd1 = self.git_workdir_cmd(); cmd1.arg("submodule") .arg("deinit") @@ -144,6 +151,13 @@ impl GixRepo { cmd2.arg("rm").arg("-f").arg("--").arg(path); let out2 = run_git_with_output(cmd2, &format!("git rm -f {}", path.display()))?; + cleanup_removed_submodule_metadata(&workdir, &git_dir, &logical_name).map_err(|err| { + Error::new(ErrorKind::Backend(format!( + "Removed submodule '{}' from the worktree and index, but failed to clean metadata: {err}", + path.display() + ))) + })?; + Ok(CommandOutput { command: format!("Remove submodule {}", path.display()), stdout: [out1.stdout.trim_end(), out2.stdout.trim_end()] @@ -371,6 +385,122 @@ fn configured_submodule_row( )) } +fn resolve_submodule_logical_name(repo: &gix::Repository, path: &Path) -> Result> { + let Some(submodules) = repo + .submodules() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodules: {e}"))))? + else { + return Ok(None); + }; + + for submodule in submodules { + let relative_path = submodule + .path() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule path: {e}")))) + .and_then(|path| pathbuf_from_gix_path(path.as_ref()))?; + if relative_path == path { + return pathbuf_from_gix_path(submodule.name()).map(Some); + } + } + + Ok(None) +} + +fn cleanup_removed_submodule_metadata( + workdir: &Path, + git_dir: &Path, + logical_name: &Path, +) -> Result<()> { + remove_local_submodule_config_section_if_present(workdir, logical_name)?; + remove_submodule_git_dir(git_dir, logical_name)?; + Ok(()) +} + +fn remove_local_submodule_config_section_if_present( + workdir: &Path, + logical_name: &Path, +) -> Result<()> { + let Some(logical_name) = logical_name.to_str() else { + return Err(Error::new(ErrorKind::Unsupported( + "submodule logical name is not valid UTF-8", + ))); + }; + let section = format!("submodule.{logical_name}"); + + let mut cmd = git_workdir_cmd_for(workdir); + cmd.arg("config") + .arg("--local") + .arg("--remove-section") + .arg(§ion); + let output = cmd + .output() + .map_err(|err| Error::new(ErrorKind::Io(err.kind())))?; + if output.status.success() { + return Ok(()); + } + + let stderr = bytes_to_text_preserving_utf8(&output.stderr); + if stderr.contains("no such section") { + return Ok(()); + } + + Err(Error::new(ErrorKind::Backend(format!( + "git config --local --remove-section {section} failed: {}", + stderr.trim() + )))) +} + +fn remove_submodule_git_dir(git_dir: &Path, logical_name: &Path) -> Result<()> { + let modules_root = git_dir.join("modules"); + let module_dir = modules_root.join(logical_name); + if !module_dir.exists() { + return Ok(()); + } + + fs::remove_dir_all(&module_dir).map_err(|e| { + Error::new(ErrorKind::Backend(format!( + "remove submodule git dir {}: {e}", + module_dir.display() + ))) + })?; + prune_empty_module_parent_dirs(&modules_root, &module_dir) +} + +fn prune_empty_module_parent_dirs(modules_root: &Path, removed_dir: &Path) -> Result<()> { + let mut current = removed_dir.parent(); + while let Some(dir) = current { + if dir == modules_root || !dir.starts_with(modules_root) { + break; + } + + let mut entries = fs::read_dir(dir).map_err(|e| { + Error::new(ErrorKind::Backend(format!( + "read module metadata dir {}: {e}", + dir.display() + ))) + })?; + match entries.next() { + None => { + fs::remove_dir(dir).map_err(|e| { + Error::new(ErrorKind::Backend(format!( + "remove empty module metadata dir {}: {e}", + dir.display() + ))) + })?; + current = dir.parent(); + } + Some(Ok(_)) => break, + Some(Err(e)) => { + return Err(Error::new(ErrorKind::Backend(format!( + "read module metadata dir entry {}: {e}", + dir.display() + )))); + } + } + } + Ok(()) +} + fn collect_gitlinks(repo: &gix::Repository) -> Result> { let index = repo .index_or_load_from_head_or_empty() diff --git a/crates/gitcomet-git-gix/tests/submodules_integration.rs b/crates/gitcomet-git-gix/tests/submodules_integration.rs index 896a12c7..7b17ffa6 100644 --- a/crates/gitcomet-git-gix/tests/submodules_integration.rs +++ b/crates/gitcomet-git-gix/tests/submodules_integration.rs @@ -43,6 +43,78 @@ fn git_stdout(repo: &Path, args: &[&str]) -> String { .to_string() } +fn local_submodule_config_entries(repo: &Path) -> Vec { + let output = git_output(repo, &["config", "--list", "--local"]); + assert!(output.status.success(), "git config --list --local failed"); + String::from_utf8(output.stdout) + .expect("git stdout is utf-8") + .lines() + .filter(|line| line.starts_with("submodule.")) + .map(ToOwned::to_owned) + .collect() +} + +fn add_submodule_raw(parent_repo: &Path, sub_repo: &Path, path: &Path, name: Option<&str>) { + let mut cmd = git_command(); + cmd.arg("-C") + .arg(parent_repo) + .arg("-c") + .arg("protocol.file.allow=always") + .arg("submodule") + .arg("add"); + if let Some(name) = name { + cmd.arg("--name").arg(name); + } + let status = cmd + .arg(sub_repo) + .arg(path) + .status() + .expect("git submodule add to run"); + assert!(status.success(), "git submodule add failed"); +} + +fn run_git_with_path(repo: &Path, args: &[&str], path: &Path) { + let status = git_command() + .arg("-C") + .arg(repo) + .args(args) + .arg(path) + .status() + .expect("git command to run"); + assert!(status.success(), "git {:?} {:?} failed", args, path); +} + +fn create_stale_submodule_git_dir( + parent_repo: &Path, + sub_repo: &Path, + path: &Path, + name: Option<&str>, +) { + add_submodule_raw(parent_repo, sub_repo, path, name); + run_git( + parent_repo, + &[ + "-c", + "commit.gpgsign=false", + "commit", + "-m", + "add submodule", + ], + ); + run_git_with_path(parent_repo, &["submodule", "deinit", "-f", "--"], path); + run_git_with_path(parent_repo, &["rm", "-f", "--"], path); + run_git( + parent_repo, + &[ + "-c", + "commit.gpgsign=false", + "commit", + "-m", + "remove submodule", + ], + ); +} + fn init_repo_with_seed(repo: &Path, file: &str, contents: &str, message: &str) { run_git(repo, &["init"]); run_git(repo, &["config", "user.email", "you@example.com"]); @@ -587,6 +659,9 @@ fn submodule_add_update_remove_round_trip() { .expect("list submodules after remove"); assert!(listed_after_remove.is_empty()); assert!(!parent_repo.join("mods/sub-one").exists()); + assert!(local_submodule_config_entries(&parent_repo).is_empty()); + assert!(!parent_repo.join(".git/modules/mods/sub-one").exists()); + assert!(!parent_repo.join(".git/modules/mods").exists()); } #[test] @@ -738,9 +813,15 @@ fn add_submodule_supports_custom_logical_name_for_local_git_dir_collision() { init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + let submodule_path = Path::new("sm"); + create_stale_submodule_git_dir(&parent_repo, &sub_repo, submodule_path, None); + assert!( + parent_repo.join(".git/modules/sm").exists(), + "expected stale local submodule git dir" + ); + let backend = GixBackend; let opened = backend.open(&parent_repo).expect("open parent repository"); - let submodule_path = Path::new("sm"); let approved_sources = match opened .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), submodule_path) .expect("check local submodule trust") @@ -749,45 +830,6 @@ fn add_submodule_supports_custom_logical_name_for_local_git_dir_collision() { other => panic!("expected trust prompt for local submodule, got {other:?}"), }; - opened - .add_submodule_with_output( - sub_repo.to_string_lossy().as_ref(), - submodule_path, - None, - None, - false, - &approved_sources, - ) - .expect("add initial submodule"); - run_git( - &parent_repo, - &[ - "-c", - "commit.gpgsign=false", - "commit", - "-m", - "add submodule", - ], - ); - - opened - .remove_submodule_with_output(submodule_path) - .expect("remove submodule"); - run_git( - &parent_repo, - &[ - "-c", - "commit.gpgsign=false", - "commit", - "-m", - "remove submodule", - ], - ); - assert!( - parent_repo.join(".git/modules/sm").exists(), - "expected stale local submodule git dir" - ); - let err = opened .add_submodule_with_output( sub_repo.to_string_lossy().as_ref(), @@ -795,7 +837,7 @@ fn add_submodule_supports_custom_logical_name_for_local_git_dir_collision() { None, None, false, - &[], + &approved_sources, ) .expect_err("add without custom name or force should fail"); let rendered = err.to_string(); @@ -813,7 +855,7 @@ fn add_submodule_supports_custom_logical_name_for_local_git_dir_collision() { None, Some("sm-renamed"), false, - &[], + &approved_sources, ) .expect("add submodule with custom logical name"); assert_eq!(add_output.exit_code, Some(0)); @@ -835,9 +877,15 @@ fn add_submodule_supports_force_for_local_git_dir_collision() { init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + let submodule_path = Path::new("sm"); + create_stale_submodule_git_dir(&parent_repo, &sub_repo, submodule_path, None); + assert!( + parent_repo.join(".git/modules/sm").exists(), + "expected stale local submodule git dir" + ); + let backend = GixBackend; let opened = backend.open(&parent_repo).expect("open parent repository"); - let submodule_path = Path::new("sm"); let approved_sources = match opened .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), submodule_path) .expect("check local submodule trust") @@ -846,30 +894,40 @@ fn add_submodule_supports_force_for_local_git_dir_collision() { other => panic!("expected trust prompt for local submodule, got {other:?}"), }; - opened + let add_output = opened .add_submodule_with_output( sub_repo.to_string_lossy().as_ref(), submodule_path, None, None, - false, + true, &approved_sources, ) - .expect("add initial submodule"); - run_git( + .expect("add submodule with force"); + assert_eq!(add_output.exit_code, Some(0)); +} + +#[test] +fn remove_submodule_cleans_custom_logical_name_metadata() { + if !require_git_shell_for_submodule_tests() { + return; + } + let dir = tempfile::tempdir().expect("create tempdir"); + let root = dir.path(); + + let sub_repo = root.join("sub"); + let parent_repo = root.join("parent"); + fs::create_dir_all(&sub_repo).expect("create sub repository directory"); + fs::create_dir_all(&parent_repo).expect("create parent repository directory"); + + init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); + init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + add_submodule_raw( &parent_repo, - &[ - "-c", - "commit.gpgsign=false", - "commit", - "-m", - "add submodule", - ], + &sub_repo, + Path::new("mods/sub"), + Some("custom"), ); - - opened - .remove_submodule_with_output(submodule_path) - .expect("remove submodule"); run_git( &parent_repo, &[ @@ -877,23 +935,16 @@ fn add_submodule_supports_force_for_local_git_dir_collision() { "commit.gpgsign=false", "commit", "-m", - "remove submodule", + "add submodule", ], ); - assert!( - parent_repo.join(".git/modules/sm").exists(), - "expected stale local submodule git dir" - ); - let add_output = opened - .add_submodule_with_output( - sub_repo.to_string_lossy().as_ref(), - submodule_path, - None, - None, - true, - &[], - ) - .expect("add submodule with force"); - assert_eq!(add_output.exit_code, Some(0)); + let backend = GixBackend; + let opened = backend.open(&parent_repo).expect("open parent repository"); + let remove_output = opened + .remove_submodule_with_output(Path::new("mods/sub")) + .expect("remove submodule"); + assert_eq!(remove_output.exit_code, Some(0)); + assert!(local_submodule_config_entries(&parent_repo).is_empty()); + assert!(!parent_repo.join(".git/modules/custom").exists()); } From 115a2e691fca9f395cdcf92d7370287038796247 Mon Sep 17 00:00:00 2001 From: Roope Airinen Date: Fri, 10 Apr 2026 13:33:37 +0300 Subject: [PATCH 5/6] fmt and clippy --- .../gitcomet-git-gix/src/repo/submodules.rs | 32 ++++------- crates/gitcomet-state/src/store/effects.rs | 16 +++--- .../src/store/effects/repo_commands.rs | 27 +++++++--- crates/gitcomet-ui-gpui/src/app.rs | 1 + .../panels/popover/submodule_trust_confirm.rs | 6 +-- .../src/view/panes/sidebar.rs | 34 +++++++----- .../src/view/rows/benchmarks/diff_fixtures.rs | 53 +++++++++++++++++++ .../src/view/rows/benchmarks/repo_history.rs | 8 +++ .../view/rows/benchmarks/runtime_fixtures.rs | 38 ++++++++++--- .../view/rows/benchmarks/scroll_fixtures.rs | 1 + .../view/rows/benchmarks/search_fixtures.rs | 32 +++++++++++ .../view/rows/benchmarks/status_fixtures.rs | 2 + .../src/view/rows/benchmarks/text_fixtures.rs | 4 ++ 13 files changed, 194 insertions(+), 60 deletions(-) diff --git a/crates/gitcomet-git-gix/src/repo/submodules.rs b/crates/gitcomet-git-gix/src/repo/submodules.rs index c1b75e2f..e9c7ea7b 100644 --- a/crates/gitcomet-git-gix/src/repo/submodules.rs +++ b/crates/gitcomet-git-gix/src/repo/submodules.rs @@ -114,13 +114,7 @@ impl GixRepo { persist_submodule_trust_approvals(&trust_root, approved_sources)?; let mut outputs = Vec::new(); - update_repo_submodules_recursive( - &repo, - &trust_root, - Path::new(""), - approved_sources, - &mut outputs, - )?; + update_repo_submodules_recursive(&repo, &trust_root, Path::new(""), &mut outputs)?; if outputs.is_empty() { Ok(CommandOutput::empty_success( @@ -266,10 +260,9 @@ fn collect_repo_untrusted_submodule_sources( let full_path = prefix.join(&relative_path); if let Some(target) = trust_target_from_submodule(current_workdir, &full_path, &submodule)? + && !submodule_source_trusted(trust_root, &target)? { - if !submodule_source_trusted(trust_root, &target)? { - out.insert(full_path.clone(), target); - } + out.insert(full_path.clone(), target); } if let Some(nested_repo) = open_configured_submodule_repo(&submodule)? { @@ -284,7 +277,6 @@ fn update_repo_submodules_recursive( repo: &gix::Repository, trust_root: &Path, prefix: &Path, - approved_sources: &[SubmoduleTrustTarget], outputs: &mut Vec, ) -> Result<()> { let Some(submodules) = repo @@ -323,13 +315,7 @@ fn update_repo_submodules_recursive( )?); if let Some(nested_repo) = open_gitlink_repo(repo, &relative_path)? { - update_repo_submodules_recursive( - &nested_repo, - trust_root, - &full_path, - approved_sources, - outputs, - )?; + update_repo_submodules_recursive(&nested_repo, trust_root, &full_path, outputs)?; } } @@ -621,11 +607,11 @@ fn resolve_local_file_transport_path( url: &gix::Url, ) -> Result { let mut path = pathbuf_from_gix_path(url.path.as_ref())?; - if let Some(host) = url.host.as_deref() { - if !host.eq_ignore_ascii_case("localhost") { - let host_path = PathBuf::from(format!("//{host}")).join(&path); - path = host_path; - } + if let Some(host) = url.host.as_deref() + && !host.eq_ignore_ascii_case("localhost") + { + let host_path = PathBuf::from(format!("//{host}")).join(&path); + path = host_path; } if path.is_relative() { path = current_repo_workdir.join(path); diff --git a/crates/gitcomet-state/src/store/effects.rs b/crates/gitcomet-state/src/store/effects.rs index 8836a308..dd460b09 100644 --- a/crates/gitcomet-state/src/store/effects.rs +++ b/crates/gitcomet-state/src/store/effects.rs @@ -285,13 +285,15 @@ pub(super) fn schedule_effect( repos, msg_tx, repo_id, - url, - path, - branch, - name, - force, - approved_sources, - auth, + repo_commands::AddSubmoduleRequest { + url, + path, + branch, + name, + force, + approved_sources, + auth, + }, ); } Effect::UpdateSubmodules { diff --git a/crates/gitcomet-state/src/store/effects/repo_commands.rs b/crates/gitcomet-state/src/store/effects/repo_commands.rs index 7f7b0275..9eda38e5 100644 --- a/crates/gitcomet-state/src/store/effects/repo_commands.rs +++ b/crates/gitcomet-state/src/store/effects/repo_commands.rs @@ -79,6 +79,16 @@ fn run_with_git_auth( } } +pub(super) struct AddSubmoduleRequest { + pub(super) url: String, + pub(super) path: PathBuf, + pub(super) branch: Option, + pub(super) name: Option, + pub(super) force: bool, + pub(super) approved_sources: Vec, + pub(super) auth: Option, +} + pub(super) fn schedule_save_worktree_file( executor: &TaskExecutor, repos: &RepoMap, @@ -231,14 +241,17 @@ pub(super) fn schedule_add_submodule( repos: &RepoMap, msg_tx: mpsc::Sender, repo_id: RepoId, - url: String, - path: PathBuf, - branch: Option, - name: Option, - force: bool, - approved_sources: Vec, - auth: Option, + request: AddSubmoduleRequest, ) { + let AddSubmoduleRequest { + url, + path, + branch, + name, + force, + approved_sources, + auth, + } = request; let command_url = url.clone(); let command_path = path.clone(); let command_branch = branch.clone(); diff --git a/crates/gitcomet-ui-gpui/src/app.rs b/crates/gitcomet-ui-gpui/src/app.rs index 0ab09791..c251fa86 100644 --- a/crates/gitcomet-ui-gpui/src/app.rs +++ b/crates/gitcomet-ui-gpui/src/app.rs @@ -213,6 +213,7 @@ pub(crate) fn show_window_system_menu(window: &Window, position: Point) window.show_window_menu(position); } +#[cfg(any(test, target_os = "windows"))] fn window_menu_position(position: Point, scale_factor: f32) -> (i32, i32) { ( (f32::from(position.x) * scale_factor).round() as i32, diff --git a/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs b/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs index bec6c0f6..8181b0a0 100644 --- a/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs +++ b/crates/gitcomet-ui-gpui/src/view/panels/popover/submodule_trust_confirm.rs @@ -80,7 +80,7 @@ pub(super) fn panel( }), ), ) - .children(sources.iter().cloned().map(|source| { + .children(sources.into_iter().map(|source| { div() .px_2() .pb_1() @@ -100,7 +100,7 @@ pub(super) fn panel( div() .text_sm() .font_family(crate::font_preferences::EDITOR_MONOSPACE_FONT_FAMILY) - .child(source.display_source.clone()), + .child(source.display_source), ) .child( div() @@ -182,7 +182,7 @@ pub(super) fn panel( }); this.submodule_path_input.update(cx, |input, cx| { input.set_theme(theme, cx); - input.set_text(&path.display().to_string(), cx); + input.set_text(path.display().to_string(), cx); cx.notify(); }); this.submodule_branch_input.update(cx, move |input, cx| { diff --git a/crates/gitcomet-ui-gpui/src/view/panes/sidebar.rs b/crates/gitcomet-ui-gpui/src/view/panes/sidebar.rs index 83fb2784..ea40f048 100644 --- a/crates/gitcomet-ui-gpui/src/view/panes/sidebar.rs +++ b/crates/gitcomet-ui-gpui/src/view/panes/sidebar.rs @@ -543,9 +543,11 @@ mod tests { #[test] fn sidebar_notify_fingerprint_tracks_open_repo_workdirs() { - let mut state = AppState::default(); - state.active_repo = Some(RepoId(1)); - state.repos.push(repo_state(RepoId(1), "/tmp/repo")); + let mut state = AppState { + repos: vec![repo_state(RepoId(1), "/tmp/repo")], + active_repo: Some(RepoId(1)), + ..AppState::default() + }; let initial = SidebarNotifyFingerprint::from_state(&state); @@ -634,15 +636,23 @@ mod tests { #[test] fn sidebar_notify_fingerprint_ignores_repo_tab_order() { - let mut state_a = AppState::default(); - state_a.active_repo = Some(RepoId(1)); - state_a.repos.push(repo_state(RepoId(1), "/tmp/repo")); - state_a.repos.push(repo_state(RepoId(2), "/tmp/repo-wt")); - - let mut state_b = AppState::default(); - state_b.active_repo = Some(RepoId(1)); - state_b.repos.push(repo_state(RepoId(2), "/tmp/repo-wt")); - state_b.repos.push(repo_state(RepoId(1), "/tmp/repo")); + let state_a = AppState { + repos: vec![ + repo_state(RepoId(1), "/tmp/repo"), + repo_state(RepoId(2), "/tmp/repo-wt"), + ], + active_repo: Some(RepoId(1)), + ..AppState::default() + }; + + let state_b = AppState { + repos: vec![ + repo_state(RepoId(2), "/tmp/repo-wt"), + repo_state(RepoId(1), "/tmp/repo"), + ], + active_repo: Some(RepoId(1)), + ..AppState::default() + }; assert_eq!( SidebarNotifyFingerprint::from_state(&state_a), diff --git a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/diff_fixtures.rs b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/diff_fixtures.rs index 752fe36e..3785dcd5 100644 --- a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/diff_fixtures.rs +++ b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/diff_fixtures.rs @@ -1727,6 +1727,8 @@ pub struct RepoTabDragFixture { tab_count: usize, tab_width_px: f32, baseline: AppState, + #[cfg(test)] + pub hit_test_steps: Vec, } /// Sidecar metrics for repo tab drag benchmarks. @@ -1738,6 +1740,17 @@ pub struct RepoTabDragMetrics { pub noop_reorders: u64, } +#[cfg(test)] +pub struct RepoTabDragHitTestTarget { + pub repo_id: RepoId, +} + +#[cfg(test)] +pub struct RepoTabDragHitTestStep { + pub target: RepoTabDragHitTestTarget, + pub insert_before: Option, +} + impl RepoTabDragFixture { pub fn new(tab_count: usize) -> Self { let commits = build_synthetic_commits(10); @@ -1760,6 +1773,8 @@ impl RepoTabDragFixture { .collect(); let active = repos.first().map(|r| r.id); + #[cfg(test)] + let hit_test_steps = build_repo_tab_drag_hit_test_steps(&repos, tab_count, 120.0); Self { tab_count, tab_width_px: 120.0, @@ -1770,7 +1785,10 @@ impl RepoTabDragFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, + #[cfg(test)] + hit_test_steps, } } @@ -1885,6 +1903,41 @@ impl RepoTabDragFixture { } } +#[cfg(test)] +fn build_repo_tab_drag_hit_test_steps( + repos: &[RepoState], + tab_count: usize, + tab_width_px: f32, +) -> Vec { + let steps = tab_count * 3; + let total_bar_width = tab_count as f32 * tab_width_px; + let mut out = Vec::with_capacity(steps); + + for step in 0..steps { + let frac = (step as f32) / (steps.max(1) as f32); + let cursor_x = frac * total_bar_width; + let tab_ix = (cursor_x / tab_width_px) as usize; + let tab_ix = tab_ix.min(tab_count.saturating_sub(1)); + let target_repo_id = repos[tab_ix].id; + let tab_left = tab_ix as f32 * tab_width_px; + let tab_center = tab_left + tab_width_px / 2.0; + let insert_before = if cursor_x <= tab_center { + Some(target_repo_id) + } else { + repos.get(tab_ix + 1).map(|repo| repo.id) + }; + + out.push(RepoTabDragHitTestStep { + target: RepoTabDragHitTestTarget { + repo_id: target_repo_id, + }, + insert_before, + }); + } + + out +} + pub struct PatchDiffSearchQueryUpdateFixture { diff_rows: Vec, click_kinds: Vec, diff --git a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/repo_history.rs b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/repo_history.rs index 6abb4e4c..c4a47b3b 100644 --- a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/repo_history.rs +++ b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/repo_history.rs @@ -997,6 +997,7 @@ impl RepoSwitchFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, target_repo_id: RepoId(1), } @@ -1038,6 +1039,7 @@ impl RepoSwitchFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, target_repo_id: RepoId(2), } @@ -1079,6 +1081,7 @@ impl RepoSwitchFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, target_repo_id: RepoId(2), } @@ -1122,6 +1125,7 @@ impl RepoSwitchFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, target_repo_id: RepoId(u64::try_from(TAB_COUNT).unwrap_or(u64::MAX)), } @@ -1165,6 +1169,7 @@ impl RepoSwitchFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, target_repo_id: RepoId(u64::try_from(REPO_COUNT).unwrap_or(u64::MAX)), } @@ -1211,6 +1216,7 @@ impl RepoSwitchFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, target_repo_id: RepoId(2), } @@ -1257,6 +1263,7 @@ impl RepoSwitchFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, target_repo_id: RepoId(2), } @@ -1311,6 +1318,7 @@ impl RepoSwitchFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, target_repo_id: RepoId(2), } diff --git a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/runtime_fixtures.rs b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/runtime_fixtures.rs index 33a33666..30b48ca5 100644 --- a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/runtime_fixtures.rs +++ b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/runtime_fixtures.rs @@ -727,9 +727,8 @@ fn idle_sample_steps(window: Duration, interval: Duration) -> usize { #[cfg(target_os = "linux")] #[cfg(any(test, feature = "benchmarks"))] fn current_cpu_runtime_ns() -> Option { - let schedstat = fs::read_to_string("/proc/self/schedstat").ok()?; - let runtime_ns = schedstat.split_whitespace().next()?; - runtime_ns.parse::().ok() + let schedstat = fs::read("/proc/self/schedstat").ok()?; + parse_first_u64_ascii_token(&schedstat) } #[cfg(not(target_os = "linux"))] @@ -741,11 +740,8 @@ fn current_cpu_runtime_ns() -> Option { #[cfg(target_os = "linux")] #[cfg(any(test, feature = "benchmarks"))] fn current_rss_kib() -> Option { - let status = fs::read_to_string("/proc/self/status").ok()?; - status.lines().find_map(|line| { - let value = line.strip_prefix("VmRSS:")?.split_whitespace().next()?; - value.parse::().ok() - }) + let status = fs::read("/proc/self/status").ok()?; + parse_vmrss_kib(&status) } #[cfg(not(target_os = "linux"))] @@ -754,6 +750,32 @@ fn current_rss_kib() -> Option { None } +#[cfg(target_os = "linux")] +#[cfg(any(test, feature = "benchmarks"))] +pub(crate) fn parse_first_u64_ascii_token(bytes: &[u8]) -> Option { + let start = bytes.iter().position(u8::is_ascii_digit)?; + let digits = &bytes[start..]; + let end = digits + .iter() + .position(|byte| !byte.is_ascii_digit()) + .unwrap_or(digits.len()); + std::str::from_utf8(&digits[..end]) + .ok()? + .parse::() + .ok() +} + +#[cfg(target_os = "linux")] +#[cfg(any(test, feature = "benchmarks"))] +pub(crate) fn parse_vmrss_kib(status: &[u8]) -> Option { + for line in status.split(|byte| *byte == b'\n') { + if let Some(rest) = line.strip_prefix(b"VmRSS:") { + return parse_first_u64_ascii_token(rest); + } + } + None +} + // --------------------------------------------------------------------------- // Clipboard — copy from diff, paste into commit message, selection range // --------------------------------------------------------------------------- diff --git a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/scroll_fixtures.rs b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/scroll_fixtures.rs index 2601bbc8..698c412e 100644 --- a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/scroll_fixtures.rs +++ b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/scroll_fixtures.rs @@ -872,6 +872,7 @@ impl KeyboardStageUnstageToggleFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, paths, toggle_events: toggle_events.max(1), diff --git a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/search_fixtures.rs b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/search_fixtures.rs index e92e21c7..78dafa51 100644 --- a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/search_fixtures.rs +++ b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/search_fixtures.rs @@ -365,6 +365,18 @@ impl CommitSearchFilterFixture { } seen.len() } + + /// Number of distinct lowercased message trigrams in the fixture. + #[cfg(test)] + pub fn distinct_message_trigrams(&self) -> usize { + let mut seen = std::collections::HashSet::new(); + for summary in &self.summaries_lower { + for trigram in summary.as_bytes().windows(3) { + seen.insert([trigram[0], trigram[1], trigram[2]]); + } + } + seen.len() + } } /// Metrics emitted as sidecar JSON for in-diff text search benchmarks. @@ -1167,6 +1179,26 @@ impl FileFuzzyFindFixture { pub fn total_files(&self) -> usize { self.total_files } + + #[cfg(test)] + pub fn run_find_without_ordered_pair_prefilter(&self, query: &str) -> u64 { + let Some(query) = AsciiCaseInsensitiveSubsequenceNeedle::new(query.trim()) else { + return 0; + }; + + let mut h = FxHasher::default(); + let mut matches_found = 0u64; + for (ix, path) in self.paths.iter().enumerate() { + if query.is_match(path.lowercase_bytes.as_ref()) { + ix.hash(&mut h); + path.len.hash(&mut h); + matches_found = matches_found.saturating_add(1); + } + } + matches_found.hash(&mut h); + self.total_files.hash(&mut h); + h.finish() + } } /// Build a deterministic corpus of `count` synthetic file paths with realistic diff --git a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/status_fixtures.rs b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/status_fixtures.rs index 94a26af8..d39f5dc3 100644 --- a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/status_fixtures.rs +++ b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/status_fixtures.rs @@ -316,6 +316,7 @@ impl StatusSelectDiffOpenFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, diff_target: DiffTarget::WorkingTree { path: target_path, @@ -345,6 +346,7 @@ impl StatusSelectDiffOpenFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, diff_target: DiffTarget::WorkingTree { path: target_path, diff --git a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/text_fixtures.rs b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/text_fixtures.rs index 5adf9d58..1d4123a9 100644 --- a/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/text_fixtures.rs +++ b/crates/gitcomet-ui-gpui/src/view/rows/benchmarks/text_fixtures.rs @@ -53,6 +53,7 @@ impl StagingFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, paths, scenario: StagingScenario::StageAll, @@ -85,6 +86,7 @@ impl StagingFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, paths, scenario: StagingScenario::UnstageAll, @@ -118,6 +120,7 @@ impl StagingFixture { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }, paths, scenario: StagingScenario::Interleaved, @@ -425,6 +428,7 @@ fn build_undo_redo_baseline(region_count: usize) -> (AppState, RepoPath) { notifications: Vec::new(), banner_error: None, auth_prompt: None, + submodule_trust_prompt: None, }; (baseline, conflict_path) From 8ccd3430dc3e7d8d086fa298dde0cb238cbe9a53 Mon Sep 17 00:00:00 2001 From: Roope Airinen Date: Fri, 10 Apr 2026 16:12:00 +0300 Subject: [PATCH 6/6] add overflow to error hints, submodule error now cleans metadata --- .../gitcomet-git-gix/src/repo/submodules.rs | 237 +++++++++++++++++- .../tests/submodules_integration.rs | 222 ++++++++++++++++ crates/gitcomet-ui-gpui/src/view/mod.rs | 18 ++ crates/gitcomet-ui-gpui/src/view/tests.rs | 23 ++ 4 files changed, 495 insertions(+), 5 deletions(-) diff --git a/crates/gitcomet-git-gix/src/repo/submodules.rs b/crates/gitcomet-git-gix/src/repo/submodules.rs index e9c7ea7b..c29cd450 100644 --- a/crates/gitcomet-git-gix/src/repo/submodules.rs +++ b/crates/gitcomet-git-gix/src/repo/submodules.rs @@ -4,7 +4,7 @@ use crate::util::{ bytes_to_text_preserving_utf8, git_workdir_cmd_for, run_git_simple, run_git_with_output, }; use gitcomet_core::domain::{CommitId, Submodule, SubmoduleStatus}; -use gitcomet_core::error::{Error, ErrorKind}; +use gitcomet_core::error::{Error, ErrorKind, GitFailure}; use gitcomet_core::path_utils::canonicalize_or_original; use gitcomet_core::services::{ CommandOutput, Result, SubmoduleTrustDecision, SubmoduleTrustTarget, @@ -14,6 +14,8 @@ use std::collections::BTreeMap; use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; +use std::thread; +use std::time::Duration; fn allow_file_submodule_transport(cmd: &mut Command) { // `git submodule` blocks local-path remotes unless `protocol.file.allow` is enabled. @@ -76,6 +78,7 @@ impl GixRepo { ) -> Result { let repo = self.reopen_repo()?; let trust_root = repo_workdir_for_submodule_trust(&repo); + let git_dir = repo.git_dir().to_path_buf(); persist_submodule_trust_approvals(trust_root, approved_sources)?; let mut cmd = self.git_workdir_cmd(); @@ -85,6 +88,9 @@ impl GixRepo { } allow_file_submodule_transport(&mut cmd); } + let logical_name = name + .map(PathBuf::from) + .unwrap_or_else(|| path.to_path_buf()); cmd.arg("submodule").arg("add"); let mut command = "git submodule add".to_string(); @@ -102,7 +108,16 @@ impl GixRepo { } cmd.arg(url).arg(path); command.push_str(&format!(" {url} {}", path.display())); - run_git_with_output(cmd, &command) + match run_git_with_output(cmd, &command) { + Ok(output) => Ok(output), + Err(err) => Err(cleanup_failed_submodule_add_error( + trust_root, + &git_dir, + path, + &logical_name, + err, + )), + } } pub(super) fn update_submodules_with_output_impl( @@ -392,6 +407,84 @@ fn resolve_submodule_logical_name(repo: &gix::Repository, path: &Path) -> Result Ok(None) } +fn cleanup_failed_submodule_add_error( + workdir: &Path, + git_dir: &Path, + path: &Path, + logical_name: &Path, + err: Error, +) -> Error { + let clone_only_state = match failed_submodule_add_left_clone_only_state(workdir, path) { + Ok(value) => value, + Err(probe_err) => { + return append_failed_submodule_add_note( + err, + &format!("GitComet could not inspect failed submodule add state: {probe_err}"), + ); + } + }; + + if !clone_only_state { + return err; + } + + match cleanup_failed_submodule_add_leftovers(workdir, git_dir, path, logical_name) { + Ok(()) => err, + Err(cleanup_err) => append_failed_submodule_add_note( + err, + &format!("Cleanup after failed submodule add also failed: {cleanup_err}"), + ), + } +} + +fn failed_submodule_add_left_clone_only_state(workdir: &Path, path: &Path) -> Result { + let repo = gix::open(workdir).map_err(|e| { + Error::new(ErrorKind::Backend(format!( + "open repo after failed submodule add {}: {e}", + workdir.display() + ))) + })?; + Ok(!submodule_path_registered(&repo, path)?) +} + +fn submodule_path_registered(repo: &gix::Repository, path: &Path) -> Result { + if configured_submodule_path_exists(repo, path)? { + return Ok(true); + } + Ok(collect_gitlinks(repo)?.contains_key(path)) +} + +fn configured_submodule_path_exists(repo: &gix::Repository, path: &Path) -> Result { + let Some(submodules) = repo + .submodules() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodules: {e}"))))? + else { + return Ok(false); + }; + + for submodule in submodules { + let relative_path = submodule + .path() + .map_err(|e| Error::new(ErrorKind::Backend(format!("gix submodule path: {e}")))) + .and_then(|path| pathbuf_from_gix_path(path.as_ref()))?; + if relative_path == path { + return Ok(true); + } + } + + Ok(false) +} + +fn cleanup_failed_submodule_add_leftovers( + workdir: &Path, + git_dir: &Path, + path: &Path, + logical_name: &Path, +) -> Result<()> { + remove_failed_submodule_checkout(workdir, git_dir, path, logical_name)?; + cleanup_removed_submodule_metadata(workdir, git_dir, logical_name) +} + fn cleanup_removed_submodule_metadata( workdir: &Path, git_dir: &Path, @@ -402,6 +495,100 @@ fn cleanup_removed_submodule_metadata( Ok(()) } +fn remove_failed_submodule_checkout( + workdir: &Path, + git_dir: &Path, + submodule_path: &Path, + logical_name: &Path, +) -> Result<()> { + let checkout_path = submodule_worktree_path(workdir, submodule_path)?; + if !checkout_path.exists() { + return Ok(()); + } + + let expected_git_dir = canonicalize_or_original(git_dir.join("modules").join(logical_name)); + let Some(actual_git_dir) = checkout_git_dir_reference(&checkout_path)? else { + return Err(Error::new(ErrorKind::Backend(format!( + "refusing to remove failed submodule checkout {} because it is not linked to {}", + checkout_path.display(), + expected_git_dir.display() + )))); + }; + + if actual_git_dir != expected_git_dir { + return Err(Error::new(ErrorKind::Backend(format!( + "refusing to remove failed submodule checkout {} because it points to {} instead of {}", + checkout_path.display(), + actual_git_dir.display(), + expected_git_dir.display() + )))); + } + + fs::remove_dir_all(&checkout_path).map_err(|e| { + Error::new(ErrorKind::Backend(format!( + "remove failed submodule checkout {}: {e}", + checkout_path.display() + ))) + }) +} + +fn submodule_worktree_path(workdir: &Path, submodule_path: &Path) -> Result { + if submodule_path.is_absolute() { + if submodule_path.starts_with(workdir) { + return Ok(submodule_path.to_path_buf()); + } + return Err(Error::new(ErrorKind::Backend(format!( + "refusing to clean failed submodule add outside repository workdir: {}", + submodule_path.display() + )))); + } + Ok(workdir.join(submodule_path)) +} + +fn checkout_git_dir_reference(checkout_path: &Path) -> Result> { + let dot_git = checkout_path.join(".git"); + let metadata = match fs::metadata(&dot_git) { + Ok(metadata) => metadata, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(err) => return Err(Error::new(ErrorKind::Io(err.kind()))), + }; + + if metadata.is_dir() { + return Ok(Some(canonicalize_or_original(dot_git))); + } + + let bytes = fs::read(&dot_git).map_err(|e| Error::new(ErrorKind::Io(e.kind())))?; + let text = bytes_to_text_preserving_utf8(&bytes); + let Some(git_dir) = text.strip_prefix("gitdir:") else { + return Ok(None); + }; + + let git_dir = PathBuf::from(git_dir.trim()); + let resolved = if git_dir.is_absolute() { + git_dir + } else { + checkout_path.join(git_dir) + }; + Ok(Some(canonicalize_or_original(resolved))) +} + +fn append_failed_submodule_add_note(err: Error, note: &str) -> Error { + match err.kind() { + ErrorKind::Git(failure) => Error::new(ErrorKind::Git(GitFailure::new( + failure.command(), + failure.id(), + failure.exit_code(), + failure.stdout().to_vec(), + failure.stderr().to_vec(), + Some(match failure.detail() { + Some(detail) if !detail.is_empty() => format!("{detail}\n\n{note}"), + _ => note.to_string(), + }), + ))), + _ => Error::new(ErrorKind::Backend(format!("{err}\n\n{note}"))), + } +} + fn remove_local_submodule_config_section_if_present( workdir: &Path, logical_name: &Path, @@ -623,19 +810,59 @@ fn persist_submodule_trust_approvals( trust_root: &Path, approved_sources: &[SubmoduleTrustTarget], ) -> Result<()> { + const GIT_CONFIG_LOCK_RETRIES: usize = 6; + for source in approved_sources { let key = submodule_file_transport_consent_key(trust_root, &source.local_source_path); if git_config_get_bool_global(trust_root, &key)?.unwrap_or(false) { continue; } - let mut cmd = git_workdir_cmd_for(trust_root); - cmd.arg("config").arg("--global").arg(&key).arg("true"); - run_git_simple(cmd, &format!("git config --global {key} true"))?; + let mut last_err = None; + for attempt in 0..GIT_CONFIG_LOCK_RETRIES { + let mut cmd = git_workdir_cmd_for(trust_root); + cmd.arg("config").arg("--global").arg(&key).arg("true"); + match run_git_simple(cmd, &format!("git config --global {key} true")) { + Ok(()) => { + last_err = None; + break; + } + Err(err) => { + if git_config_get_bool_global(trust_root, &key)?.unwrap_or(false) { + last_err = None; + break; + } + let retryable = + attempt + 1 < GIT_CONFIG_LOCK_RETRIES && is_git_config_lock_error(&err); + last_err = Some(err); + if retryable { + thread::sleep(Duration::from_millis(25)); + continue; + } + break; + } + } + } + if let Some(err) = last_err { + return Err(err); + } } Ok(()) } +fn is_git_config_lock_error(err: &Error) -> bool { + let text = match err.kind() { + ErrorKind::Git(failure) => format!( + "{}{}{}", + failure.detail().unwrap_or_default(), + String::from_utf8_lossy(failure.stderr()), + String::from_utf8_lossy(failure.stdout()) + ), + _ => err.to_string(), + }; + text.contains("could not lock config file") +} + fn submodule_source_trusted(trust_root: &Path, source: &SubmoduleTrustTarget) -> Result { let key = submodule_file_transport_consent_key(trust_root, &source.local_source_path); Ok(git_config_get_bool_global(trust_root, &key)?.unwrap_or(false)) diff --git a/crates/gitcomet-git-gix/tests/submodules_integration.rs b/crates/gitcomet-git-gix/tests/submodules_integration.rs index 7b17ffa6..d0b9b8d9 100644 --- a/crates/gitcomet-git-gix/tests/submodules_integration.rs +++ b/crates/gitcomet-git-gix/tests/submodules_integration.rs @@ -797,6 +797,228 @@ fn add_submodule_supports_branch_selection() { ); } +#[test] +fn add_submodule_supports_multiple_branches_from_same_source() { + if !require_git_shell_for_submodule_tests() { + return; + } + let dir = tempfile::tempdir().expect("create tempdir"); + let root = dir.path(); + + let sub_repo = root.join("sub"); + let parent_repo = root.join("parent"); + fs::create_dir_all(&sub_repo).expect("create sub repository directory"); + fs::create_dir_all(&parent_repo).expect("create parent repository directory"); + + init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); + let default_branch = git_stdout(&sub_repo, &["symbolic-ref", "--short", "HEAD"]); + run_git(&sub_repo, &["checkout", "-b", "feature"]); + fs::write(sub_repo.join("file.txt"), "feature\n").expect("write feature contents"); + run_git( + &sub_repo, + &[ + "-c", + "commit.gpgsign=false", + "commit", + "-am", + "feature commit", + ], + ); + run_git(&sub_repo, &["checkout", &default_branch]); + init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + + let backend = GixBackend; + let opened = backend.open(&parent_repo).expect("open parent repository"); + + let main_path = Path::new("mods/main"); + let approved_main = match opened + .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), main_path) + .expect("check local submodule trust for main") + { + SubmoduleTrustDecision::Prompt { sources } => sources, + other => panic!("expected trust prompt for local submodule, got {other:?}"), + }; + let main_output = opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + main_path, + Some(default_branch.as_str()), + None, + false, + &approved_main, + ) + .expect("add default-branch submodule"); + assert_eq!(main_output.exit_code, Some(0)); + + let feature_path = Path::new("mods/feature"); + let approved_feature = match opened + .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), feature_path) + .expect("check local submodule trust for feature") + { + SubmoduleTrustDecision::Prompt { sources } => sources, + SubmoduleTrustDecision::Proceed => Vec::new(), + }; + let feature_output = opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + feature_path, + Some("feature"), + None, + false, + &approved_feature, + ) + .expect("add feature-branch submodule"); + assert_eq!(feature_output.exit_code, Some(0)); + + let listed = opened.list_submodules().expect("list added submodules"); + assert_eq!(listed.len(), 2); + assert_eq!( + git_stdout( + &parent_repo.join("mods/main"), + &["branch", "--show-current"] + ), + default_branch + ); + assert_eq!( + git_stdout( + &parent_repo.join("mods/feature"), + &["branch", "--show-current"] + ), + "feature" + ); + + let gitmodules = fs::read_to_string(parent_repo.join(".gitmodules")).expect("read .gitmodules"); + assert!(gitmodules.contains(&format!("branch = {default_branch}"))); + assert!(gitmodules.contains("branch = feature")); +} + +#[test] +fn add_submodule_failed_branch_checkout_cleans_partial_clone_and_metadata() { + if !require_git_shell_for_submodule_tests() { + return; + } + let dir = tempfile::tempdir().expect("create tempdir"); + let root = dir.path(); + + let sub_repo = root.join("sub"); + let parent_repo = root.join("parent"); + fs::create_dir_all(&sub_repo).expect("create sub repository directory"); + fs::create_dir_all(&parent_repo).expect("create parent repository directory"); + + init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); + init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + + let backend = GixBackend; + let opened = backend.open(&parent_repo).expect("open parent repository"); + let submodule_path = Path::new("mods/sub"); + let approved_sources = match opened + .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), submodule_path) + .expect("check local submodule trust") + { + SubmoduleTrustDecision::Prompt { sources } => sources, + other => panic!("expected trust prompt for local submodule, got {other:?}"), + }; + + let err = opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + Some("does-not-exist"), + None, + false, + &approved_sources, + ) + .expect_err("add with missing branch should fail"); + let rendered = err.to_string(); + assert!( + rendered.contains("does-not-exist"), + "unexpected branch failure error: {rendered}" + ); + + assert!( + !parent_repo.join("mods/sub").exists(), + "expected failed submodule checkout to be removed" + ); + assert!( + !parent_repo.join(".git/modules/mods/sub").exists(), + "expected failed submodule metadata to be removed" + ); + assert!( + !parent_repo.join(".gitmodules").exists(), + "expected no .gitmodules entry after failed add" + ); + assert!(local_submodule_config_entries(&parent_repo).is_empty()); + assert!( + git_stdout(&parent_repo, &["submodule"]).is_empty(), + "expected git submodule to report no registered submodules" + ); + assert!( + opened + .list_submodules() + .expect("list submodules") + .is_empty(), + "expected failed submodule add not to be listed" + ); +} + +#[test] +fn add_submodule_failed_branch_checkout_cleans_partial_clone_with_custom_name() { + if !require_git_shell_for_submodule_tests() { + return; + } + let dir = tempfile::tempdir().expect("create tempdir"); + let root = dir.path(); + + let sub_repo = root.join("sub"); + let parent_repo = root.join("parent"); + fs::create_dir_all(&sub_repo).expect("create sub repository directory"); + fs::create_dir_all(&parent_repo).expect("create parent repository directory"); + + init_repo_with_seed(&sub_repo, "file.txt", "hello\n", "seed submodule"); + init_repo_with_seed(&parent_repo, "seed.txt", "seed\n", "seed parent"); + + let backend = GixBackend; + let opened = backend.open(&parent_repo).expect("open parent repository"); + let submodule_path = Path::new("mods/sub"); + let approved_sources = match opened + .check_submodule_add_trust(sub_repo.to_string_lossy().as_ref(), submodule_path) + .expect("check local submodule trust") + { + SubmoduleTrustDecision::Prompt { sources } => sources, + other => panic!("expected trust prompt for local submodule, got {other:?}"), + }; + + let err = opened + .add_submodule_with_output( + sub_repo.to_string_lossy().as_ref(), + submodule_path, + Some("does-not-exist"), + Some("custom-name"), + false, + &approved_sources, + ) + .expect_err("add with missing branch and custom name should fail"); + let rendered = err.to_string(); + assert!( + rendered.contains("does-not-exist"), + "unexpected branch failure error: {rendered}" + ); + + assert!( + !parent_repo.join("mods/sub").exists(), + "expected failed submodule checkout to be removed" + ); + assert!( + !parent_repo.join(".git/modules/custom-name").exists(), + "expected failed custom-name metadata to be removed" + ); + assert!( + !parent_repo.join(".gitmodules").exists(), + "expected no .gitmodules entry after failed add" + ); + assert!(local_submodule_config_entries(&parent_repo).is_empty()); +} + #[test] fn add_submodule_supports_custom_logical_name_for_local_git_dir_collision() { if !require_git_shell_for_submodule_tests() { diff --git a/crates/gitcomet-ui-gpui/src/view/mod.rs b/crates/gitcomet-ui-gpui/src/view/mod.rs index 82ad06ab..72a18c37 100644 --- a/crates/gitcomet-ui-gpui/src/view/mod.rs +++ b/crates/gitcomet-ui-gpui/src/view/mod.rs @@ -135,6 +135,8 @@ const HISTORY_COL_DATE_MAX_PX: f32 = 240.0; const HISTORY_COL_SHA_MIN_PX: f32 = 60.0; const HISTORY_COL_SHA_MAX_PX: f32 = 160.0; const HISTORY_COL_MESSAGE_MIN_PX: f32 = 220.0; +const ERROR_BANNER_OVERFLOW_HINT_MIN_LINES: usize = 8; +const ERROR_BANNER_OVERFLOW_HINT_MIN_CHARS: usize = 240; const HISTORY_GRAPH_COL_GAP_PX: f32 = 16.0; const HISTORY_GRAPH_MARGIN_X_PX: f32 = 10.0; @@ -1315,6 +1317,11 @@ impl GitCometView { (Some(command.into()), collapsed.join("\n").into()) } + fn should_show_error_banner_overflow_hint(err_text: &str) -> bool { + err_text.lines().count() > ERROR_BANNER_OVERFLOW_HINT_MIN_LINES + || err_text.len() > ERROR_BANNER_OVERFLOW_HINT_MIN_CHARS + } + fn should_render_generic_error_banner(auth_prompt_active: bool) -> bool { !auth_prompt_active } @@ -1721,6 +1728,8 @@ impl Render for GitCometView { if let Some(err_text) = banner_error { let (error_command, display_error) = Self::split_error_banner_message(err_text.as_ref()); + let show_overflow_hint = + Self::should_show_error_banner_overflow_hint(err_text.as_ref()); self.error_banner_input.update(cx, |input, cx| { input.set_theme(theme, cx); input.set_text(display_error.clone(), cx); @@ -1778,6 +1787,15 @@ impl Render for GitCometView { .child(self.error_banner_input.clone()), ), ) + .when(show_overflow_hint, |this| { + this.child( + div() + .mt_1() + .text_xs() + .text_color(theme.colors.text_muted) + .child("Scroll for full output"), + ) + }) .child(div().absolute().top(px(6.0)).right(px(6.0)).child(dismiss)), ); } diff --git a/crates/gitcomet-ui-gpui/src/view/tests.rs b/crates/gitcomet-ui-gpui/src/view/tests.rs index 4217c15e..1b3d8bc5 100644 --- a/crates/gitcomet-ui-gpui/src/view/tests.rs +++ b/crates/gitcomet-ui-gpui/src/view/tests.rs @@ -1894,6 +1894,29 @@ fn generic_error_banner_is_hidden_when_auth_prompt_is_active() { assert!(!GitCometView::should_render_generic_error_banner(true)); } +#[test] +fn error_banner_overflow_hint_is_hidden_for_short_errors() { + assert!(!GitCometView::should_show_error_banner_overflow_hint( + "Submodule failed:\n\nfatal: branch not found" + )); +} + +#[test] +fn error_banner_overflow_hint_is_shown_for_long_command_failures() { + let error = [ + "Submodule failed:", + "", + " git submodule add --branch git-subtree /tmp/src comet2", + "", + " Cloning into '/tmp/comet2'...", + " done.", + " fatal: 'origin/git-subtree' is not a commit and a branch 'git-subtree' cannot be created from it", + " fatal: unable to checkout submodule 'comet2'", + ] + .join("\n"); + assert!(GitCometView::should_show_error_banner_overflow_hint(&error)); +} + #[test] fn auth_prompt_banner_colors_use_accent_palette() { let theme = AppTheme::gitcomet_light();