fix: accept plugin install options before the repository (#4453)

refs #4446

Co-authored-by: akbash-bot <300245827+akbash-bot@users.noreply.github.com>
Co-authored-by: JJ Liebig <jonathan.liebig@gmail.com>

2 files changed, 139 insertions, 21 deletions

diff --git a/src/cli/plugin.rs b/src/cli/plugin.rs
index a2c1e6c3..46caf509 100644
--- a/src/cli/plugin.rs
+++ b/src/cli/plugin.rs
@@ -15,6 +15,8 @@ use crate::api::schema::{
 use crate::popup_size::PopupSize;
 
 const PLUGIN_BUILD_OUTPUT_MAX_BYTES: usize = 64 * 1024;
+const PLUGIN_INSTALL_USAGE: &str =
+    "usage: herdr plugin install [--ref REF] [--yes|-y] <owner>/<repo>[/subdir...]";
 
 pub(super) fn run_plugin_command(args: &[String]) -> std::io::Result<i32> {
     let Some(subcommand) = args.first().map(|arg| arg.as_str()) else {
@@ -151,39 +153,58 @@ fn plugin_unlink(args: &[String]) -> std::io::Result<i32> {
     }))
 }
 
-fn plugin_install(args: &[String]) -> std::io::Result<i32> {
-    let Some(source_arg) = args.first() else {
-        eprintln!("usage: herdr plugin install <owner>/<repo>[/subdir...] [--ref REF] [--yes]");
-        return Ok(2);
-    };
-    let source = match GithubPluginSource::parse(source_arg) {
-        Ok(source) => source,
-        Err(err) => {
-            eprintln!("{err}");
-            return Ok(2);
-        }
-    };
+#[derive(Debug)]
+struct PluginInstallArgs {
+    source: GithubPluginSource,
+    requested_ref: Option<String>,
+    yes: bool,
+}
+
+fn parse_plugin_install_args(args: &[String]) -> Result<PluginInstallArgs, String> {
+    let mut source_arg = None;
     let mut requested_ref = None;
     let mut yes = false;
-    let mut index = 1;
+    let mut index = 0;
     while index < args.len() {
         match args[index].as_str() {
             "--ref" => {
-                let Some(value) = required_value(args, &mut index, "--ref") else {
-                    return Ok(2);
-                };
-                requested_ref = Some(value);
+                let value = args.get(index + 1).ok_or("missing value for --ref")?;
+                requested_ref = Some(value.clone());
+                index += 2;
             }
             "--yes" | "-y" => {
                 yes = true;
                 index += 1;
             }
+            other if other.starts_with('-') || source_arg.is_some() => {
+                return Err(format!("unknown option: {other}"));
+            }
             other => {
-                eprintln!("unknown option: {other}");
-                return Ok(2);
+                source_arg = Some(other);
+                index += 1;
             }
         }
     }
+    let source = GithubPluginSource::parse(source_arg.ok_or(PLUGIN_INSTALL_USAGE)?)?;
+    Ok(PluginInstallArgs {
+        source,
+        requested_ref,
+        yes,
+    })
+}
+
+fn plugin_install(args: &[String]) -> std::io::Result<i32> {
+    let PluginInstallArgs {
+        source,
+        requested_ref,
+        yes,
+    } = match parse_plugin_install_args(args) {
+        Ok(args) => args,
+        Err(err) => {
+            eprintln!("{err}");
+            return Ok(2);
+        }
+    };
 
     if !yes && !io::stdin().is_terminal() {
         eprintln!("remote plugin install requires --yes when stdin is not interactive");
@@ -737,7 +758,7 @@ impl GithubPluginSource {
         }
         let parts = value.split('/').collect::<Vec<_>>();
         if parts.len() < 2 {
-            return Err("usage: herdr plugin install <owner>/<repo>[/subdir...]".into());
+            return Err(PLUGIN_INSTALL_USAGE.into());
         }
         let owner = parts[0];
         let repo = parts[1];
@@ -1738,6 +1759,68 @@ mod tests {
     }
 
     #[test]
+    fn plugin_install_args_accept_options_around_source() {
+        for (args, expected_ref, expected_yes) in [
+            (vec!["owner/repo"], None, false),
+            (vec!["--yes", "owner/repo"], None, true),
+            (vec!["-y", "owner/repo"], None, true),
+            (
+                vec!["owner/repo", "--ref", "main", "-y"],
+                Some("main"),
+                true,
+            ),
+            (
+                vec!["--ref", "main", "owner/repo", "--yes"],
+                Some("main"),
+                true,
+            ),
+            (
+                vec![
+                    "--yes",
+                    "--ref",
+                    "old",
+                    "owner/repo",
+                    "--ref",
+                    "main",
+                    "--yes",
+                ],
+                Some("main"),
+                true,
+            ),
+            // Preserve ref-value consumption: a value named --yes is not consent.
+            (vec!["owner/repo", "--ref", "--yes"], Some("--yes"), false),
+        ] {
+            let args = args.into_iter().map(String::from).collect::<Vec<_>>();
+            let parsed = parse_plugin_install_args(&args).unwrap();
+            assert_eq!(parsed.source.display(), "owner/repo", "{args:?}");
+            assert_eq!(parsed.requested_ref.as_deref(), expected_ref, "{args:?}");
+            assert_eq!(parsed.yes, expected_yes, "{args:?}");
+        }
+    }
+
+    #[test]
+    fn plugin_install_args_reject_invalid_syntax() {
+        for (args, expected_error) in [
+            (vec![], PLUGIN_INSTALL_USAGE),
+            (vec!["--yes"], PLUGIN_INSTALL_USAGE),
+            (vec!["owner/repo", "--ref"], "missing value for --ref"),
+            (vec!["--unknown", "owner/repo"], "unknown option: --unknown"),
+            (
+                vec!["owner/repo", "extra/repo"],
+                "unknown option: extra/repo",
+            ),
+            (vec!["owner"], PLUGIN_INSTALL_USAGE),
+        ] {
+            let args = args.into_iter().map(String::from).collect::<Vec<_>>();
+            assert_eq!(
+                parse_plugin_install_args(&args).unwrap_err(),
+                expected_error,
+                "{args:?}"
+            );
+        }
+    }
+
+    #[test]
     fn github_plugin_source_parses_root_repo() {
         let source = GithubPluginSource::parse("ogulcancelik/herdr-plugin-examples").unwrap();
         assert_eq!(source.owner, "ogulcancelik");
diff --git a/tests/cli/plugins.rs b/tests/cli/plugins.rs
index 6726c243..23f1b352 100644
--- a/tests/cli/plugins.rs
+++ b/tests/cli/plugins.rs
@@ -388,6 +388,36 @@ command = ["sh", "-c", "sleep 5"]
 }
 
 #[test]
+fn plugin_install_usage_errors_include_options_without_installing() {
+    let base = unique_test_dir();
+    let config_home = base.join("config");
+    let runtime_dir = base.join("runtime");
+    let state_home = base.join("state");
+    fs::create_dir_all(&runtime_dir).unwrap();
+
+    for args in [
+        vec!["plugin", "install"],
+        vec!["plugin", "install", "owner"],
+    ] {
+        let output = run_named_cli_with_env(
+            &config_home,
+            &runtime_dir,
+            &args,
+            &[("XDG_STATE_HOME", &state_home)],
+        );
+        assert_eq!(output.status.code(), Some(2), "{args:?}");
+        let stderr = String::from_utf8_lossy(&output.stderr);
+        assert!(stderr.contains("usage: herdr plugin install"), "{stderr}");
+        assert!(stderr.contains("[--ref REF] [--yes|-y]"), "{stderr}");
+        assert!(!config_home.join("herdr-dev/plugins").exists());
+        assert!(!config_home.join("herdr-dev/plugins.json").exists());
+        assert!(!state_home.exists());
+    }
+
+    cleanup_test_base(&base);
+}
+
+#[test]
 fn plugin_install_list_uninstall_offline_cli_smoke_test() {
     let base = unique_test_dir();
     let config_home = base.join("config");
@@ -433,6 +463,7 @@ command = ["sh", "-c", "echo bootstrap"]
     )
     .unwrap();
 
+    run_git(&source_repo, &["tag", "v0.43.0"]);
     let install = run_named_cli_with_env(
         &config_home,
         &runtime_dir,
@@ -441,11 +472,14 @@ command = ["sh", "-c", "echo bootstrap"]
             "plugins",
             "plugin",
             "install",
-            "ogulcancelik/herdr-plugin-examples/worktree-bootstrap",
             "--yes",
+            "--ref",
+            "v0.43.0",
+            "ogulcancelik/herdr-plugin-examples/worktree-bootstrap",
         ],
         &[
             ("GIT_CONFIG_GLOBAL", &git_config),
+            ("XDG_STATE_HOME", &base.join("state")),
             ("HERDR_SESSION", Path::new("leaked-session")),
         ],
     );
@@ -467,6 +501,7 @@ command = ["sh", "-c", "echo bootstrap"]
     assert_eq!(plugin["source"]["owner"], "ogulcancelik");
     assert_eq!(plugin["source"]["repo"], "herdr-plugin-examples");
     assert_eq!(plugin["source"]["subdir"], "worktree-bootstrap");
+    assert_eq!(plugin["source"]["requested_ref"], "v0.43.0");
     assert!(plugin["source"]["resolved_commit"].as_str().is_some());
     let managed_path = PathBuf::from(plugin["source"]["managed_path"].as_str().unwrap());
     assert!(managed_path.exists(), "managed checkout should exist");