From 841750223fe9d04789e5312252a6edb09a6d1714 Mon Sep 17 00:00:00 2001 From: Jaro-c <75870284+Jaro-c@users.noreply.github.com> Date: Sat, 26 Sep 2026 10:13:24 -0500 Subject: [PATCH 1/3] docs: stop describing library items that are gone After #1933, some comments still named what it removed or said the wrong thing about what stayed: - `stream_events_with_options` repeated half a sentence of its own summary. - `RunOptions` and `CommitOptions` pointed at `with_*` builders that no longer exist. - `list_tests.rs` described `Engine::list_volumes`; the kept entry point is `list_volumes_with_display`. - `ByteCounter` said its raw counter was for the libpod client's PUT helper, and that the upload-side producer was `put_archive_verified`. The counter goes to the upload body in `pack.rs`, and the wrapper that advances it is `receiver_body_with_counter`. - One test comment said extends resolution needs `parse_file`. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --- internal/engine/copy/progress.rs | 9 ++++----- internal/engine/events.rs | 1 - internal/engine/image/export.rs | 5 ++--- internal/engine/lifecycle/options.rs | 5 ++--- internal/engine/volume/list_tests.rs | 6 +++--- tests/parse/fields/extends_build.rs | 2 +- 6 files changed, 12 insertions(+), 16 deletions(-) diff --git a/internal/engine/copy/progress.rs b/internal/engine/copy/progress.rs index 9301cffc..248b4ee9 100644 --- a/internal/engine/copy/progress.rs +++ b/internal/engine/copy/progress.rs @@ -31,7 +31,7 @@ use crate::units::{format_bytes, SizeFormat}; const EMIT_INTERVAL: Duration = Duration::from_millis(100); /// A shared, monotonically-rising byte counter. Producers (the reader -/// in `extract_streamed`, the body wrapper in `put_archive_verified`) +/// in `extract_streamed`, the body wrapper in `receiver_body_with_counter`) /// `fetch_add` as bytes flow; the emitter task reads and rewrites the /// row verb. #[derive(Clone, Debug)] @@ -54,10 +54,9 @@ impl ByteCounter { self.0.load(Ordering::Relaxed) } - /// The raw shared counter, for callers that need to hand it to a - /// type outside the module (the libpod client's PUT helper takes - /// `Arc` directly to keep the byte-counting dep out of - /// the client API). + /// The raw shared counter, for a producer outside this module that + /// advances it itself (the upload body built in `pack.rs` takes the + /// `Arc` and adds each frame as it leaves). pub(crate) fn inner(&self) -> &Arc { &self.0 } diff --git a/internal/engine/events.rs b/internal/engine/events.rs index 359debb8..3b4340db 100644 --- a/internal/engine/events.rs +++ b/internal/engine/events.rs @@ -70,7 +70,6 @@ impl Engine { /// events`-style `--since`, `--until`, and `--filter` options. With `json`, /// each event is printed as a compact JSON line; otherwise as /// `TYPE ACTION NAME`. - /// `--until`, and `--filter` options. /// /// # Errors /// diff --git a/internal/engine/image/export.rs b/internal/engine/image/export.rs index d1b8ccc1..b5fbdcb8 100644 --- a/internal/engine/image/export.rs +++ b/internal/engine/image/export.rs @@ -17,9 +17,8 @@ use super::super::Engine; /// /// `#[non_exhaustive]` since 4.0.0, so a new field can be added in a minor /// release without breaking every external caller that built the struct with -/// a literal. Construct it via [`CommitOptions::new`] or the `with_*` builders -/// below; a struct literal is refused outside this crate, which is what buys -/// the room to grow. +/// a literal. Construct it via [`CommitOptions::new`]; a struct literal is refused +/// outside this crate, which is what buys the room to grow. #[derive(Debug, Clone, Default)] #[non_exhaustive] pub struct CommitOptions { diff --git a/internal/engine/lifecycle/options.rs b/internal/engine/lifecycle/options.rs index 823506ac..4ddb03a9 100644 --- a/internal/engine/lifecycle/options.rs +++ b/internal/engine/lifecycle/options.rs @@ -10,9 +10,8 @@ /// /// `#[non_exhaustive]` since 4.0.0, so a new field can be added in a minor /// release without breaking every external caller that built the struct with -/// a literal. Construct it via [`RunOptions::new`] or the `with_*` builders -/// below; a struct literal is refused outside this crate, which is what buys -/// the room to grow. +/// a literal. Construct it via [`RunOptions::new`]; a struct literal is refused +/// outside this crate, which is what buys the room to grow. #[derive(Default)] #[non_exhaustive] pub struct RunOptions { diff --git a/internal/engine/volume/list_tests.rs b/internal/engine/volume/list_tests.rs index 157d407f..e4768c8d 100644 --- a/internal/engine/volume/list_tests.rs +++ b/internal/engine/volume/list_tests.rs @@ -60,11 +60,11 @@ fn reclaimable_is_reported_separately_from_size() { /// line, which a parser distinguishes from "no volumes" only by row count. /// Stdout stays empty; stderr carries the explicit `no volumes` (#1675). /// -/// The full `Engine::list_volumes` path needs an `Engine` and a `ComposeFile`, +/// The full `Engine::list_volumes_with_display` path needs an `Engine` and a `ComposeFile`, /// neither of which is light to construct for a unit test. Instead, pin the /// table's empty predicate (the branch the printer reads) and the source path /// that consults it. A regression that drops the `table.is_empty()` branch -/// from `list_volumes` would make the next header-only line appear on stdout +/// from `list_volumes_with_display` would make the next header-only line appear on stdout /// again, and this test would catch it. #[test] fn an_empty_volumes_table_prints_one_line_on_stderr() { @@ -72,7 +72,7 @@ fn an_empty_volumes_table_prints_one_line_on_stderr() { assert!(t.is_empty(), "a freshly built table is empty"); t.push(vec!["data".into(), "local".into(), "no".into()]); assert!(!t.is_empty(), "a table with one row is not empty"); - // The empty-table branch in `list_volumes` consults `Table::is_empty()` + // The empty-table branch in `list_volumes_with_display` consults `Table::is_empty()` // before printing, and the regression that would matter is removing the // call to `crate::ui::progress_note("no volumes")` in that branch. let src = include_str!("list.rs"); diff --git a/tests/parse/fields/extends_build.rs b/tests/parse/fields/extends_build.rs index a198e6d1..ca57934f 100644 --- a/tests/parse/fields/extends_build.rs +++ b/tests/parse/fields/extends_build.rs @@ -44,7 +44,7 @@ services: #[test] fn extends_with_file_field_parses() { - // Just verify that extends with file is parsed (resolution requires parse_file). + // Just verify that extends with file is parsed (resolution needs a file on disk). let yaml = r#" services: app: From aa10d7f2fef43c0fbd75beb34b1e5c3ea2e1397f Mon Sep 17 00:00:00 2001 From: Jaro-c <75870284+Jaro-c@users.noreply.github.com> Date: Sat, 26 Sep 2026 10:13:25 -0500 Subject: [PATCH 2/3] refactor: remove what #1933 left without a caller - `CpOptions::with_index`: the command line sets the index through `CpOptions::new`, and its only other caller was a test that went with #1933. - `Table`'s per-row key: `push_keyed` was the only thing that set one, so every row carried `None` and `format_row_keyed` always fell back to the identity cell's own text. `format_row` now does that directly. - `build_vars_with_env_files_inner`'s `strict` flag: its only caller passes `true` since the lenient wrapper was removed, so the lenient branches could not run. The `.env` fallback keeps its lenient behaviour; it goes through `build_vars`, not this function. Nothing a user sees changes: the non-live suite passes unchanged (2960 with the tests of the next commit). Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --- internal/engine/copy.rs | 8 ------- internal/substitute/mod.rs | 47 ++++++++++++++------------------------ internal/ui/table.rs | 23 +++---------------- 3 files changed, 20 insertions(+), 58 deletions(-) diff --git a/internal/engine/copy.rs b/internal/engine/copy.rs index 12b7120e..82d42819 100644 --- a/internal/engine/copy.rs +++ b/internal/engine/copy.rs @@ -119,14 +119,6 @@ impl CpOptions { archive, } } - - /// 1-based replica index for a scaled service, `--index` (default: first). - /// Builder-style. - #[must_use] - pub fn with_index(mut self, index: Option) -> Self { - self.index = index; - self - } } impl Engine { diff --git a/internal/substitute/mod.rs b/internal/substitute/mod.rs index c5658f6a..cb2bb081 100644 --- a/internal/substitute/mod.rs +++ b/internal/substitute/mod.rs @@ -312,7 +312,7 @@ pub fn build_vars_with_env_files_strict( dir: &Path, extra: &[String], ) -> Result> { - build_vars_with_env_files_inner(dir, extra, true) + build_vars_with_env_files_inner(dir, extra) } /// The first control character in `value` that is never legitimate in an @@ -330,7 +330,6 @@ fn first_disallowed_control_char(value: &str) -> Option { fn build_vars_with_env_files_inner( dir: &Path, extra: &[String], - strict: bool, ) -> Result> { if extra.is_empty() { return Ok(build_vars(dir)); @@ -344,23 +343,13 @@ fn build_vars_with_env_files_inner( } else { dir.join(path) }; - let content = match crate::filesystem::read_to_string_capped(&abs) { - Ok(content) => content, - Err(e) => { - if strict { - return Err(crate::error::ComposeError::EnvFile(format!( - "env file not found: {} ({e})", - abs.display() - ))); - } - continue; - } - }; - let pairs = if strict { - crate::dotenv::parse_strict(&content)? - } else { - crate::dotenv::parse(&content) - }; + let content = crate::filesystem::read_to_string_capped(&abs).map_err(|e| { + crate::error::ComposeError::EnvFile(format!( + "env file not found: {} ({e})", + abs.display() + )) + })?; + let pairs = crate::dotenv::parse_strict(&content)?; for (key, value) in pairs { // A disallowed control character (e.g. NUL) in a value would be // interpolated verbatim into a compose scalar, where it is meaningless @@ -368,17 +357,15 @@ fn build_vars_with_env_files_inner( // at load time, with an error that names the originating env file and // key, instead of letting it surface later as a compose-file parse // error at a meaningless post-substitution offset. Only the explicit - // (strict) `--env-file`/`env_file:` path errors; the lenient `.env` - // fallback keeps its historical pass-through behaviour. - if strict { - if let Some(bad) = first_disallowed_control_char(&value) { - return Err(crate::error::ComposeError::EnvFile(format!( - "env file {}: value of '{key}' contains a disallowed control \ - character ({}); remove it before use", - abs.display(), - bad.escape_default(), - ))); - } + // `--env-file`/`env_file:` path errors; the `.env` fallback + // (`build_vars`) keeps its historical pass-through behaviour. + if let Some(bad) = first_disallowed_control_char(&value) { + return Err(crate::error::ComposeError::EnvFile(format!( + "env file {}: value of '{key}' contains a disallowed control \ + character ({}); remove it before use", + abs.display(), + bad.escape_default(), + ))); } file_vars.insert(key, value); } diff --git a/internal/ui/table.rs b/internal/ui/table.rs index 4bb3e705..68a337e9 100644 --- a/internal/ui/table.rs +++ b/internal/ui/table.rs @@ -84,9 +84,6 @@ pub struct Table { /// lands on. See [`Table::dim_cols`]. dim_cols: Vec, rows: Vec>, - /// Per-row identity key, parallel to `rows`. `None` falls back to the - /// identity cell's own text. - keys: Vec>, } impl Table { @@ -101,7 +98,6 @@ impl Table { caution_col: None, dim_cols: Vec::new(), rows: Vec::new(), - keys: Vec::new(), } } @@ -162,7 +158,6 @@ impl Table { /// cells render blank and extra cells are ignored. pub fn push(&mut self, cells: Vec) { self.rows.push(cells); - self.keys.push(None); } /// Whether any column marker is set, i.e. whether rendering with colour could @@ -206,17 +201,6 @@ impl Table { /// meaning (the padding is applied first so the zero-width ANSI codes never /// disturb alignment). fn format_row(&self, cells: &[String], widths: &[usize], colour: bool) -> String { - self.format_row_keyed(cells, widths, colour, None) - } - - /// [`Table::format_row`] with the row's identity key, when it has one. - fn format_row_keyed( - &self, - cells: &[String], - widths: &[usize], - colour: bool, - key: Option<&str>, - ) -> String { let last = self.headers.len().saturating_sub(1); (0..self.headers.len()) .map(|i| { @@ -230,7 +214,7 @@ impl Table { // The padding is inside the paint so the colour does not stop // at the name and leave the gap bare; the codes are zero-width // either way, so alignment is untouched. - return super::paint(super::identity_style(key.unwrap_or(cell)), &padded, true); + return super::paint(super::identity_style(cell), &padded, true); } if colour && Some(i) == self.caution_col { return super::paint(caution_style(cell), &padded, true); @@ -280,9 +264,8 @@ impl Table { // without being listed, and it went unnoticed because its first caller, // `volumes`, also sets `identity_col`, so the gate happened to be open. let colour = self.colours_any_column() && super::stdout_colored(); - for (i, row) in self.rows.iter().enumerate() { - let key = self.keys.get(i).and_then(Option::as_deref); - writeln!(w, "{}", self.format_row_keyed(row, &widths, colour, key))?; + for row in &self.rows { + writeln!(w, "{}", self.format_row(row, &widths, colour))?; } Ok(()) } From 96a8d533b68912bcd1a19782a305e26793a3b7d2 Mon Sep 17 00:00:00 2001 From: Jaro-c <75870284+Jaro-c@users.noreply.github.com> Date: Sat, 26 Sep 2026 10:13:25 -0500 Subject: [PATCH 3/3] test: pin what three tests removed in #1933 still covered - `an_unprefixed_label_is_keyed_on_itself`: a label without the project prefix gets the colour of the service of that name, now compared on `identity_slot` and `service_slot`. Keying the unprefixed label on anything else fails it. - `set_services_makes_registered_names_distinct` also checks that the three distinct slots render as three distinct colours through `slot_to_style`; collapsing the wide palette to one colour fails it. - `a_board_with_every_row_finished_tallies_all_of_them`: finishing every seeded row tallies (3, 3). Capping the done count fails it. - `check_status_falls_back_to_raw_body_on_non_json` also checks the body is kept verbatim, not only contained; adding one character to it fails the test. `slot_to_style` gets back the reason its palette choice is a parameter: the real probe is process-cached, so a test that flipped it would depend on scheduling. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com> --- internal/libpod/client/tests.rs | 4 ++++ internal/ui/mod.rs | 4 ++++ internal/ui/mod_tests.rs | 28 +++++++++++++++++++++++++++- internal/ui/progress/board_tests.rs | 15 +++++++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-) diff --git a/internal/libpod/client/tests.rs b/internal/libpod/client/tests.rs index c714b8a2..7178ec56 100644 --- a/internal/libpod/client/tests.rs +++ b/internal/libpod/client/tests.rs @@ -45,6 +45,10 @@ fn check_status_falls_back_to_raw_body_on_non_json() { Client::check_status(StatusCode::INTERNAL_SERVER_ERROR, b"plain text error").unwrap_err(); assert!(err.is_status(500)); assert!(err.to_string().contains("plain text error")); + assert!( + matches!(&err, super::super::PodmanError::Api { message, .. } if message == "plain text error"), + "a non-JSON body must be kept verbatim: {err:?}" + ); } // --------------------------------------------------------------------------- diff --git a/internal/ui/mod.rs b/internal/ui/mod.rs index 411339aa..7cadc241 100644 --- a/internal/ui/mod.rs +++ b/internal/ui/mod.rs @@ -520,6 +520,10 @@ pub(crate) fn service_slot(name: &str) -> usize { /// The style for a palette slot, from whichever palette the terminal supports. /// +/// The palette choice is a parameter so both branches are testable: the real +/// decision reads a process-cached environment probe, and a test that flipped +/// it would pass or fail on test scheduling. +/// /// The narrow branch takes a slot assigned against the WIDE palette's size, so /// it must wrap again: indexing a six-element array with a slot up to 19 is /// the out-of-bounds bug the old hash's `assert!` used to guard. diff --git a/internal/ui/mod_tests.rs b/internal/ui/mod_tests.rs index 7dafb182..55dfa4de 100644 --- a/internal/ui/mod_tests.rs +++ b/internal/ui/mod_tests.rs @@ -74,7 +74,7 @@ fn paint_gates_on_enabled() { #[test] fn colour_choice_resolution() { // Pure resolution: never touches the process-global choice, so it can't - // race the production code (LinePrefixer/status_cell) that reads it. + // race the production code (LinePrefixer, the table renderer) that reads it. temp_env::with_var_unset("NO_COLOR", || { assert!(!colored_with(ColorChoice::Never, true)); assert!(colored_with(ColorChoice::Always, false)); @@ -190,6 +190,32 @@ fn set_services_makes_registered_names_distinct() { assert_ne!(a, b, "registered names must not share a colour"); assert_ne!(b, g, "registered names must not share a colour"); assert_ne!(a, g, "registered names must not share a colour"); + // Distinct slots are only worth it if they render as distinct colours. + let style = |slot| slot_to_style(slot, true).render().to_string(); + assert_ne!( + style(a), + style(b), + "distinct slots must render distinct colours" + ); + assert_ne!( + style(b), + style(g), + "distinct slots must render distinct colours" + ); + assert_ne!( + style(a), + style(g), + "distinct slots must render distinct colours" + ); +} + +/// A label that does not carry the project prefix is left alone: it gets the +/// colour of the service of that name. +#[test] +fn an_unprefixed_label_is_keyed_on_itself() { + let _guard = registry_guard(); + set_project("proj"); + assert_eq!(identity_slot("web"), service_slot("web")); } /// The narrow-terminal palette is a six-entry array, and `slot_to_style`'s diff --git a/internal/ui/progress/board_tests.rs b/internal/ui/progress/board_tests.rs index a908bc35..b7e273b1 100644 --- a/internal/ui/progress/board_tests.rs +++ b/internal/ui/progress/board_tests.rs @@ -19,6 +19,21 @@ fn a_seeded_board_knows_its_total_before_anything_happens() { assert_eq!(seeded().tally(), (0, 3)); } +/// Finishing every seeded row is what the progress line reports as done. +#[test] +fn a_board_with_every_row_finished_tallies_all_of_them() { + let mut b = seeded(); + let now = t0(); + for (kind, name) in [ + (Kind::Network, "proj_default"), + (Kind::Container, "proj-web-1"), + (Kind::Container, "proj-db-1"), + ] { + b.finish(kind, name, "Created", now); + } + assert_eq!(b.tally(), (3, 3)); +} + /// The counter tracks finished rows, not started ones. A row being worked on is /// not progress the user can rely on. #[test]