diff --git a/src/git_manager.rs b/src/git_manager.rs index ae28fb3..f1d43da 100644 --- a/src/git_manager.rs +++ b/src/git_manager.rs @@ -707,9 +707,10 @@ impl GitManager { SubmoduleError::GitoxideError(format!("GitOpsManager update failed: {e}")) })?; - if self.verbose { - println!("✅ Updated {name} successfully"); - } + // Name every submodule that was updated, not just a trailing count: with + // only the count, a multi-submodule `update` gives no way to tell which + // ones it actually touched. + println!("✅ Updated {name}"); Ok(()) } diff --git a/src/git_ops/gix_ops.rs b/src/git_ops/gix_ops.rs index 8fc7c8b..f19e50f 100644 --- a/src/git_ops/gix_ops.rs +++ b/src/git_ops/gix_ops.rs @@ -29,9 +29,12 @@ use crate::utilities; #[derive(Debug, Clone, PartialEq)] pub struct GixOperations { repo: gix::Repository, + /// Whether to surface the underlying gitoxide fetch report on stderr. + verbose: bool, } impl GixOperations { - /// Create a new `GixOperations` instance + /// Create a new `GixOperations` instance. Quiet by default; see + /// [`GixOperations::with_verbose`]. pub fn new(repo_path: Option<&Path>) -> Result { let repo = match repo_path { Some(path) => gix::open(path) @@ -39,7 +42,17 @@ impl GixOperations { None => gix::discover(".") .with_context(|| "Failed to discover repository in current directory")?, }; - Ok(Self { repo }) + Ok(Self { + repo, + verbose: false, + }) + } + + /// Set whether gitoxide's own fetch report is echoed to stderr. + #[must_use] + pub const fn with_verbose(mut self, verbose: bool) -> Self { + self.verbose = verbose; + self } /// Try to perform operation with gix, return error if not supported @@ -457,8 +470,13 @@ impl GitOperations for GixOperations { // Pass None to let gix resolve the default remote (which has refspecs configured). // Passing the URL string would create a bare remote without refspecs. let submodule_repo = gix::open(&submodule_path)?; - fetch_repo(submodule_repo, None, entry.shallow == Some(true)) - .map_err(|e| anyhow::anyhow!("Failed to fetch submodule: {e}"))?; + fetch_repo( + submodule_repo, + None, + entry.shallow == Some(true), + self.verbose, + ) + .map_err(|e| anyhow::anyhow!("Failed to fetch submodule: {e}"))?; match opts.strategy { crate::options::SerializableUpdate::Checkout | crate::options::SerializableUpdate::Unspecified => { @@ -696,7 +714,7 @@ impl GitOperations for GixOperations { fn fetch_submodule(&self, path: &str) -> Result<()> { // Pass None to let gix resolve the default remote (which has refspecs configured). let submodule_repo = utilities::repo_from_path(&std::path::PathBuf::from(path))?; - fetch_repo(submodule_repo, None, false) + fetch_repo(submodule_repo, None, false, self.verbose) .map_err(|e| anyhow::anyhow!("Failed to fetch submodule: {e}")) } diff --git a/src/git_ops/mod.rs b/src/git_ops/mod.rs index 6d14cf9..28f09b5 100644 --- a/src/git_ops/mod.rs +++ b/src/git_ops/mod.rs @@ -178,7 +178,9 @@ pub struct GitOpsManager { impl GitOpsManager { /// Create a new `GitOpsManager` with automatic fallback pub fn new(repo_path: Option<&Path>, verbose: bool) -> Result { - let gix_ops = GixOperations::new(repo_path).ok(); + let gix_ops = GixOperations::new(repo_path) + .ok() + .map(|ops| ops.with_verbose(verbose)); let git2_ops = Git2Operations::new(repo_path) .with_context(|| "Failed to initialize git2 operations")?; @@ -266,7 +268,7 @@ impl GitOpsManager { // gix is an optional optimistic backend — log failures but don't fail. match GixOperations::new(Some(&workdir)) { Ok(new_gix) => { - self.gix_ops = Some(new_gix); + self.gix_ops = Some(new_gix.with_verbose(self.verbose)); } Err(e) => { if self.verbose { diff --git a/src/git_ops/simple_gix.rs b/src/git_ops/simple_gix.rs index 6a5189a..a5f96d2 100644 --- a/src/git_ops/simple_gix.rs +++ b/src/git_ops/simple_gix.rs @@ -13,7 +13,7 @@ use gitoxide_core::repository::fetch::{ }; use gix::{features::progress, progress::prodash}; use prodash::render::line; -use std::io::{stderr, stdout}; +use std::io::stderr; /// A standard range for line renderer. pub fn setup_line_renderer_range( @@ -46,7 +46,14 @@ pub fn progress_tree(trace: bool) -> std::sync::Arc { .into() } -/// Run a function with progress tracking, capturing output to stdout and stderr. +/// Run a function with progress tracking, returning its result alongside the +/// bytes it wrote to the `out` and `err` handles it was given. +/// +/// `gitoxide-core` writes CLI-shaped reports to those handles — for `fetch`, +/// the refspec mapping and a per-ref status line. That is narration for someone +/// running `gix` directly, not part of `submod`'s output contract, so it is +/// captured and handed back rather than written to this process's own streams. +/// The caller decides where, and whether, it is shown. pub fn get_progress( func_name: &str, range: Option>, @@ -55,7 +62,7 @@ pub fn get_progress( &mut dyn std::io::Write, &mut dyn std::io::Write, ) -> T, -) -> Result { +) -> (T, Vec, Vec) { let standard_range = 2..=2; let range = range.unwrap_or_else(|| standard_range.clone()); let progress = progress_tree(false); @@ -75,9 +82,7 @@ pub fn get_progress( }); handle.shutdown_and_wait(); - std::io::Write::write_all(&mut stdout(), &out)?; - std::io::Write::write_all(&mut stderr(), &err)?; - Ok(result) + (result, out, err) } /// Fetch options for the `fetch` command, with an option for shallow fetching. @@ -100,9 +105,33 @@ const fn fetch_options(remote: Option, shallow: bool) -> FetchOptions { } /// Fetch updates from a remote repository. -pub fn fetch_repo(repo: gix::Repository, remote: Option, shallow: bool) -> Result<()> { - let inner_result = get_progress("fetch", Some(FetchProgressRange), |progress, out, err| { - gitoxide_core::repository::fetch(repo, progress, out, err, fetch_options(remote, shallow)) - })?; +/// +/// The fetch report `gitoxide-core` produces never reaches stdout: stdout +/// carries `submod`'s own output, and callers parse it. The report is written to +/// stderr when the caller asked for verbose output, or when the fetch failed and +/// it is the only detail available to explain why. +pub fn fetch_repo( + repo: gix::Repository, + remote: Option, + shallow: bool, + verbose: bool, +) -> Result<()> { + let (inner_result, out, err) = + get_progress("fetch", Some(FetchProgressRange), |progress, out, err| { + gitoxide_core::repository::fetch( + repo, + progress, + out, + err, + fetch_options(remote, shallow), + ) + }); + + if verbose || inner_result.is_err() { + let mut sink = stderr(); + std::io::Write::write_all(&mut sink, &out)?; + std::io::Write::write_all(&mut sink, &err)?; + } + inner_result.map_err(|e| anyhow::anyhow!("Fetch failed: {e}")) } diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 1b0474a..178e79b 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -192,13 +192,23 @@ sparse_paths = ["src"] .run_submod_success(&["update"]) .expect("Failed to run update"); - // The one submodule in the config must be counted. `update` does not name - // the submodules it touched — it forwards the raw fetch output and then - // prints this summary — so the count is the assertable contract here. + // `update` must name each submodule it touched, then summarize the count. + assert!( + stdout.contains("✅ Updated update-lib"), + "update should name the submodule it updated; got: {stdout}" + ); assert!( stdout.contains("Updated 1 submodule(s)"), "update should report how many submodules it updated; got: {stdout}" ); + + // gitoxide's fetch report is plumbing narration for `gix fetch`, not part + // of submod's output. Leaking it to stdout is what made the per-submodule + // line unassertable in the first place. + assert!( + !stdout.contains("refs/remotes/origin/"), + "update must not leak the raw fetch refspec report to stdout; got: {stdout}" + ); } /// `update` must check the submodule worktree out to the commit recorded as