Address all PR review suggestions

Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>

Author: Copilot

src/commands.rs

@@ -33,13 +33,14 @@ Defines the CLI structure and commands using [`clap`] for managing git submodule
 # Usage Example
 
 ```sh
-submod add my-lib libs/my-lib https://github.com/example/my-lib.git --sparse-paths "src/,include/" --settings "ignore=all"
+submod add https://github.com/example/my-lib.git --name my-lib --path libs/my-lib --sparse-paths "src/,include/"
 submod change my-lib --branch "main" --sparse-paths "src/,include/" --fetch "always" --update "checkout"
 submod check
 submod init
 submod update
 submod reset --all
 submod sync
+submod completeme bash
 ```
 
 # Configuration
@@ -110,7 +111,7 @@ pub enum Commands {
         no_init: bool,
     },
     // TODO: Implement this command
-    #[command(name = "change", next_help_heading = "Change a Submodule's Settings", about = "Change the configuration of an existing submodule. Any field you provide will overwrite an existing value (unless both are defaults). If you change the path, it will nuke-it-from-orbit (delete it and re-clone it).")]
+    #[command(name = "change", hide = true, next_help_heading = "Change a Submodule's Settings", about = "Change the configuration of an existing submodule. Any field you provide will overwrite an existing value (unless both are defaults). If you change the path, it will nuke-it-from-orbit (delete it and re-clone it).")]
     Change {
         #[arg(required = true, value_parser = clap::value_parser!(String), value_hint = clap::ValueHint::CommandName, help = "The name of the submodule to change. Must match an existing submodule.", long_help = "The name of the submodule to change. Must match an existing submodule in your submod.toml. Because we use this value to lookup your config, you cannot change the name from the CLI. You must manually change it in your submod.toml. All other options can be changed here.")]
         name: String,
@@ -146,7 +147,7 @@ pub enum Commands {
         active: bool,
     },
     // TODO: Implement this command
-    #[command(name = "change-global", visible_aliases = ["cg", "chgl", "global"], next_help_heading = "Change Global Settings", about = "Add or change the global settings for submodules, affecting all submodules in the current repository. Any individual submodule settings will override these global settings.")]
+    #[command(name = "change-global", hide = true, visible_aliases = ["cg", "chgl", "global"], next_help_heading = "Change Global Settings", about = "Add or change the global settings for submodules, affecting all submodules in the current repository. Any individual submodule settings will override these global settings.")]
     ChangeGlobal {
 
         #[arg(short = 'i', long = "ignore", help = "Sets the default ignore behavior for all submodules in this repository. This will override any individual submodule settings.")]
@@ -163,7 +164,7 @@ pub enum Commands {
     Check,
 
     // TODO: Implement this command
-    #[command(name = "list", visible_aliases = ["ls", "l"], next_help_heading = "List Submodules", about = "Lists all submodules, optionally recursively.")]
+    #[command(name = "list", hide = true, visible_aliases = ["ls", "l"], next_help_heading = "List Submodules", about = "Lists all submodules, optionally recursively.")]
     List {
         /// Recursively list all submodules for the current repository.
         #[arg(short = 'r', long = "recursive", default_value = "false", action = clap::ArgAction::SetTrue, default_missing_value = "true", help = "If given, lists all submodules recursively (like, the submodules of the submodules).")]
@@ -174,11 +175,11 @@ pub enum Commands {
     Init,
 
     // TODO: Implement this command (use git2 + fs to delete files)
-    #[command(name = "delete", visible_alias = "del", next_help_heading = "Delete a Submodule", about = "Deletes a submodule by name; removes it from the configuration and the filesystem.")]
+    #[command(name = "delete", hide = true, visible_alias = "del", next_help_heading = "Delete a Submodule", about = "Deletes a submodule by name; removes it from the configuration and the filesystem.")]
     Delete,
 
     // TODO: Implement this command (use git2). Functionally this changes a module to `active = false` in our config and `.gitmodules`, but does not delete the submodule from the filesystem.
-    #[command(name = "disable", visible_alias = "d", next_help_heading = "Disable a Submodule", about = "Disables a submodule by name; sets its active status to false. Does not remove settings or files.")]
+    #[command(name = "disable", hide = true, visible_alias = "d", next_help_heading = "Disable a Submodule", about = "Disables a submodule by name; sets its active status to false. Does not remove settings or files.")]
     Disable,
 
     #[command(name = "update", visible_alias = "u", next_help_heading = "Update Submodules", about = "Updates all submodules to their configured state.")]
@@ -198,7 +199,7 @@ pub enum Commands {
     Sync,
 
     // TODO: Implement this command
-    #[command(name = "generate-config", visible_aliases = ["gc", "genconf"], next_help_heading = "Generate a Config File", about = "Generates a new configuration file.")]
+    #[command(name = "generate-config", hide = true, visible_aliases = ["gc", "genconf"], next_help_heading = "Generate a Config File", about = "Generates a new configuration file.")]
     GenerateConfig {
         /// Path to the new configuration file to generate.
         #[arg(short = 'o', long = "output", value_parser = clap::value_parser!(PathBuf), value_hint = clap::ValueHint::FilePath, default_value = "submod.toml", help = "Path to the output configuration file. Defaults to submod.toml in the current directory.")]
@@ -215,7 +216,7 @@ pub enum Commands {
     },
 
     // TODO: Implement this command (use git2) (not we can leverage this logic for `delete` because the `kill` option is the same.)
-    #[command(name = "nuke-it-from-orbit", visible_aliases = ["nuke-em", "nuke-it", "nuke-them"], next_help_heading = "Nuke It From Orbit", about = "Deletes all submodules or specific ones, removing them from the configuration and the filesystem. Optionally leaves them dead. 🚀💥👾💥💀.")]
+    #[command(name = "nuke-it-from-orbit", hide = true, visible_aliases = ["nuke-em", "nuke-it", "nuke-them"], next_help_heading = "Nuke It From Orbit", about = "Deletes all submodules or specific ones, removing them from the configuration and the filesystem. Optionally leaves them dead. 🚀💥👾💥💀.")]
     NukeItFromOrbit {
         #[arg(long = "all", default_value = "false", action = clap::ArgAction::SetTrue, default_missing_value = "true", help = "Nuke 'em all? 🤓")]
         all: bool,
@@ -226,8 +227,8 @@ pub enum Commands {
         kill: bool,
     },
 
-    // TODO: Implement this command (super simple with clap_complete/clap_complete_nushell. The latter is just another enum variant that implements the `Generator` trait like all of the other clap_complete shells.)
-    #[command(name = "completeme", visible_aliases = ["comp", "complete", "comp-me", "complete-me"], next_help_heading = "Generate Shell Completions", about = "Generates shell completions for the specified shell. Completions generated to stdout.", long_about = COMPLETE_ME, value_parser = clap::value_parser!(Shell))]
+    // Shell completions are implemented using clap_complete/clap_complete_nushell
+    #[command(name = "completeme", visible_aliases = ["comp", "complete", "comp-me", "complete-me"], next_help_heading = "Generate Shell Completions", about = "Generates shell completions for the specified shell. Completions generated to stdout.", long_about = COMPLETE_ME)]
     CompleteMe {
         #[arg(value_enum, action = clap::ArgAction::Set, help = "The shell to generate completions for. Supported shells: `bash`, `zsh`, `fish`, `powershell`, `elvish`, `nushell`.")]
         shell: Shell,

src/config.rs

@@ -492,7 +492,7 @@ impl SubmoduleEntry {
         };
         let branch = SerializableBranch::from_gitmodules(entries.get("branch").map_or("", |b| b.as_str())).ok();
         let ignore = entries.get("ignore").and_then(|i| SerializableIgnore::from_gitmodules(i).ok());
-        let fetch_recurse = entries.get("fetchRecurse").and_then(|fr| SerializableFetchRecurse::from_gitmodules(fr).ok());
+        let fetch_recurse = entries.get("fetchRecurseSubmodules").and_then(|fr| SerializableFetchRecurse::from_gitmodules(fr).ok());
         let update = entries.get("update").and_then(|u| SerializableUpdate::from_gitmodules(u).ok());
         let active = entries.get("active").and_then(|a| a.parse::<bool>().ok()).unwrap_or(true);
         let shallow = entries.get("shallow").and_then(|s| s.parse::<bool>().ok()).unwrap_or(false);

src/git_ops/git2_ops.rs

@@ -90,7 +90,8 @@ impl Git2Operations {
 
         match config.get_string(&key) {
             Ok(fetch_str) => match fetch_str.as_str() {
-                "true" | "on-demand" => Ok(Some(SerializableFetchRecurse::OnDemand)),
+                "true" => Ok(Some(SerializableFetchRecurse::Always)),
+                "on-demand" => Ok(Some(SerializableFetchRecurse::OnDemand)),
                 "false" | "no" => Ok(Some(SerializableFetchRecurse::Never)),
                 _ => Ok(None),
             },

src/git_ops/gix_ops.rs

@@ -74,8 +74,8 @@ impl GixOperations {
     for section in as_config_file.sections() {
         // we need to convert everything to String and add to map
         let mut section_entries = std::collections::HashMap::new();
-        let name = if section.header().subsection_name().is_some() {
-            section.header().name().to_string()
+        let name = if let Some(subsection) = section.header().subsection_name() {
+            subsection.to_str_lossy().to_string()
         } else {
             section.header().name().to_string()
         };

src/git_ops/simple_gix.rs

@@ -1,4 +1,4 @@
-// SPDX-LicenseIdentifier: MIT OR Apache-2.0
+// SPDX-License-Identifier: MIT OR Apache-2.0
 //
 // SPDX-FileCopyrightText: 2018-2025 Sebastian Thiel and [contributors](https://github.com/byron/gitoxide/contributors)
 // SPDX-FileCopyrightText: 2025 Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>

src/main.rs

@@ -35,6 +35,7 @@ use crate::options::SerializableBranch as Branch;
 use crate::git_manager::GitManager;
 use anyhow::Result;
 use clap::Parser;
+use clap_complete::generate;
 
 
 fn main() -> Result<()> {
@@ -141,7 +142,7 @@ fn main() -> Result<()> {
             }
         }
         Commands::Sync => {
-            let mut manager = GitManager::new(cli.config)
+            let mut manager = GitManager::new(config_path)
                 .map_err(|e| anyhow::anyhow!("Failed to create manager: {}", e))?;
 
             // Run check, init, and update in sequence
@@ -189,8 +190,10 @@ fn main() -> Result<()> {
         Commands::NukeItFromOrbit { .. } => {
             return Err(anyhow::anyhow!("NukeItFromOrbit command not yet implemented"));
         }
-        Commands::CompleteMe { .. } => {
-            return Err(anyhow::anyhow!("Completions command not yet implemented"));
+        Commands::CompleteMe { shell } => {
+            let mut cmd = <Cli as clap::CommandFactory>::command();
+            let name = cmd.get_name().to_string();
+            generate(shell, &mut cmd, name, &mut std::io::stdout());
         }
     }
 

src/options.rs

@@ -423,25 +423,14 @@ impl GitmodulesConvert for SerializableBranch {
     /// Convert to gitmodules string (what you would get from the .gitmodules or .git/config)
     fn to_gitmodules(&self) -> String {
         match self {
-            SerializableBranch::CurrentInSuperproject => get_current_repository()
-                .map(|repo| {
-                    let branch = get_current_branch(Some(&repo))
-                        .unwrap_or_else(|_| "current-in-super-project".to_string());
-                    if branch.is_empty() {
-                        "current-in-super-project".to_string()
-                    } else {
-                        branch
-                    }
-                })
-                .unwrap_or_else(|_| "current-in-super-project".to_string()),
+            SerializableBranch::CurrentInSuperproject => ".".to_string(),
             SerializableBranch::Name(name) => name.to_string(),
         }
     }
 
     /// Convert from gitmodules string (what you would get from the .gitmodules or .git/config)
     fn from_gitmodules(options: &str) -> Result<Self, ()> {
-
-        if options == "." || options == "current" || options == "current-in-super-project" || options == "superproject" || options == "super" || options == SerializableBranch::current_in_superproject().unwrap_or_default() {
+        if options == "." {
             return Ok(SerializableBranch::CurrentInSuperproject);
         }
         Ok(SerializableBranch::Name(options.to_string()))

src/shells.rs

@@ -49,7 +49,7 @@ impl clap::ValueEnum for Shell {
             Shell::Bash => PossibleValue::new("bash"),
             Shell::Elvish => PossibleValue::new("elvish"),
             Shell::Fish => PossibleValue::new("fish"),
-            Shell::PowerShell => PossibleValue::new("powershell"),
+            Shell::PowerShell => PossibleValue::new("powershell").alias("pwsh"),
             Shell::Zsh => PossibleValue::new("zsh"),
             Shell::Nushell => PossibleValue::new("nushell"),
         })

src/utilities.rs

@@ -82,9 +82,13 @@ pub fn get_main_root(
 
 /// Get the current branch name from the repository.
 pub fn get_current_branch(repo: Option<&gix::Repository>) -> Result<String, anyhow::Error> {
+    let owned;
     let repo = match repo {
         Some(r) => r,
-        None => &get_current_repository()?,
+        None => {
+            owned = get_current_repository()?;
+            &owned
+        }
     };
     let head = repo.head()?;
     if let Some(reference) = head.referent_name() {
@@ -153,19 +157,17 @@ pub fn name_from_url(url: &str) -> Result<String, anyhow::Error> {
 /// Convert an `OsString` to a `String`, extracting the name from the path
 pub 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"));
         }
+        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"))
     })
 }
 

tests/config_tests.rs

@@ -192,11 +192,11 @@ active = true
         harness
             .run_submod_success(&[
                 "add",
+                &remote_url,
                 "--name",
                 "new-submodule",
                 "--path",
                 "lib/new",
-                &remote_url,
                 "--sparse-paths",
                 "src,docs",
             ])