Remove all values from previous occurrences on self-override

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 {

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);
     }

src/args/matched_arg.rs

@@ -7,6 +7,10 @@ pub struct MatchedArg {
     #[doc(hidden)] pub occurs: u64,
     #[doc(hidden)] pub indices: Vec<usize>,
     #[doc(hidden)] pub vals: Vec<OsString>,
+    // 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<usize>,
 }
 
 impl Default for MatchedArg {
@@ -15,6 +19,7 @@ impl Default for MatchedArg {
             occurs: 1,
             indices: Vec::new(),
             vals: Vec::new(),
+            occurrences: Vec::new(),
         }
     }
 }

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::<Vec<_>>(),
+        &["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::<Vec<_>>(),
+        &["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::<Vec<_>>(),
+        &["val1", "val2"]
+    );
+}
+
+
 #[test]
 fn aaos_opts_w_other_overrides() {
     // opts with other overrides