diff --git a/CLAUDE.md b/CLAUDE.md index 051394c..59883f7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -89,10 +89,15 @@ After destructive operations (delete, nuke), `GitOpsManager::reopen()` must be c ### Testing Approach -Integration tests in `tests/` use a `TestHarness` (in `tests/common/mod.rs`) that creates temporary git repos and invokes the compiled binary. Test files: +Integration tests in `tests/` use a `TestHarness` (in `tests/common/mod.rs`) that creates temporary git repos and invokes the compiled binary. Beyond running the CLI, the harness inspects real git state — `index_gitlink_mode`, `gitmodules_entries`, `submodule_config_entries`, `git_modules_dir_exists` — and tests should assert on that rather than on printed output where both are available. Test files: - `integration_tests.rs` — end-to-end CLI behavior - `command_contract_tests.rs` — CLI command argument contracts - `config_tests.rs` — configuration parsing/serialization - `sparse_checkout_tests.rs` — sparse checkout behavior - `error_handling_tests.rs` — error conditions and messages -- `performance_tests.rs` — benchmarks (uses criterion) +- `fallback_tests.rs` — the gix→git2→CLI fallback chain, via the `GitOpsManager::without_gix` and `::forcing_cli_add` injection seams +- `git_ops_tests.rs` — the `GitOperations` backends directly +- `security_tests.rs` — path traversal, symlink escape, and command/flag injection containment +- `performance_tests.rs` — timing and peak-allocation ceilings (plain `#[test]`s, not criterion) + +Criterion benchmarks live separately in `benches/benchmark.rs` (`cargo bench`). diff --git a/src/lib.rs b/src/lib.rs index eb07705..cfcd8fc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2,6 +2,10 @@ // // SPDX-License-Identifier: LicenseRef-PlainMIT OR MIT #![cfg_attr(coverage_nightly, feature(coverage_attribute))] +// Cargo.toml sets `unsafe_code = "deny"` workspace-wide, which a single +// `#![allow(unsafe_code)]` can opt back out of. The test crate needs that escape +// hatch for its tracking allocator; the library does not, so forbid it here. +#![forbid(unsafe_code)] //! A Rust CLI tool for managing Git submodules with enhanced features and user-friendly configuration. //! This module is exposed for integration testing; it is not intended for public use and may contain unstable APIs. diff --git a/src/main.rs b/src/main.rs index 2d744ef..3a9cd51 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,6 +2,8 @@ // // SPDX-License-Identifier: LicenseRef-PlainMIT OR MIT #![cfg_attr(coverage_nightly, feature(coverage_attribute))] +// See the note in lib.rs: `deny` is opt-out-able per file, `forbid` is not. +#![forbid(unsafe_code)] #![doc = r" Main entry point for the submod CLI tool. diff --git a/tests/command_contract_tests.rs b/tests/command_contract_tests.rs index 9d12d90..ac00f48 100644 --- a/tests/command_contract_tests.rs +++ b/tests/command_contract_tests.rs @@ -44,10 +44,16 @@ mod tests { ); let stdout = String::from_utf8_lossy(&output.stdout); - // Bash completion scripts always contain the function/complete keyword + // A usable bash completion script needs both halves: the function that + // produces candidates, and the registration that binds it to the command. + // ("submod" alone would be satisfied by any output mentioning the binary.) assert!( - stdout.contains("complete") || stdout.contains("_submod") || stdout.contains("submod"), - "bash completion script should reference submod; got: {stdout}" + stdout.contains("_submod()"), + "bash completion script should define the completion function; got: {stdout}" + ); + assert!( + stdout.contains("complete -F _submod"), + "bash completion script should register the function for submod; got: {stdout}" ); } @@ -230,9 +236,11 @@ mod tests { let init_verbose = harness .run_submod_success(&["init", "--verbose"]) .expect("Failed to run init --verbose"); + // The first init above already initialized it, so a second run must say so + // by name rather than claim to initialize it again. assert!( - init_verbose.contains("lazy-lib") || init_verbose.contains("already initialized"), - "Verbose init should mention the submodule; got: {init_verbose}" + init_verbose.contains("lazy-lib already initialized"), + "Verbose init should report the submodule as already initialized; got: {init_verbose}" ); } @@ -269,9 +277,14 @@ mod tests { .run_submod_success(&["nuke-it-from-orbit", "--all", "--kill"]) .expect("Failed to nuke all"); + // --all must nuke every submodule, not just the first one it finds. + assert!( + stdout.contains("Nuking submodule 'nuke-a'..."), + "Expected nuke progress for nuke-a; got: {stdout}" + ); assert!( - stdout.contains("Nuking") || stdout.contains("💥"), - "Expected nuke progress output; got: {stdout}" + stdout.contains("Nuking submodule 'nuke-b'..."), + "Expected nuke progress for nuke-b; got: {stdout}" ); let config_after = harness.read_config().expect("Failed to read config"); @@ -313,9 +326,15 @@ mod tests { .run_submod_success(&["nuke-it-from-orbit", "reinit-lib"]) .expect("Failed to nuke-and-reinit"); + // Without --kill both halves must happen; either one alone is the bug this + // test exists to catch, so assert them separately rather than as a disjunction. + assert!( + stdout.contains("Nuking submodule 'reinit-lib'..."), + "Expected nuke progress; got: {stdout}" + ); assert!( - stdout.contains("Nuking") || stdout.contains("Reinitializing"), - "Expected nuke/reinit progress; got: {stdout}" + stdout.contains("Reinitializing submodule 'reinit-lib'..."), + "Expected reinit progress; got: {stdout}" ); // After reinit, submodule should exist again @@ -565,9 +584,9 @@ mod tests { .run_submod_success(&["change", "movable-lib", "--path", "lib/moved"]) .expect("Failed to change submodule path"); - // Should confirm the update + // Should confirm the update, naming the submodule it re-added at the new path. assert!( - stdout.contains("Added submodule") || stdout.contains("movable-lib"), + stdout.contains("Added submodule movable-lib"), "Expected confirmation of path change; got: {stdout}" ); @@ -1063,9 +1082,11 @@ mod tests { let stdout_verbose = harness .run_submod_success(&["init", "--verbose"]) .expect("Failed to run init --verbose"); + // "Initializing" || "initialized" could not distinguish the two states; the + // default init above ran first, so this run must report the already-done case. assert!( - stdout_verbose.contains("Initializing") || stdout_verbose.contains("initialized"), - "verbose init output should mention initialization; got: {stdout_verbose}" + stdout_verbose.contains("init-contract already initialized"), + "verbose init output should name the already-initialized submodule; got: {stdout_verbose}" ); } diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 4d7961f..1b0474a 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -192,7 +192,13 @@ sparse_paths = ["src"] .run_submod_success(&["update"]) .expect("Failed to run update"); - assert!(stdout.contains("Updated") || stdout.contains("Already up to date")); + // 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. + assert!( + stdout.contains("Updated 1 submodule(s)"), + "update should report how many submodules it updated; got: {stdout}" + ); } /// `update` must check the submodule worktree out to the commit recorded as @@ -586,11 +592,17 @@ active = true let output = harness .run_submod(&["check", "--verbose"]) .expect("Failed to run submod"); - assert!(!output.status.success()); + assert_eq!(output.status.code(), Some(1)); let stderr = String::from_utf8_lossy(&output.stderr); assert!( - stderr.contains("Failed to create manager") || stderr.contains("Repository not found") + stderr.contains("Failed to create manager"), + "error should name the operation that failed; got: {stderr}" + ); + // The underlying cause must survive the wrapping, not be flattened away. + assert!( + stderr.contains("Repository not found"), + "error should preserve the underlying cause; got: {stderr}" ); } @@ -611,9 +623,12 @@ active = true ]) .expect("Failed to run submod"); - assert!(!output.status.success()); + assert_eq!(output.status.code(), Some(1)); let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("Failed to add submodule") || stderr.contains("clone failed")); + assert!( + stderr.contains("Failed to add submodule"), + "error should name the operation that failed; got: {stderr}" + ); } #[test] @@ -1042,9 +1057,13 @@ active = true ]) .expect("Failed to run generate-config"); - assert!(!output.status.success()); + assert_eq!(output.status.code(), Some(1)); let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("already exists") || stderr.contains("Use --force")); + // Both the refusal and the way out of it must be in the message. + assert!( + stderr.contains("already exists. Use --force to overwrite."), + "error should refuse to clobber and name the override flag; got: {stderr}" + ); } #[test] @@ -1073,7 +1092,10 @@ active = true .run_submod_success(&["nuke-it-from-orbit", "nuke-lib", "--kill"]) .expect("Failed to nuke submodule"); - assert!(stdout.contains("Nuking") || stdout.contains("💥")); + assert!( + stdout.contains("💥 Nuking submodule 'nuke-lib'..."), + "nuke should report the submodule it is nuking; got: {stdout}" + ); // Config should not contain the submodule anymore let config = harness.read_config().expect("Failed to read config");