From 3e0477cad4033df75a7f691ed52ca5377f1339f3 Mon Sep 17 00:00:00 2001 From: Adam Poulemanos Date: Sat, 25 Jul 2026 22:49:05 -0400 Subject: [PATCH] fix(update): keep gitoxide's fetch report off stdout, name each submodule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `submod update` printed gitoxide's own CLI-shaped fetch report — the refspec mapping and a per-ref status line — straight to stdout, then a bare "Updated N submodule(s)" summary that never said which submodules were touched. The plumbing narration was the only per-submodule detail on stdout, and it is gitoxide's output, not ours. `simple_gix::get_progress` was writing the captured `out` buffer to this process's stdout unconditionally. It now returns the captured buffers and lets the caller decide. `fetch_repo` takes a `verbose` flag and routes the report to stderr when the caller asked for it, or when the fetch failed and it is the only detail explaining why — so the diagnostic is preserved, not discarded. stdout is left to carry submod's own output, which callers parse. `GitManager::update_submodule` now names each submodule unconditionally rather than only under --verbose, so a multi-submodule update reports what it did rather than just how many. Verbose is plumbed through `GixOperations` via a `with_verbose` builder so the ~35 existing single-argument `GixOperations::new` call sites in the test suite are untouched and stay quiet by default. test_update_command now asserts the submodule is named and that the refspec report does not reach stdout — the leak is what made the per-submodule line unassertable in the first place, and left the wide `||` disjunction that #62 P2 flagged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XocU6NjgZricrRWMLyLXDn --- src/git_manager.rs | 7 +++--- src/git_ops/gix_ops.rs | 28 ++++++++++++++++++---- src/git_ops/mod.rs | 6 +++-- src/git_ops/simple_gix.rs | 49 ++++++++++++++++++++++++++++++-------- tests/integration_tests.rs | 16 ++++++++++--- 5 files changed, 83 insertions(+), 23 deletions(-) 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