refactor: clean up what #1933 left behind

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<u32>) -> Self {
-		self.index = index;
-		self
-	}
 }
 
 impl Engine {

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<AtomicU64>` 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<AtomicU64>` and adds each frame as it leaves).
 	pub(crate) fn inner(&self) -> &Arc<AtomicU64> {
 		&self.0
 	}

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
 	///

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 {

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 {

internal/engine/volume/list_tests.rs

@@ -60,19 +60,19 @@ 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() {
 	let mut t = crate::ui::Table::new(&["NAME", "DRIVER", "EXTERNAL"]);
 	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");

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:?}"
+	);
 }
 
 // ---------------------------------------------------------------------------

internal/substitute/mod.rs

@@ -312,7 +312,7 @@ pub fn build_vars_with_env_files_strict(
 	dir: &Path,
 	extra: &[String],
 ) -> Result<HashMap<String, String>> {
-	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<char> {
 fn build_vars_with_env_files_inner(
 	dir: &Path,
 	extra: &[String],
-	strict: bool,
 ) -> Result<HashMap<String, String>> {
 	if extra.is_empty() {
 		return Ok(build_vars(dir));
@@ -344,41 +343,29 @@ 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
 			// at best and corrupts the container's config at worst. Reject it here,
 			// 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);
 		}

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.

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

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]

internal/ui/table.rs

@@ -84,9 +84,6 @@ pub struct Table {
 	/// lands on. See [`Table::dim_cols`].
 	dim_cols: Vec<usize>,
 	rows: Vec<Vec<String>>,
-	/// Per-row identity key, parallel to `rows`. `None` falls back to the
-	/// identity cell's own text.
-	keys: Vec<Option<String>>,
 }
 
 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<String>) {
 		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(())
 	}

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: