Address PR review feedback: security, CLI ergonomics, git2 add_submodule, and correctness fixes

.roo/mcp.json

@@ -25,7 +25,7 @@
             "args": [
                 "-y",
                 "@modelcontextprotocol/server-filesystem",
-                "~/submod", "~/.cargo"
+                "~/submod"
             ],
             "alwaysAllow": [
                 "read_file",

Cargo.toml

@@ -8,7 +8,6 @@ version = "0.2.0"
 edition = "2024"
 rust-version = "1.87"
 description = "A headache-free submodule management tool, built on top of gitoxide. Manage sparse checkouts, submodule updates, and adding/removing submodules with ease."
-license = "MIT" # Plain MIT license: plainlicense.org/licenses/permissive/mit/
 license-file = "LICENSE.md"
 repository = "https://github.com/bashandbone/submod"
 homepage = "https://github.com/bashandbone/submod"
@@ -20,15 +19,6 @@ keywords = [
   "gitoxide",
   "cli",
   "sparse-checkout",
-  "git2",
-  "gitmodules",
-  "repository",
-  "management",
-  "command-line",
-  "tool",
-  "development",
-  "utilities",
-  "development-tools",
 ]
 categories = ["command-line-utilities", "development-tools"]
 

README.md

@@ -245,15 +245,15 @@ submod list --recursive
 Delete a submodule from configuration and filesystem:
 
 ```bash
-submod delete
+submod delete my-lib
 ```
 
 ### `submod disable`
 
 Disable a submodule without deleting files (sets `active = false`):
 
 ```bash
-submod disable
+submod disable my-lib
 ```
 
 ### `submod nuke-it-from-orbit`

sample_config/submod.toml

@@ -59,7 +59,7 @@ ignore = "dirty" # Override default ignore setting for all submodules
 #
 # ## `shallow`
 #
-# If `true`, performs a shallow clone of the submodule, which means it only fetchest the most recent commit. Defaults to `false`. This is useful for large repositories where you only need the latest commit.
+# If `true`, performs a shallow clone of the submodule, which means it only fetches the most recent commit. Defaults to `false`. This is useful for large repositories where you only need the latest commit.
 #
 
 # NAMES (the part between "[" and "]" below).

src/commands.rs

@@ -101,8 +101,8 @@ pub enum Commands {
         )]
         branch: Option<String>,
 
-        #[arg(short = 'i', long = "ignore", default_value = "unspecified", help = "What changes in the submodule git should ignore.")]
-        ignore: Ignore,
+        #[arg(short = 'i', long = "ignore", help = "What changes in the submodule git should ignore.")]
+        ignore: Option<Ignore>,
 
         #[arg(
             short = 'x',
@@ -112,11 +112,11 @@ pub enum Commands {
         )]
         sparse_paths: Option<Vec<String>>,
 
-        #[arg(short = 'f', long = "fetch", default_value = "unspecified", help = "Sets the recursive fetch behavior for the submodule (like, if we should fetch its submodules).")]
-        fetch: FetchRecurse,
+        #[arg(short = 'f', long = "fetch", help = "Sets the recursive fetch behavior for the submodule (like, if we should fetch its submodules).")]
+        fetch: Option<FetchRecurse>,
 
-        #[arg(short = 'u', long = "update", default_value = "unspecified", help = "How git should update the submodule when you run `git submodule update`.")]
-        update: Update,
+        #[arg(short = 'u', long = "update", help = "How git should update the submodule when you run `git submodule update`.")]
+        update: Option<Update>,
 
         // TODO: Implement this arg
         #[arg(short = 's', long = "shallow", default_value = "false", action = clap::ArgAction::SetTrue, default_missing_value = "true", help = "If given, sets the submodule as a shallow clone. It will only fetch the last commit of the branch, not the full history.")]

src/git_ops/git2_ops.rs

@@ -288,12 +288,92 @@ impl GitOperations for Git2Operations {
         Ok(())
     }
     fn add_submodule(&mut self, opts: &SubmoduleAddOptions) -> Result<()> {
-        // git2 submodule cloning requires remote callbacks that are complex to configure.
-        // Fall through to the CLI fallback which handles this reliably.
-        Err(anyhow::anyhow!(
-            "Unable to add submodule '{}' using the library API; it will be added using the Git CLI instead",
-            opts.name
-        ))
+        // 1. Create submodule entry in .gitmodules and index
+        let mut sub = self
+            .repo
+            .submodule(&opts.url, opts.path.as_path(), true)
+            .with_context(|| {
+                format!(
+                    "Failed to create submodule entry for '{}' from '{}'",
+                    opts.name, opts.url
+                )
+            })?;
+
+        // 2. Configure clone options
+        let mut update_opts = git2::SubmoduleUpdateOptions::new();
+        let mut fetch_opts = git2::FetchOptions::new();
+        if opts.shallow {
+            fetch_opts.depth(1);
+        }
+        update_opts.fetch(fetch_opts);
+
+        // 3. Clone the submodule repository
+        sub.clone(Some(&mut update_opts)).with_context(|| {
+            format!(
+                "Failed to clone submodule '{}' from '{}'",
+                opts.name, opts.url
+            )
+        })?;
+
+        // 4. Add to index and finalize
+        sub.add_to_index(true)
+            .with_context(|| format!("Failed to add submodule '{}' to index", opts.name))?;
+        sub.add_finalize()
+            .with_context(|| format!("Failed to finalize submodule '{}'", opts.name))?;
+
+        // 5. Apply optional configuration via git config.
+        // git2's submodule() keys the submodule by path; use the path as the config key.
+        let path_str = opts.path.to_string_lossy();
+        let mut config = self
+            .repo
+            .config()
+            .with_context(|| "Failed to open git config")?;
+
+        // Set branch if specified
+        if let Some(branch) = &opts.branch {
+            let branch_key = format!("submodule.{}.branch", path_str);
+            config
+                .set_str(&branch_key, &branch.to_string())
+                .with_context(|| format!("Failed to set branch for submodule '{}'", opts.name))?;
+        }
+
+        // Set ignore rule if specified and not the sentinel Unspecified value
+        if let Some(ignore) = &opts.ignore {
+            if !matches!(ignore, SerializableIgnore::Unspecified) {
+                let ignore_key = format!("submodule.{}.ignore", path_str);
+                config
+                    .set_str(&ignore_key, &ignore.to_string())
+                    .with_context(|| {
+                        format!("Failed to set ignore for submodule '{}'", opts.name)
+                    })?;
+            }
+        }
+
+        // Set fetch recurse if specified and not the sentinel Unspecified value
+        if let Some(fetch_recurse) = &opts.fetch_recurse {
+            if !matches!(fetch_recurse, SerializableFetchRecurse::Unspecified) {
+                let fetch_key = format!("submodule.{}.fetchRecurseSubmodules", path_str);
+                config
+                    .set_str(&fetch_key, &fetch_recurse.to_string())
+                    .with_context(|| {
+                        format!("Failed to set fetchRecurse for submodule '{}'", opts.name)
+                    })?;
+            }
+        }
+
+        // Set update strategy if specified and not the sentinel Unspecified value
+        if let Some(update) = &opts.update {
+            if !matches!(update, SerializableUpdate::Unspecified) {
+                let update_key = format!("submodule.{}.update", path_str);
+                config
+                    .set_str(&update_key, &update.to_string())
+                    .with_context(|| {
+                        format!("Failed to set update for submodule '{}'", opts.name)
+                    })?;
+            }
+        }
+
+        Ok(())
     }
     fn init_submodule(&mut self, path: &str) -> Result<()> {
         let mut submodule = self

src/git_ops/mod.rs

@@ -256,17 +256,20 @@ impl GitOperations for GitOpsManager {
         .or_else(|_| {
             let workdir = self.git2_ops.workdir()
                 .ok_or_else(|| anyhow::anyhow!("Repository has no working directory"))?;
-            let output = std::process::Command::new("git")
-                .current_dir(workdir)
+            let mut cmd = std::process::Command::new("git");
+            cmd.current_dir(workdir)
                 .arg("submodule")
                 .arg("add")
                 .arg("--name")
-                .arg(&opts.name)
-                .arg("--")
-                .arg(&opts.url)
-                .arg(&opts.path)
-                .output()
-                .context("Failed to run git submodule add")?;
+                .arg(&opts.name);
+            if let Some(branch) = &opts.branch {
+                cmd.arg("--branch").arg(branch.to_string());
+            }
+            if opts.shallow {
+                cmd.arg("--depth").arg("1");
+            }
+            cmd.arg("--").arg(&opts.url).arg(&opts.path);
+            let output = cmd.output().context("Failed to run git submodule add")?;
             if output.status.success() {
                 Ok(())
             } else {

src/main.rs

@@ -81,9 +81,9 @@ fn main() -> Result<()> {
                     set_url,
                     sparse_paths_vec,
                     Some(set_branch),
-                    Some(ignore),
-                    Some(fetch),
-                    Some(update),
+                    ignore,
+                    fetch,
+                    update,
                     Some(shallow),
                     no_init,
                 )

src/options.rs

@@ -125,6 +125,7 @@ pub enum SerializableIgnore {
     None,
     /// Used as a sentinel value internally; do not use in a submod.toml or submod CLI command.
     #[serde(skip)]
+    #[value(skip)]
     Unspecified,
 }
 
@@ -279,6 +280,7 @@ pub enum SerializableFetchRecurse {
     Never,
     /// Used as a sentinel value internally; do not use in a submod.toml or submod CLI command.
     #[serde(skip)]
+    #[value(skip)]
     Unspecified,
 }
 
@@ -557,6 +559,7 @@ pub enum SerializableUpdate {
     None,
     /// Used as a sentinel value internally; do not use in a submod.toml or submod CLI command.
     #[serde(skip)]
+    #[value(skip)]
     Unspecified,
 }
 

src/utilities.rs

@@ -3,11 +3,10 @@
 // SPDX-License-Identifier: LicenseRef-PlainMIT OR MIT
 //! Utility functions for working with `Gitoxide` APIs commonly used across the codebase.
 
-use anyhow::Ok;
+use anyhow::Result;
 use git2::Repository as Git2Repository;
 use gix::open::Options;
 use std::path::PathBuf;
-use std::result::Result;
 
 /// Get the current repository using git2, with an optional provided repository. If no repository is provided, it will attempt to discover one in the current directory.
 pub(crate) fn get_current_git2_repository(
@@ -164,20 +163,18 @@ pub(crate) fn name_from_url(url: &str) -> Result<String, anyhow::Error> {
 /// Convert an `OsString` to a `String`, extracting the name from the path
 pub(crate) fn name_from_osstring(os_string: std::ffi::OsString) -> Result<String, anyhow::Error> {
     osstring_to_string(os_string).and_then(|s| {
-        if s.is_empty() {
-            if s.contains('\0') {
-                Err(anyhow::anyhow!("Name cannot contain null bytes"))
-            } else {
-                Ok(s)
-            }
-        } else {
-            let sep = std::path::MAIN_SEPARATOR.to_string();
-            s.trim()
-                .split(&sep)
-                .last()
-                .map(|name| name.to_string())
-                .ok_or_else(|| anyhow::anyhow!("Failed to extract name from OsString"))
+        if s.contains('\0') {
+            return Err(anyhow::anyhow!("Name cannot contain null bytes"));
+        }
+        if s.trim().is_empty() {
+            return Err(anyhow::anyhow!("Name cannot be empty or whitespace-only"));
         }
+        let sep = std::path::MAIN_SEPARATOR.to_string();
+        s.trim()
+            .split(&sep)
+            .last()
+            .map(|name| name.to_string())
+            .ok_or_else(|| anyhow::anyhow!("Failed to extract name from OsString"))
     })
 }