Repository: clap-rs/clap
Pull request #1412 (2019-02-14 11:52:00)

Fix interaction between AllowExternalSubcommands and SubcommandRequired
I am classifying this as a bugfix even though it is technically breaking - but I think the old behaviour was not sensibleand it seems unlikely anyone was relying on it. 

Basically - setting both `AppSettings::SubcommandRequired` (or `SubcommandRequiredElseHelp`) and `AppSettings::AllowExternalSubcommands` has unintuitive behaviour.

Before this fix, with both `SubcommandRequired` and `AllowExternalSubcommands` set, if any external subcommand is provided, clap prints usage and exits - but really clap should treat an external subcommand as normal and allow the calling code to handle it.


Patch:
diff --git a/src/app/parser.rs b/src/app/parser.rs
index cb367cf6..9e1bbba6 100644
--- a/src/app/parser.rs
+++ b/src/app/parser.rs
@@ -871,6 +871,7 @@ where
         let mut subcmd_name: Option<String> = None;
         let mut needs_val_of: ParseResult<'a> = ParseResult::NotFound;
         let mut pos_counter = 1;
+        let mut sc_is_external = false;
         while let Some(arg) = it.next() {
             let arg_os = arg.into();
             debugln!(
@@ -1115,6 +1116,7 @@ where
                     name: sc_name,
                     matches: sc_m.into(),
                 });
+                sc_is_external = true;
             } else if !((self.is_set(AS::AllowLeadingHyphen)
                 || self.is_set(AS::AllowNegativeNumbers))
                 && arg_os.starts_with(b"-"))
@@ -1154,32 +1156,34 @@ where
             }
         }
 
-        if let Some(ref pos_sc_name) = subcmd_name {
-            let sc_name = {
-                find_subcmd!(self, pos_sc_name)
-                    .expect(INTERNAL_ERROR_MSG)
-                    .p
-                    .meta
-                    .name
-                    .clone()
-            };
-            self.parse_subcommand(&*sc_name, matcher, it)?;
-        } else if self.is_set(AS::SubcommandRequired) {
-            let bn = self.meta.bin_name.as_ref().unwrap_or(&self.meta.name);
-            return Err(Error::missing_subcommand(
-                bn,
-                &usage::create_error_usage(self, matcher, None),
-                self.color(),
-            ));
-        } else if self.is_set(AS::SubcommandRequiredElseHelp) {
-            debugln!("Parser::get_matches_with: SubcommandRequiredElseHelp=true");
-            let mut out = vec![];
-            self.write_help_err(&mut out)?;
-            return Err(Error {
-                message: String::from_utf8_lossy(&*out).into_owned(),
-                kind: ErrorKind::MissingArgumentOrSubcommand,
-                info: None,
-            });
+        if !sc_is_external {
+            if let Some(ref pos_sc_name) = subcmd_name {
+                let sc_name = {
+                    find_subcmd!(self, pos_sc_name)
+                        .expect(INTERNAL_ERROR_MSG)
+                        .p
+                        .meta
+                        .name
+                        .clone()
+                };
+                self.parse_subcommand(&*sc_name, matcher, it)?;
+            } else if self.is_set(AS::SubcommandRequired) {
+                let bn = self.meta.bin_name.as_ref().unwrap_or(&self.meta.name);
+                return Err(Error::missing_subcommand(
+                    bn,
+                    &usage::create_error_usage(self, matcher, None),
+                    self.color(),
+                ));
+            } else if self.is_set(AS::SubcommandRequiredElseHelp) {
+                debugln!("Parser::get_matches_with: SubcommandRequiredElseHelp=true");
+                let mut out = vec![];
+                self.write_help_err(&mut out)?;
+                return Err(Error {
+                    message: String::from_utf8_lossy(&*out).into_owned(),
+                    kind: ErrorKind::MissingArgumentOrSubcommand,
+                    info: None,
+                });
+            }
         }
 
         // In case the last arg was new, we  need to process it's overrides


Test patch:
diff --git a/tests/app_settings.rs b/tests/app_settings.rs
index 7f2c06ec..3d72a3b7 100644
--- a/tests/app_settings.rs
+++ b/tests/app_settings.rs
@@ -768,6 +768,25 @@ fn issue_1093_allow_ext_sc() {
     assert!(test::compare_output(app, "clap-test --help", ALLOW_EXT_SC, false));
 }
 
+
+#[test]
+fn allow_ext_sc_when_sc_required() {
+    let res = App::new("clap-test")
+        .version("v1.4.8")
+        .setting(AppSettings::AllowExternalSubcommands)
+        .setting(AppSettings::SubcommandRequiredElseHelp)
+        .get_matches_from_safe(vec!["clap-test", "external-cmd", "foo"]);
+    assert!(res.is_ok());
+    let m = res.unwrap();
+    match m.subcommand() {
+        (name, Some(args)) => {
+            assert_eq!(name, "external-cmd");
+            assert_eq!(args.values_of_lossy(""), Some(vec!["foo".to_string()]));
+        }
+        _ => assert!(false),
+    }
+}
+
 #[test]
 fn external_subcommand_looks_like_built_in() {
     let res = App::new("cargo")