fix(config): round-trip submod.toml — preserve [defaults] on load, persist edits on save (#62 P1) (#70)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: bashandbone

src/config.rs

@@ -1091,14 +1091,50 @@ impl Config {
         Ok(())
     }
 
+    /// Overlay CLI-supplied options onto this configuration.
+    ///
+    /// Only fields explicitly set on the CLI override the current values: a
+    /// `None` `[defaults]` field (or an empty submodule set) leaves the existing
+    /// value intact, so an absent CLI flag never erases configuration the file
+    /// provided. Merging the whole `cli_options` struct through figment instead
+    /// would serialize its unset `[defaults]` fields as nulls and clobber the
+    /// file's values (#62 P1).
+    fn merge_cli_overrides(&mut self, cli: Self) {
+        let cli_defaults = cli.defaults;
+        if cli_defaults.ignore.is_some() {
+            self.defaults.ignore = cli_defaults.ignore;
+        }
+        if cli_defaults.fetch_recurse.is_some() {
+            self.defaults.fetch_recurse = cli_defaults.fetch_recurse;
+        }
+        if cli_defaults.update.is_some() {
+            self.defaults.update = cli_defaults.update;
+        }
+        if cli_defaults.use_git_default_sparse_checkout.is_some() {
+            self.defaults.use_git_default_sparse_checkout =
+                cli_defaults.use_git_default_sparse_checkout;
+        }
+        // CLI submodule entries override/extend by name (no-op when none given).
+        for (name, entry) in cli.submodules {
+            self.submodules.update_entry(name, entry);
+        }
+    }
+
     /// Load configuration from a file, merging with CLI options
     pub fn load(&self, path: impl AsRef<Path>, cli_options: Self) -> anyhow::Result<Self> {
-        let fig = Figment::from(Self::default()) // 1) start from Rust-side defaults
-            .merge(Toml::file(path)) // 2) file-based overrides
-            .merge(cli_options); // 3) CLI overrides file
-
-        // 4) extract into Config, then post-process submodules
-        let cfg: Self = fig.extract()?;
+        // 1) Read the file's values. NOTE: layering an empty `Config::default()`
+        //    provider beneath the file (the previous approach) actively *erased*
+        //    the file's `[defaults]` — that provider emits all-`None` defaults
+        //    under its own figment profile (`REPO`), which then overrode the
+        //    file's values. Rust-side defaults are filled by `apply_defaults()`
+        //    below, not by a figment base layer (#62 P1).
+        let mut cfg: Self = Figment::from(Toml::file(path)).extract()?;
+
+        // 2) CLI overrides the file, but only where the CLI actually set a value
+        //    (None-aware — see `merge_cli_overrides`).
+        cfg.merge_cli_overrides(cli_options);
+
+        // 3) post-process submodules
         Ok(cfg.apply_defaults())
     }
 
@@ -1108,9 +1144,10 @@ impl Config {
             Some(ref p) => p,
             None => &".",
         };
-        let fig = Figment::from(Self::default()).merge(Toml::file(p));
-        // Extract the configuration from Figment
-        let cfg: Self = fig.extract()?;
+        // See `load`: an empty `Config::default()` base layer erases the file's
+        // `[defaults]`, so read the file directly and let `apply_defaults()`
+        // supply Rust-side defaults (#62 P1).
+        let cfg: Self = Figment::from(Toml::file(p)).extract()?;
         Ok(cfg.apply_defaults())
     }
 
@@ -2165,6 +2202,118 @@ active = true
         );
     }
 
+    // ================================================================
+    // Config::load — CLI-over-file precedence
+    // ================================================================
+
+    #[test]
+    fn test_config_load_cli_overrides_file_but_preserves_unspecified() {
+        // `Config::load` layers three sources: Rust-side defaults → submod.toml
+        // → cli_options. A value supplied via `cli_options` must override the
+        // same key from the file, while a key the CLI leaves unset must keep the
+        // file's value — an absent CLI flag must not erase file configuration.
+        // This precedence contract was previously untested (#62 P1).
+        figment::Jail::expect_with(|jail| {
+            jail.create_file(
+                "submod.toml",
+                r#"
+[defaults]
+ignore = "all"
+update = "rebase"
+"#,
+            )?;
+
+            // The CLI overrides `ignore` and leaves `update` unspecified.
+            let mut cli_options = Config::default();
+            cli_options.defaults.ignore = Some(SerializableIgnore::Dirty);
+
+            let cfg = Config::default()
+                .load("submod.toml", cli_options)
+                .expect("load should succeed");
+
+            assert_eq!(
+                cfg.defaults.ignore,
+                Some(SerializableIgnore::Dirty),
+                "CLI-supplied `ignore` must override the file's value"
+            );
+            assert_eq!(
+                cfg.defaults.update,
+                Some(SerializableUpdate::Rebase),
+                "file's `update` must survive when the CLI leaves it unspecified"
+            );
+
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn test_config_load_default_cli_preserves_file_defaults() {
+        // The production callers (`GitManager::with_verbose`) load config with
+        // `cli_options = Config::default()`, whose `[defaults]` are all `None`.
+        // Those unset CLI fields must not erase the file's `[defaults]`; the
+        // whole `[defaults]` block was previously clobbered to `None` on every
+        // load (#62 P1).
+        figment::Jail::expect_with(|jail| {
+            jail.create_file(
+                "submod.toml",
+                r#"
+[defaults]
+ignore = "all"
+update = "rebase"
+fetchRecurse = "always"
+"#,
+            )?;
+
+            let cfg = Config::default()
+                .load("submod.toml", Config::default())
+                .expect("load should succeed");
+
+            assert_eq!(
+                cfg.defaults.ignore,
+                Some(SerializableIgnore::All),
+                "file `[defaults] ignore` must survive a default-CLI load"
+            );
+            assert_eq!(
+                cfg.defaults.update,
+                Some(SerializableUpdate::Rebase),
+                "file `[defaults] update` must survive a default-CLI load"
+            );
+            assert_eq!(
+                cfg.defaults.fetch_recurse,
+                Some(SerializableFetchRecurse::Always),
+                "file `[defaults] fetchRecurse` must survive a default-CLI load"
+            );
+
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn test_config_load_from_file_preserves_file_defaults() {
+        // `load_from_file` shares the figment-base hazard with `load`: an empty
+        // default provider layered beneath the file erased the file's
+        // `[defaults]`. The file's values must survive (#62 P1).
+        figment::Jail::expect_with(|jail| {
+            jail.create_file(
+                "submod.toml",
+                r#"
+[defaults]
+ignore = "all"
+update = "rebase"
+"#,
+            )?;
+
+            let cfg = Config::default()
+                .load_from_file(Some("submod.toml"))
+                .expect("load_from_file should succeed");
+
+            assert_eq!(cfg.defaults.ignore, Some(SerializableIgnore::All));
+            assert_eq!(cfg.defaults.update, Some(SerializableUpdate::Rebase));
+
+            Ok(())
+        });
+    }
+
     // ================================================================
     // SubmoduleEntries::from_gitmodules
     // ================================================================

src/git_manager.rs

@@ -206,108 +206,15 @@ impl GitManager {
         self.save_config()
     }
 
-    /// Save the current in-memory configuration to the config file
+    /// Save the current in-memory configuration to the config file.
+    ///
+    /// Delegates to [`write_full_config`], which rewrites the file
+    /// section-by-section: it preserves the preamble, comments, `[defaults]`,
+    /// and any unknown keys, *updates* the bodies of sections that already
+    /// exist, and appends sections that are new. The previous implementation
+    /// was append-only and silently dropped edits to existing sections (#62 P1).
     fn save_config(&self) -> Result<(), SubmoduleError> {
-        // Read existing TOML to preserve content (defaults, comments, existing entries)
-        let existing = if self.config_path.exists() {
-            std::fs::read_to_string(&self.config_path)
-                .map_err(|e| SubmoduleError::ConfigError(format!("Failed to read config: {e}")))?
-        } else {
-            String::new()
-        };
-
-        let mut output = existing.clone();
-
-        // Append any new submodule sections not already in the file
-        for (name, entry) in self.config.get_submodules() {
-            // Determine whether this name needs quoting (contains TOML-special characters).
-            // Simple names (alphanumeric, hyphens, underscores) can use the bare [name] form.
-            let needs_quoting = name
-                .chars()
-                .any(|c| !c.is_alphanumeric() && c != '-' && c != '_');
-            let escaped_name = name.replace('\\', "\\\\").replace('"', "\\\"");
-            let section_header = if needs_quoting {
-                format!("[\"{escaped_name}\"]")
-            } else {
-                format!("[{name}]")
-            };
-            // Check at line boundaries to avoid false positives from comments/values.
-            // Accept either quoted or unquoted form so existing files written before this
-            // change are recognised.
-            let already_present = existing.lines().any(|line| {
-                let trimmed = line.trim();
-                trimmed == section_header
-                    || trimmed == format!("[{name}]")
-                    || trimmed == format!("[\"{escaped_name}\"]")
-            });
-            if !already_present {
-                output.push('\n');
-                output.push_str(&section_header);
-                output.push('\n');
-                if let Some(path) = &entry.path {
-                    output.push_str(&format!(
-                        "path = \"{}\"\n",
-                        path.replace('\\', "\\\\").replace('"', "\\\"")
-                    ));
-                }
-                if let Some(url) = &entry.url {
-                    output.push_str(&format!(
-                        "url = \"{}\"\n",
-                        url.replace('\\', "\\\\").replace('"', "\\\"")
-                    ));
-                }
-                if let Some(branch) = &entry.branch {
-                    let val = branch.to_string();
-                    if !val.is_empty() {
-                        output.push_str(&format!(
-                            "branch = \"{}\"\n",
-                            val.replace('\\', "\\\\").replace('"', "\\\"")
-                        ));
-                    }
-                }
-                if let Some(ignore) = &entry.ignore {
-                    let val = ignore.to_string();
-                    if !val.is_empty() {
-                        output.push_str(&format!("ignore = \"{val}\"\n"));
-                    }
-                }
-                if let Some(fetch_recurse) = &entry.fetch_recurse {
-                    let val = fetch_recurse.as_config_value();
-                    if !val.is_empty() {
-                        output.push_str(&format!("fetchRecurse = \"{val}\"\n"));
-                    }
-                }
-                if let Some(update) = &entry.update {
-                    let val = update.to_string();
-                    if !val.is_empty() {
-                        output.push_str(&format!("update = \"{val}\"\n"));
-                    }
-                }
-                if let Some(active) = entry.active {
-                    output.push_str(&format!("active = {active}\n"));
-                }
-                if let Some(shallow) = entry.shallow
-                    && shallow
-                {
-                    output.push_str("shallow = true\n");
-                }
-                if let Some(sparse_paths) = &entry.sparse_paths
-                    && !sparse_paths.is_empty()
-                {
-                    let joined = sparse_paths
-                        .iter()
-                        .map(|p| format!("\"{}\"", p.replace('\\', "\\\\").replace('"', "\\\"")))
-                        .collect::<Vec<_>>()
-                        .join(", ");
-                    output.push_str(&format!("sparse_paths = [{joined}]\n"));
-                }
-            }
-        }
-
-        std::fs::write(&self.config_path, &output).map_err(|e| {
-            SubmoduleError::ConfigError(format!("Failed to write config file: {e}"))
-        })?;
-        Ok(())
+        self.write_full_config()
     }
 
     /// Creates a new `GitManager` by loading configuration from the given path
@@ -2080,6 +1987,71 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_save_config_persists_edits_to_existing_section() {
+        // `save_config` (the writer used by `add`) was append-only: once a
+        // submodule's section existed in submod.toml, later edits to that entry
+        // were silently not written back, because the section header was
+        // "already present". A load→modify→save→reload must reflect the edit
+        // (#62 P1).
+        let temp_dir = tempdir().unwrap();
+        let config_path = temp_dir.path().join("submod.toml");
+        let mut manager = create_test_manager(temp_dir.path(), config_path.clone());
+
+        // Seed an existing [mymod] section tracking branch = "main".
+        manager.config.add_submodule(
+            "mymod".to_string(),
+            SubmoduleEntry::new(
+                Some("https://example.com/repo.git".to_string()),
+                Some("libs/mymod".to_string()),
+                Some(SerializableBranch::Name("main".to_string())),
+                None,
+                None,
+                None,
+                Some(true),
+                None,
+                None,
+            ),
+        );
+        manager.save_config().expect("initial save");
+
+        // Precondition (guards against a vacuous pass): the section was written
+        // with branch = "main".
+        let first = fs::read_to_string(&config_path).unwrap();
+        let parsed_first: Config = toml::from_str(&first).expect("initial save must be valid TOML");
+        assert_eq!(
+            parsed_first.submodules.get("mymod").unwrap().branch,
+            Some(SerializableBranch::Name("main".to_string())),
+            "precondition: initial save must record branch=main; file:\n{first}"
+        );
+
+        // Modify the existing entry: branch main → develop.
+        manager.config.add_submodule(
+            "mymod".to_string(),
+            SubmoduleEntry::new(
+                Some("https://example.com/repo.git".to_string()),
+                Some("libs/mymod".to_string()),
+                Some(SerializableBranch::Name("develop".to_string())),
+                None,
+                None,
+                None,
+                Some(true),
+                None,
+                None,
+            ),
+        );
+        manager.save_config().expect("second save");
+
+        // The edit must be persisted, not dropped because the section pre-existed.
+        let second = fs::read_to_string(&config_path).unwrap();
+        let reloaded: Config = toml::from_str(&second).expect("second save must be valid TOML");
+        assert_eq!(
+            reloaded.submodules.get("mymod").unwrap().branch,
+            Some(SerializableBranch::Name("develop".to_string())),
+            "save_config must persist edits to an existing section; file:\n{second}"
+        );
+    }
+
     #[test]
     fn test_sparse_checkout_mismatch() {
         let temp_dir = tempdir().unwrap();