diff --git a/src/app/parser.rs b/src/app/parser.rs index cb367cf6c66..33a968966c8 100644 --- a/src/app/parser.rs +++ b/src/app/parser.rs @@ -1071,12 +1071,15 @@ where } self.cache = Some(p.b.name); } - let _ = self.add_val_to_arg(p, &arg_os, matcher)?; matcher.inc_occurrence_of(p.b.name); let _ = self.groups_for_arg(p.b.name) .and_then(|vec| Some(matcher.inc_occurrences_of(&*vec))); + // Add value after increasing the occurrence count to ensure it is associated with + // the new occurence. + let _ = self.add_val_to_arg(p, &arg_os, matcher)?; + self.settings.set(AS::ValidArgFound); // Only increment the positional counter if it doesn't allow multiples if !p.b.settings.is_set(ArgSettings::Multiple) { @@ -1740,6 +1743,7 @@ where let needs_eq = opt.is_set(ArgSettings::RequireEquals); debug!("Parser::parse_opt; Checking for val..."); + let mut parsed_val = None; if let Some(fv) = val { has_eq = fv.starts_with(&[b'=']) || had_eq; let v = fv.trim_left_matches(b'='); @@ -1757,7 +1761,8 @@ where fv, fv.starts_with(&[b'=']) ); - self.add_val_to_arg(opt, v, matcher)?; + + parsed_val = Some(v); } else if needs_eq && !(empty_vals || min_vals_zero) { sdebugln!("None, but requires equals...Error"); return Err(Error::empty_value( @@ -1774,6 +1779,12 @@ where self.groups_for_arg(opt.b.name) .and_then(|vec| Some(matcher.inc_occurrences_of(&*vec))); + // Add value after increasing the occurrence count to ensure it is associated with the new + // occurence. + if let Some(v) = parsed_val { + self.add_val_to_arg(opt, v, matcher)?; + } + let needs_delim = opt.is_set(ArgSettings::RequireDelimiter); let mult = opt.is_set(ArgSettings::Multiple); if no_val && min_vals_zero && !has_eq && needs_eq { diff --git a/src/args/arg_matcher.rs b/src/args/arg_matcher.rs index e1d8067e69a..c6eadc24c42 100644 --- a/src/args/arg_matcher.rs +++ b/src/args/arg_matcher.rs @@ -10,6 +10,8 @@ use args::{ArgMatches, MatchedArg, SubCommand}; use args::AnyArg; use args::settings::ArgSettings; +use INTERNAL_ERROR_MSG; + #[doc(hidden)] #[allow(missing_debug_implementations)] pub struct ArgMatcher<'a>(pub ArgMatches<'a>); @@ -64,13 +66,24 @@ impl<'a> ArgMatcher<'a> { return; } if let Some(ma) = self.get_mut(aa.name()) { - if ma.vals.len() > 1 { - // swap_remove(0) would be O(1) but does not preserve order, which - // we need - ma.vals.remove(0); - ma.occurs = 1; - } else if !aa.takes_value() && ma.occurs > 1 { + if ma.occurs > 1 { + if aa.takes_value() { + // Only keep values for the last occurrence + let len = ma.vals.len(); + let occurrence_start = ma.occurrences.pop().expect(INTERNAL_ERROR_MSG); + + // 1.26 has `rotate_left`, but we can't use it. + // ma.vals.rotate_left(occurrence_start); + let keep = len - occurrence_start; + for i in 0..keep { + ma.vals.swap(i, occurrence_start + i); + } + + ma.vals.truncate(keep); + } + ma.occurs = 1; + ma.occurrences.clear(); } } } @@ -155,6 +168,11 @@ impl<'a> ArgMatcher<'a> { pub fn inc_occurrence_of(&mut self, arg: &'a str) { debugln!("ArgMatcher::inc_occurrence_of: arg={}", arg); if let Some(a) = self.get_mut(arg) { + if a.occurs > 0 { + // If not the first occurrence, we need to record the position in the vals vector + // at which this occurrence starts + a.occurrences.push(a.vals.len()); + } a.occurs += 1; return; } @@ -174,6 +192,7 @@ impl<'a> ArgMatcher<'a> { occurs: 0, indices: Vec::with_capacity(1), vals: Vec::with_capacity(1), + occurrences: Vec::new(), }); ma.vals.push(val.to_owned()); } @@ -183,6 +202,7 @@ impl<'a> ArgMatcher<'a> { occurs: 0, indices: Vec::with_capacity(1), vals: Vec::new(), + occurrences: Vec::new(), }); ma.indices.push(idx); } diff --git a/src/args/matched_arg.rs b/src/args/matched_arg.rs index eeda2611ca9..9c49f9eba7e 100644 --- a/src/args/matched_arg.rs +++ b/src/args/matched_arg.rs @@ -7,6 +7,10 @@ pub struct MatchedArg { #[doc(hidden)] pub occurs: u64, #[doc(hidden)] pub indices: Vec, #[doc(hidden)] pub vals: Vec, + // This contains the positions in `vals` at which each occurrence starts. The first occurrence + // is implicitely starting at position `0` so that `occurrences` is empty in the common case of + // a single occurrence. + #[doc(hidden)] pub occurrences: Vec, } impl Default for MatchedArg { @@ -15,6 +19,7 @@ impl Default for MatchedArg { occurs: 1, indices: Vec::new(), vals: Vec::new(), + occurrences: Vec::new(), } } } diff --git a/tests/app_settings.rs b/tests/app_settings.rs index 7f2c06ec0c3..e93fd8f9ef6 100644 --- a/tests/app_settings.rs +++ b/tests/app_settings.rs @@ -826,6 +826,64 @@ fn aaos_opts() { assert_eq!(m.value_of("opt"), Some("other")); } +#[test] +fn aaos_opts_two_values() { + // opts with two values + let res = App::new("posix") + .setting(AppSettings::AllArgsOverrideSelf) + .arg(Arg::from_usage("--opt [val1] [val2] 'some option'")) + .get_matches_from_safe(vec![ + "", "--opt", "some", "thing", "--opt", "other", "stuff", + ]); + assert!(res.is_ok()); + let m = res.unwrap(); + assert!(m.is_present("opt")); + assert_eq!(m.occurrences_of("opt"), 1); + assert_eq!( + m.values_of("opt").unwrap().collect::>(), + &["other", "stuff"] + ); +} + +#[test] +fn aaos_opts_two_values_delimiter() { + // opts with two values and a delimiter + let res = App::new("posix") + .setting(AppSettings::AllArgsOverrideSelf) + .arg(Arg::from_usage("--opt [val1] [val2] 'some option'").require_delimiter(true)) + .get_matches_from_safe(vec!["", "--opt=some,thing", "--opt=other,stuff"]); + assert!(res.is_ok()); + let m = res.unwrap(); + assert!(m.is_present("opt")); + assert_eq!(m.occurrences_of("opt"), 1); + assert_eq!( + m.values_of("opt").unwrap().collect::>(), + &["other", "stuff"] + ); +} + +#[test] +fn aaos_opts_min_value() { + // opts with min_value + let res = App::new("posix") + .setting(AppSettings::AllArgsOverrideSelf) + .arg( + Arg::from_usage("--opt [val] 'some option'") + .require_delimiter(true) + .min_values(0), + ) + .get_matches_from_safe(vec!["", "--opt", "--opt=val1,val2"]); + assert!(res.is_ok()); + let m = res.unwrap(); + assert!(m.is_present("opt")); + assert_eq!(m.occurrences_of("opt"), 1); + assert_eq!( + m.values_of("opt").unwrap().collect::>(), + &["val1", "val2"] + ); +} + + #[test] fn aaos_opts_w_other_overrides() { // opts with other overrides