From 6752ac5915c9dcb4739559d6e7aff1fb9972fa68 Mon Sep 17 00:00:00 2001 From: Adam Poulemanos Date: Mon, 29 Jun 2026 22:56:05 -0400 Subject: [PATCH] fix(git_ops): clean up path-keyed .gitmodules entry after a failed add (#62 P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #62 audit (P2) asked for idempotency / partial-failure coverage. Writing the tests surfaced a real masked bug: when a submodule already exists (so .gitmodules is present) and a subsequent `add` fails against an unreachable URL, a stale `[submodule ""]` entry was left behind in .gitmodules. Root cause: the add fallback cleanup (git_ops/mod.rs) removed the .gitmodules section by `opts.name`, but git2 writes the section keyed by *path* (it uses the path as the section key when name != path). The name-keyed match missed git2's path-keyed entry, so it lingered after the failed add. Fix: match both the name- and path-keyed section headers during cleanup. TDD: - test_failed_add_leaves_no_partial_state — baseline add, then a failed add; asserts no stale .gitmodules/git-config entry, no orphan worktree dir, no dangling .git/modules, AND that the pre-existing submodule survives (non-vacuous). RED before the fix, GREEN after. - test_add_same_submodule_twice_is_idempotent — re-adding the same name+path succeeds with exactly one entry in .gitmodules, git config, and submod.toml (characterization; behavior was already correct). - test_delete_nonexistent_submodule_fails — deleting a missing submodule exits non-zero with a specific "ghost ... not found" error (characterization). Full suite 558 pass; fmt + clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01T8D5ZK1473YCiZkbueAY2X --- src/git_ops/mod.rs | 7 ++- tests/integration_tests.rs | 125 +++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 1 deletion(-) diff --git a/src/git_ops/mod.rs b/src/git_ops/mod.rs index fcd460e..b25f4db 100644 --- a/src/git_ops/mod.rs +++ b/src/git_ops/mod.rs @@ -394,10 +394,15 @@ impl GitOperations for GitOpsManager { if let Ok(content) = std::fs::read_to_string(&gitmodules_path) { let mut new_content = String::new(); let mut in_target_section = false; + // git2 may key the `.gitmodules` section by either the name or the + // path (it uses the path as the section key when name != path), so + // match both to avoid leaving a stale entry behind. let target_name = format!("\"{}\"", opts.name); + let target_path = format!("\"{}\"", opts.path.display()); for line in content.lines() { if line.starts_with("[submodule \"") { - in_target_section = line.contains(&target_name); + in_target_section = + line.contains(&target_name) || line.contains(&target_path); } if !in_target_section { new_content.push_str(line); diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index ff41018..5735409 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -1391,4 +1391,129 @@ active = true "re-added submodule should be staged as a gitlink again" ); } + + /// Adding the same submodule (same name + path) a second time is idempotent: + /// it succeeds without creating duplicate `.gitmodules` or config entries. + #[test] + fn test_add_same_submodule_twice_is_idempotent() { + let harness = TestHarness::new().expect("Failed to create test harness"); + harness.init_git_repo().expect("Failed to init git repo"); + + let remote = harness + .create_test_remote("idem-remote") + .expect("Failed to create remote"); + let url = format!("file://{}", remote.display()); + + harness + .run_submod_success(&["add", &url, "--name", "idem", "--path", "lib/idem"]) + .expect("first add should succeed"); + harness + .run_submod_success(&["add", &url, "--name", "idem", "--path", "lib/idem"]) + .expect("re-adding the same submodule should be a graceful no-op"); + + // Exactly one entry must exist in .gitmodules, git config, and submod.toml. + let gm_raw = std::fs::read_to_string(harness.work_dir.join(".gitmodules")) + .expect("read .gitmodules"); + assert_eq!( + gm_raw.matches("[submodule \"lib/idem\"]").count(), + 1, + ".gitmodules must hold exactly one section for the re-added submodule, got:\n{gm_raw}" + ); + let cfg = harness.submodule_config_entries(); + assert_eq!( + cfg.matches("submodule.lib/idem.url").count(), + 1, + "git config must hold exactly one entry for the submodule, got:\n{cfg}" + ); + let toml = std::fs::read_to_string(harness.config_path()).expect("read submod.toml"); + assert_eq!( + toml.matches("[idem]").count(), + 1, + "submod.toml must hold exactly one [idem] section, got:\n{toml}" + ); + } + + /// Deleting a submodule that does not exist must fail with a specific, + /// informative error — not silently succeed. + #[test] + fn test_delete_nonexistent_submodule_fails() { + let harness = TestHarness::new().expect("Failed to create test harness"); + harness.init_git_repo().expect("Failed to init git repo"); + + let output = harness + .run_submod(&["delete", "ghost"]) + .expect("Failed to run submod"); + assert!( + !output.status.success(), + "deleting a nonexistent submodule must exit non-zero" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("ghost") && stderr.contains("not found"), + "error must name the missing submodule and say it was not found, got: {stderr}" + ); + } + + /// A failed `add` (e.g. an unreachable URL) must not leave partial state + /// behind when a submodule already exists. Regression for the #62 audit (P2): + /// the fallback cleanup matched the `.gitmodules` section by name, but git2 + /// writes the section keyed by path, so a stale `[submodule ""]` entry + /// lingered after the failed add. + #[test] + fn test_failed_add_leaves_no_partial_state() { + let harness = TestHarness::new().expect("Failed to create test harness"); + harness.init_git_repo().expect("Failed to init git repo"); + + let remote = harness + .create_test_remote("survivor-remote") + .expect("Failed to create remote"); + let url = format!("file://{}", remote.display()); + + // A real submodule so .gitmodules already exists when the bad add runs. + harness + .run_submod_success(&["add", &url, "--name", "good", "--path", "lib/good"]) + .expect("baseline add should succeed"); + + // A failed add against an unreachable URL. + let bad = harness + .run_submod(&[ + "add", + "file:///nonexistent/definitely-not-here.git", + "--name", + "bad", + "--path", + "lib/bad", + ]) + .expect("Failed to run submod"); + assert!( + !bad.status.success(), + "an add against an unreachable URL must fail" + ); + + // No stale `bad`/`lib/bad` entry may remain in .gitmodules or git config. + let gm = harness.gitmodules_entries(); + assert!( + !gm.contains("lib/bad") && !gm.contains("\"bad\""), + "failed add left a stale .gitmodules entry:\n{gm}" + ); + let cfg = harness.submodule_config_entries(); + assert!( + !cfg.contains("lib/bad") && !cfg.contains("submodule.bad."), + "failed add left a stale git config entry:\n{cfg}" + ); + assert!( + !harness.dir_exists("lib/bad"), + "failed add left an orphan working-tree directory" + ); + assert!( + !harness.git_modules_dir_exists("bad") && !harness.git_modules_dir_exists("lib/bad"), + "failed add left a dangling .git/modules directory" + ); + + // Non-vacuity: the pre-existing good submodule must survive the cleanup. + assert!( + gm.contains("lib/good"), + "cleanup of the failed add must not remove the existing submodule:\n{gm}" + ); + } }