Repository: clap-rs/clap
Pull request #2333 (2021-02-07 04:41:52)

Build help and version args at the beginning
This is a start to solve the mut_arg problem. This PR builds the help and version args at the beginning when `App` is constructed but still prefers user provided args.

A nice side effect is cleaning up of some bugs in generator code.



Patch:
diff --git a/clap_generate/src/generators/mod.rs b/clap_generate/src/generators/mod.rs
index adeb8ab9..6cd1f8db 100644
--- a/clap_generate/src/generators/mod.rs
+++ b/clap_generate/src/generators/mod.rs
@@ -4,7 +4,7 @@ mod shells;
 use std::io::Write;
 
 // Internal
-use clap::{App, AppSettings, Arg};
+use clap::{App, Arg};
 pub use shells::*;
 
 /// Generator trait which can be used to write generators
@@ -120,8 +120,7 @@ pub trait Generator {
     fn shorts_and_visible_aliases(p: &App) -> Vec<char> {
         debug!("shorts: name={}", p.get_name());
 
-        let mut shorts_and_visible_aliases: Vec<char> = p
-            .get_arguments()
+        p.get_arguments()
             .filter_map(|a| {
                 if a.get_index().is_none() {
                     if a.get_visible_short_aliases().is_some() && a.get_short().is_some() {
@@ -138,19 +137,7 @@ pub trait Generator {
                 }
             })
             .flatten()
-            .collect();
-
-        if !shorts_and_visible_aliases.iter().any(|&x| x == 'h') {
-            shorts_and_visible_aliases.push('h');
-        }
-
-        if !p.is_set(AppSettings::DisableVersionFlag)
-            && !shorts_and_visible_aliases.iter().any(|&x| x == 'V')
-        {
-            shorts_and_visible_aliases.push('V');
-        }
-
-        shorts_and_visible_aliases
+            .collect()
     }
 
     /// Gets all the long options and flags of a [`clap::App`](../clap/struct.App.html).
@@ -158,8 +145,7 @@ pub trait Generator {
     fn longs(p: &App) -> Vec<String> {
         debug!("longs: name={}", p.get_name());
 
-        let mut longs: Vec<String> = p
-            .get_arguments()
+        p.get_arguments()
             .filter_map(|a| {
                 if a.get_index().is_none() && a.get_long().is_some() {
                     Some(a.get_long().unwrap().to_string())
@@ -167,47 +153,14 @@ pub trait Generator {
                     None
                 }
             })
-            .collect();
-
-        if !longs.iter().any(|x| *x == "help") {
-            longs.push(String::from("help"));
-        }
-
-        if !p.is_set(AppSettings::DisableVersionFlag) && !longs.iter().any(|x| *x == "version") {
-            longs.push(String::from("version"));
-        }
-
-        longs
+            .collect()
     }
 
     /// Gets all the flags of a [`clap::App`](../clap/struct.App.html).
     /// Includes `help` and `version` depending on the [`clap::AppSettings`](../clap/enum.AppSettings.html).
     fn flags<'help>(p: &App<'help>) -> Vec<Arg<'help>> {
         debug!("flags: name={}", p.get_name());
-
-        let mut flags: Vec<_> = p.get_flags().cloned().collect();
-
-        if !flags.iter().any(|x| x.get_name() == "help") {
-            flags.push(
-                Arg::new("help")
-                    .short('h')
-                    .long("help")
-                    .about("Prints help information"),
-            );
-        }
-
-        if !p.is_set(AppSettings::DisableVersionFlag)
-            && !flags.iter().any(|x| x.get_name() == "version")
-        {
-            flags.push(
-                Arg::new("version")
-                    .short('V')
-                    .long("version")
-                    .about("Prints version information"),
-            );
-        }
-
-        flags
+        p.get_flags().cloned().collect()
     }
 }
 
diff --git a/clap_generate/src/lib.rs b/clap_generate/src/lib.rs
index 426c3a58..ec3aee73 100644
--- a/clap_generate/src/lib.rs
+++ b/clap_generate/src/lib.rs
@@ -181,6 +181,7 @@ where
 {
     app.set_bin_name(bin_name);
 
+    // TODO: All the subcommands need to be built instead of just the top one
     app._build();
     app._build_bin_names();
 
diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs
index 0ce9f2fd..f2bbffc9 100644
--- a/src/build/app/mod.rs
+++ b/src/build/app/mod.rs
@@ -23,7 +23,7 @@ use yaml_rust::Yaml;
 
 // Internal
 use crate::{
-    build::{app::settings::AppFlags, Arg, ArgGroup, ArgSettings},
+    build::{app::settings::AppFlags, arg::ArgProvider, Arg, ArgGroup, ArgSettings},
     mkeymap::MKeyMap,
     output::{fmt::Colorizer, Help, HelpWriter, Usage},
     parse::{ArgMatcher, ArgMatches, Input, Parser},
@@ -356,12 +356,27 @@ impl<'help> App<'help> {
     /// ```
     pub fn new<S: Into<String>>(name: S) -> Self {
         let name = name.into();
+
         App {
             id: Id::from(&*name),
             name,
             disp_ord: 999,
             ..Default::default()
         }
+        .arg(
+            Arg::new("help")
+                .long("help")
+                .about("Prints help information")
+                .global(true)
+                .generated(),
+        )
+        .arg(
+            Arg::new("version")
+                .long("version")
+                .about("Prints version information")
+                .global(true)
+                .generated(),
+        )
     }
 
     /// Sets a string of author(s) that will be displayed to the user when they
@@ -2288,9 +2303,9 @@ impl<'help> App<'help> {
             self.settings = self.settings | self.g_settings;
 
             self._propagate();
-
+            self._check_help_and_version();
+            self._propagate_global_args();
             self._derive_display_order();
-            self._create_help_and_version();
 
             let mut pos_counter = 1;
             for a in self.args.args_mut() {
@@ -2368,7 +2383,27 @@ impl<'help> App<'help> {
         two_elements_of(self.groups.iter().filter(|a| condition(a)))
     }
 
-    /// Propagate one and only one level.
+    /// Propagate global args
+    pub(crate) fn _propagate_global_args(&mut self) {
+        debug!("App::_propagate_global_args:{}", self.name);
+
+        for sc in &mut self.subcommands {
+            for a in self.args.args().filter(|a| a.global) {
+                let is_generated = a.provider == ArgProvider::Generated;
+
+                // Remove generated help and version args in the subcommand
+                if is_generated {
+                    sc.args.remove_by_name(&a.id);
+                }
+
+                if is_generated || sc.find(&a.id).is_none() {
+                    sc.args.push(a.clone());
+                }
+            }
+        }
+    }
+
+    /// Propagate settings
     pub(crate) fn _propagate(&mut self) {
         macro_rules! propagate_subcmd {
             ($_self:expr, $sc:expr) => {{
@@ -2398,14 +2433,6 @@ impl<'help> App<'help> {
                         $sc.version_about = $_self.version_about.clone();
                     }
                 }
-                {
-                    // FIXME: This doesn't belong here at all.
-                    for a in $_self.args.args().filter(|a| a.global) {
-                        if $sc.find(&a.id).is_none() {
-                            $sc.args.push(a.clone());
-                        }
-                    }
-                }
             }};
         }
 
@@ -2416,62 +2443,95 @@ impl<'help> App<'help> {
         }
     }
 
-    pub(crate) fn _create_help_and_version(&mut self) {
-        debug!("App::_create_help_and_version");
+    #[allow(clippy::blocks_in_if_conditions)]
+    pub(crate) fn _check_help_and_version(&mut self) {
+        debug!("App::_check_help_and_version");
 
-        if !(self.is_set(AppSettings::DisableHelpFlag)
-            || self
-                .args
-                .args()
-                .any(|x| x.long == Some("help") || x.id == Id::help_hash())
+        if self.is_set(AppSettings::DisableHelpFlag)
+            || self.args.args().any(|x| {
+                x.provider == ArgProvider::User
+                    && (x.long == Some("help") || x.id == Id::help_hash())
+            })
             || self
                 .subcommands
                 .iter()
-                .any(|sc| sc.long_flag == Some("help")))
+                .any(|sc| sc.long_flag == Some("help"))
         {
-            debug!("App::_create_help_and_version: Building --help");
-            let mut help = Arg::new("help")
-                .long("help")
-                .about(self.help_about.unwrap_or("Prints help information"));
+            debug!("App::_check_help_and_version: Removing generated help");
 
-            if !(self.args.args().any(|x| x.short == Some('h'))
+            let generated_help_pos = self
+                .args
+                .args()
+                .position(|x| x.id == Id::help_hash() && x.provider == ArgProvider::Generated);
+
+            if let Some(index) = generated_help_pos {
+                self.args.remove(index);
+            }
+        } else {
+            let other_arg_has_short = self.args.args().any(|x| x.short == Some('h'));
+            let help = self
+                .args
+                .args_mut()
+                .find(|x| x.id == Id::help_hash())
+                .expect(INTERNAL_ERROR_MSG);
+
+            if !(help.short.is_some()
+                || other_arg_has_short
                 || self.subcommands.iter().any(|sc| sc.short_flag == Some('h')))
             {
-                help = help.short('h');
+                help.short = Some('h');
             }
 
-            self.args.push(help);
+            if self.help_about.is_some() {
+                help.about = self.help_about;
+            }
         }
 
-        if !(self.is_set(AppSettings::DisableVersionFlag)
-            || self
-                .args
-                .args()
-                .any(|x| x.long == Some("version") || x.id == Id::version_hash())
+        if self.is_set(AppSettings::DisableVersionFlag)
+            || self.args.args().any(|x| {
+                x.provider == ArgProvider::User
+                    && (x.long == Some("version") || x.id == Id::version_hash())
+            })
             || self
                 .subcommands
                 .iter()
-                .any(|sc| sc.long_flag == Some("version")))
+                .any(|sc| sc.long_flag == Some("version"))
         {
-            debug!("App::_create_help_and_version: Building --version");
-            let mut version = Arg::new("version")
-                .long("version")
-                .about(self.version_about.unwrap_or("Prints version information"));
+            debug!("App::_check_help_and_version: Removing generated version");
+
+            let generated_version_pos = self
+                .args
+                .args()
+                .position(|x| x.id == Id::version_hash() && x.provider == ArgProvider::Generated);
 
-            if !(self.args.args().any(|x| x.short == Some('V'))
+            if let Some(index) = generated_version_pos {
+                self.args.remove(index);
+            }
+        } else {
+            let other_arg_has_short = self.args.args().any(|x| x.short == Some('V'));
+            let version = self
+                .args
+                .args_mut()
+                .find(|x| x.id == Id::version_hash())
+                .expect(INTERNAL_ERROR_MSG);
+
+            if !(version.short.is_some()
+                || other_arg_has_short
                 || self.subcommands.iter().any(|sc| sc.short_flag == Some('V')))
             {
-                version = version.short('V');
+                version.short = Some('V');
             }
 
-            self.args.push(version);
+            if self.version_about.is_some() {
+                version.about = self.version_about;
+            }
         }
 
         if !self.is_set(AppSettings::DisableHelpSubcommand)
             && self.has_subcommands()
             && !self.subcommands.iter().any(|s| s.id == Id::help_hash())
         {
-            debug!("App::_create_help_and_version: Building help");
+            debug!("App::_check_help_and_version: Building help subcommand");
             self.subcommands.push(
                 App::new("help").about(
                     self.help_about
diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs
index 63303f58..7dd04c1b 100644
--- a/src/build/arg/mod.rs
+++ b/src/build/arg/mod.rs
@@ -42,6 +42,18 @@ use yaml_rust::Yaml;
 type Validator<'a> = dyn FnMut(&str) -> Result<(), Box<dyn Error + Send + Sync>> + Send + 'a;
 type ValidatorOs<'a> = dyn FnMut(&OsStr) -> Result<(), Box<dyn Error + Send + Sync>> + Send + 'a;
 
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub(crate) enum ArgProvider {
+    Generated,
+    User,
+}
+
+impl Default for ArgProvider {
+    fn default() -> Self {
+        ArgProvider::User
+    }
+}
+
 /// The abstract representation of a command line argument. Used to set all the options and
 /// relationships that define a valid argument for the program.
 ///
@@ -68,6 +80,7 @@ type ValidatorOs<'a> = dyn FnMut(&OsStr) -> Result<(), Box<dyn Error + Send + Sy
 #[derive(Default, Clone)]
 pub struct Arg<'help> {
     pub(crate) id: Id,
+    pub(crate) provider: ArgProvider,
     pub(crate) name: &'help str,
     pub(crate) about: Option<&'help str>,
     pub(crate) long_about: Option<&'help str>,
@@ -208,6 +221,11 @@ impl<'help> Arg<'help> {
         }
     }
 
+    pub(crate) fn generated(mut self) -> Self {
+        self.provider = ArgProvider::Generated;
+        self
+    }
+
     /// Sets the short version of the argument without the preceding `-`.
     ///
     /// By default `clap` automatically assigns `V` and `h` to the auto-generated `version` and
@@ -4671,6 +4689,7 @@ impl<'help> fmt::Debug for Arg<'help> {
     fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> {
         f.debug_struct("Arg")
             .field("id", &self.id)
+            .field("provider", &self.provider)
             .field("name", &self.name)
             .field("about", &self.about)
             .field("long_about", &self.long_about)
diff --git a/src/mkeymap.rs b/src/mkeymap.rs
index d22a4aba..c1ea6a09 100644
--- a/src/mkeymap.rs
+++ b/src/mkeymap.rs
@@ -133,7 +133,12 @@ impl<'help> MKeyMap<'help> {
             .iter()
             .position(|arg| &arg.id == name)
             // since it's a cold function, using this wouldn't hurt much
-            .map(|i| self.args.swap_remove(i))
+            .map(|i| self.args.remove(i))
+    }
+
+    /// Remove an arg based on index
+    pub(crate) fn remove(&mut self, index: usize) -> Arg<'help> {
+        self.args.remove(index)
     }
 }
 


Test patch:
diff --git a/clap_generate/tests/completions/fish.rs b/clap_generate/tests/completions/fish.rs
index e398de0b..6e2cc459 100644
--- a/clap_generate/tests/completions/fish.rs
+++ b/clap_generate/tests/completions/fish.rs
@@ -112,12 +112,12 @@ fn build_app_special_help() -> App<'static> {
         )
 }
 
-static FISH_SPECIAL_HELP: &str = r#"complete -c my_app -n "__fish_use_subcommand" -l single-quotes -d 'Can be \'always\', \'auto\', or \'never\''
+static FISH_SPECIAL_HELP: &str = r#"complete -c my_app -n "__fish_use_subcommand" -s h -l help -d 'Prints help information'
+complete -c my_app -n "__fish_use_subcommand" -s V -l version -d 'Prints version information'
+complete -c my_app -n "__fish_use_subcommand" -l single-quotes -d 'Can be \'always\', \'auto\', or \'never\''
 complete -c my_app -n "__fish_use_subcommand" -l double-quotes -d 'Can be "always", "auto", or "never"'
 complete -c my_app -n "__fish_use_subcommand" -l backticks -d 'For more information see `echo test`'
 complete -c my_app -n "__fish_use_subcommand" -l backslash -d 'Avoid \'\\n\''
 complete -c my_app -n "__fish_use_subcommand" -l brackets -d 'List packages [filter]'
 complete -c my_app -n "__fish_use_subcommand" -l expansions -d 'Execute the shell command with $SHELL'
-complete -c my_app -n "__fish_use_subcommand" -s h -l help -d 'Prints help information'
-complete -c my_app -n "__fish_use_subcommand" -s V -l version -d 'Prints version information'
 "#;
diff --git a/clap_generate/tests/completions/zsh.rs b/clap_generate/tests/completions/zsh.rs
index 4e4ee953..30392c49 100644
--- a/clap_generate/tests/completions/zsh.rs
+++ b/clap_generate/tests/completions/zsh.rs
@@ -295,16 +295,16 @@ _my_app() {
 
     local context curcontext="$curcontext" state line
     _arguments "${_arguments_options[@]}" \
+'-h[Prints help information]' \
+'--help[Prints help information]' \
+'-V[Prints version information]' \
+'--version[Prints version information]' \
 '--single-quotes[Can be '\''always'\'', '\''auto'\'', or '\''never'\'']' \
 '--double-quotes[Can be "always", "auto", or "never"]' \
 '--backticks[For more information see `echo test`]' \
 '--backslash[Avoid '\''\\n'\'']' \
 '--brackets[List packages \[filter\]]' \
 '--expansions[Execute the shell command with $SHELL]' \
-'-h[Prints help information]' \
-'--help[Prints help information]' \
-'-V[Prints version information]' \
-'--version[Prints version information]' \
 && ret=0
     
 }
@@ -374,9 +374,7 @@ case $state in
         case $line[1] in
             (third)
 _arguments "${_arguments_options[@]}" \
-'-h[Prints help information]' \
 '--help[Prints help information]' \
-'-V[Prints version information]' \
 '--version[Prints version information]' \
 && ret=0
 ;;
diff --git a/tests/app_settings.rs b/tests/app_settings.rs
index b971ad19..b730546c 100644
--- a/tests/app_settings.rs
+++ b/tests/app_settings.rs
@@ -70,7 +70,7 @@ FLAGS:
 OPTIONS:
     -o, --opt <opt>    some option";
 
-static ARG_REQUIRED_ELSE_HELP: &str = "test 
+static ARG_REQUIRED_ELSE_HELP: &str = "test 1.0
 
 USAGE:
     test [FLAGS]
@@ -80,7 +80,7 @@ FLAGS:
     -i, --info       Provides more info
     -V, --version    Prints version information";
 
-static SUBCOMMAND_REQUIRED_ELSE_HELP: &str = "test 
+static SUBCOMMAND_REQUIRED_ELSE_HELP: &str = "test 1.0
 
 USAGE:
     test <SUBCOMMAND>
@@ -157,6 +157,7 @@ fn arg_required_else_help_over_reqs() {
 fn arg_required_else_help_error_message() {
     let app = App::new("test")
         .setting(AppSettings::ArgRequiredElseHelp)
+        .version("1.0")
         .arg(
             Arg::new("info")
                 .about("Provides more info")
@@ -189,6 +190,7 @@ fn subcommand_required_else_help() {
 fn subcommand_required_else_help_error_message() {
     let app = App::new("test")
         .setting(AppSettings::SubcommandRequiredElseHelp)
+        .version("1.0")
         .subcommand(App::new("info").arg(Arg::new("filename")));
     assert!(utils::compare_output(
         app,
diff --git a/tests/derive_order.rs b/tests/derive_order.rs
index 96e7b90b..e797338b 100644
--- a/tests/derive_order.rs
+++ b/tests/derive_order.rs
@@ -25,10 +25,10 @@ USAGE:
     test [FLAGS] [OPTIONS]
 
 FLAGS:
-        --flag_b     first flag
-        --flag_a     second flag
     -h, --help       Prints help information
     -V, --version    Prints version information
+        --flag_b     first flag
+        --flag_a     second flag
 
 OPTIONS:
         --option_b <option_b>    first option
@@ -53,12 +53,12 @@ USAGE:
     test [OPTIONS]
 
 OPTIONS:
+    -h, --help                   Prints help information
+    -V, --version                Prints version information
         --flag_b                 first flag
         --option_b <option_b>    first option
         --flag_a                 second flag
-        --option_a <option_a>    second option
-    -h, --help                   Prints help information
-    -V, --version                Prints version information";
+        --option_a <option_a>    second option";
 
 static DERIVE_ORDER_SC_PROP: &str = "test-sub 1.2
 
@@ -114,6 +114,28 @@ OPTIONS:
     -h, --help                   Prints help information
     -V, --version                Prints version information";
 
+static PREFER_USER_HELP_DERIVE_ORDER: &str = "test 1.2
+
+USAGE:
+    test [FLAGS]
+
+FLAGS:
+    -V, --version    Prints version information
+    -h, --help       Prints help message
+        --flag_b     first flag
+        --flag_a     second flag";
+
+static PREFER_USER_HELP_SUBCMD_DERIVE_ORDER: &str = "test-sub 1.2
+
+USAGE:
+    test sub [FLAGS]
+
+FLAGS:
+    -h, --help       Prints help message
+        --flag_b     first flag
+        --flag_a     second flag
+    -V, --version    Prints version information";
+
 #[test]
 fn no_derive_order() {
     let app = App::new("test").version("1.2").args(&[
@@ -329,3 +351,48 @@ fn unified_help_and_derive_order_subcommand_propagate_with_explicit_display_orde
         false
     ));
 }
+
+#[test]
+fn prefer_user_help_with_derive_order() {
+    let app = App::new("test")
+        .setting(AppSettings::DeriveDisplayOrder)
+        .version("1.2")
+        .args(&[
+            Arg::new("help")
+                .long("help")
+                .short('h')
+                .about("Prints help message"),
+            Arg::new("flag_b").long("flag_b").about("first flag"),
+            Arg::new("flag_a").long("flag_a").about("second flag"),
+        ]);
+
+    assert!(utils::compare_output(
+        app,
+        "test --help",
+        PREFER_USER_HELP_DERIVE_ORDER,
+        false
+    ));
+}
+
+#[test]
+fn prefer_user_help_in_subcommand_with_derive_order() {
+    let app = App::new("test")
+        .global_setting(AppSettings::DeriveDisplayOrder)
+        .subcommand(
+            App::new("sub").version("1.2").args(&[
+                Arg::new("help")
+                    .long("help")
+                    .short('h')
+                    .about("Prints help message"),
+                Arg::new("flag_b").long("flag_b").about("first flag"),
+                Arg::new("flag_a").long("flag_a").about("second flag"),
+            ]),
+        );
+
+    assert!(utils::compare_output(
+        app,
+        "test sub --help",
+        PREFER_USER_HELP_SUBCMD_DERIVE_ORDER,
+        false
+    ));
+}
diff --git a/tests/global_args.rs b/tests/global_args.rs
index a0efb836..def291b3 100644
--- a/tests/global_args.rs
+++ b/tests/global_args.rs
@@ -2,8 +2,9 @@ mod utils;
 
 use clap::{App, Arg};
 
-fn get_app() -> App<'static> {
-    App::new("myprog")
+#[test]
+fn issue_1076() {
+    let mut app = App::new("myprog")
         .arg(
             Arg::new("GLOBAL_ARG")
                 .long("global-arg")
@@ -19,12 +20,7 @@ fn get_app() -> App<'static> {
                 .multiple(true)
                 .global(true),
         )
-        .subcommand(App::new("outer").subcommand(App::new("inner")))
-}
-
-#[test]
-fn issue_1076() {
-    let mut app = get_app();
+        .subcommand(App::new("outer").subcommand(App::new("inner")));
     let _ = app.try_get_matches_from_mut(vec!["myprog"]);
     let _ = app.try_get_matches_from_mut(vec!["myprog"]);
     let _ = app.try_get_matches_from_mut(vec!["myprog"]);
@@ -51,7 +47,7 @@ fn propagate_global_arg_in_subcommand_to_subsubcommand_1385() {
 }
 
 #[test]
-fn propagate_global_arg_in_subcommand_to_subsubcommand_2053() {
+fn propagate_global_arg_to_subcommand_in_subsubcommand_2053() {
     let m = App::new("opts")
         .arg(Arg::from("--global-flag").global(true))
         .arg(
diff --git a/tests/help.rs b/tests/help.rs
index b71b4b5d..7a6905b2 100644
--- a/tests/help.rs
+++ b/tests/help.rs
@@ -1556,29 +1556,30 @@ fn escaped_whitespace_values() {
 fn issue_1112_setup() -> App<'static> {
     App::new("test")
         .version("1.3")
-        .global_setting(AppSettings::NoAutoHelp)
-        .arg(Arg::from("-h, --help 'some help'"))
-        .subcommand(App::new("foo").arg(Arg::from("-h, --help 'some help'")))
+        .arg(Arg::new("help1").long("help").short('h').about("some help"))
+        .subcommand(
+            App::new("foo").arg(Arg::new("help1").long("help").short('h').about("some help")),
+        )
 }
 
 #[test]
-fn issue_1112_override_help_long() {
+fn prefer_user_help_long_1112() {
     let m = issue_1112_setup().try_get_matches_from(vec!["test", "--help"]);
 
     assert!(m.is_ok());
-    assert!(m.unwrap().is_present("help"));
+    assert!(m.unwrap().is_present("help1"));
 }
 
 #[test]
-fn issue_1112_override_help_short() {
+fn prefer_user_help_short_1112() {
     let m = issue_1112_setup().try_get_matches_from(vec!["test", "-h"]);
 
     assert!(m.is_ok());
-    assert!(m.unwrap().is_present("help"));
+    assert!(m.unwrap().is_present("help1"));
 }
 
 #[test]
-fn issue_1112_override_help_subcmd_long() {
+fn prefer_user_subcmd_help_long_1112() {
     let m = issue_1112_setup().try_get_matches_from(vec!["test", "foo", "--help"]);
 
     assert!(m.is_ok());
@@ -1586,11 +1587,11 @@ fn issue_1112_override_help_subcmd_long() {
         .unwrap()
         .subcommand_matches("foo")
         .unwrap()
-        .is_present("help"));
+        .is_present("help1"));
 }
 
 #[test]
-fn issue_1112_override_help_subcmd_short() {
+fn prefer_user_subcmd_help_short_1112() {
     let m = issue_1112_setup().try_get_matches_from(vec!["test", "foo", "-h"]);
 
     assert!(m.is_ok());
@@ -1598,7 +1599,7 @@ fn issue_1112_override_help_subcmd_short() {
         .unwrap()
         .subcommand_matches("foo")
         .unwrap()
-        .is_present("help"));
+        .is_present("help1"));
 }
 
 #[test]
diff --git a/tests/version.rs b/tests/version.rs
index cf302d5d..944cc29d 100644
--- a/tests/version.rs
+++ b/tests/version.rs
@@ -2,7 +2,7 @@ mod utils;
 
 use std::str;
 
-use clap::{App, AppSettings, ErrorKind};
+use clap::{App, AppSettings, Arg, ErrorKind};
 
 static VERSION: &str = "clap-test v1.4.8\n";
 
@@ -43,6 +43,65 @@ fn complex_version_output() {
     assert_eq!(a.render_version(), VERSION);
 }
 
+fn prefer_user_app() -> App<'static> {
+    App::new("test")
+        .version("1.3")
+        .arg(
+            Arg::new("version1")
+                .long("version")
+                .short('V')
+                .about("some version"),
+        )
+        .subcommand(
+            App::new("foo").arg(
+                Arg::new("version1")
+                    .long("version")
+                    .short('V')
+                    .about("some version"),
+            ),
+        )
+}
+
+#[test]
+fn prefer_user_version_long() {
+    let m = prefer_user_app().try_get_matches_from(vec!["test", "--version"]);
+
+    assert!(m.is_ok());
+    assert!(m.unwrap().is_present("version1"));
+}
+
+#[test]
+fn prefer_user_version_short() {
+    let m = prefer_user_app().try_get_matches_from(vec!["test", "-V"]);
+
+    assert!(m.is_ok());
+    assert!(m.unwrap().is_present("version1"));
+}
+
+#[test]
+fn prefer_user_subcmd_version_long() {
+    let m = prefer_user_app().try_get_matches_from(vec!["test", "foo", "--version"]);
+
+    assert!(m.is_ok());
+    assert!(m
+        .unwrap()
+        .subcommand_matches("foo")
+        .unwrap()
+        .is_present("version1"));
+}
+
+#[test]
+fn prefer_user_subcmd_version_short() {
+    let m = prefer_user_app().try_get_matches_from(vec!["test", "foo", "-V"]);
+
+    assert!(m.is_ok());
+    assert!(m
+        .unwrap()
+        .subcommand_matches("foo")
+        .unwrap()
+        .is_present("version1"));
+}
+
 #[test]
 fn override_ver() {
     let m = App::new("test")