Revert #81, #82, #83

alexcrichton · alexcrichton · commit 58436e30e1e1 · 2019-08-19T08:02:40.000-07:00
This commit reverts three recent PRs to the `getopts` crate. One of the main consumers of `getopts` is `rustc`, which isn't allowed to have breaking changes. In #82, however, a breaking change was landed. It looks like #83 builds on this change, and while #81 seems unrelated the diffs were somewhat tangled.
diff --git a/src/lib.rs b/src/lib.rs
@@ -364,7 +364,7 @@ impl Options {
                     .ok_or_else(|| Fail::UnrecognizedOption(format!("{:?}", i.as_ref())))
                     .map(|s| s.to_owned())
             }).collect::<::std::result::Result<Vec<_>, _>>()?;
-        let mut args = args.into_iter().peekable();
+        let mut args = args.into_iter();
         let mut arg_pos = 0;
         while let Some(cur) = args.next() {
             if !is_arg(&cur) {
@@ -380,9 +380,8 @@ impl Options {
                 free.extend(args);
                 break;
             } else {
-                let mut names;
+                let mut name = None;
                 let mut i_arg = None;
-                let mut was_long = true;
                 if cur.as_bytes()[1] == b'-' || self.long_only {
                     let tail = if cur.as_bytes()[1] == b'-' {
                         &cur[2..]
@@ -391,81 +390,71 @@ impl Options {
                         &cur[1..]
                     };
                     let mut parts = tail.splitn(2, '=');
-                    names = vec![Name::from_str(parts.next().unwrap())];
+                    name = Some(Name::from_str(parts.next().unwrap()));
                     if let Some(rest) = parts.next() {
                         i_arg = Some(rest.to_string());
                     }
                 } else {
-                    was_long = false;
-                    names = Vec::new();
                     for (j, ch) in cur.char_indices().skip(1) {
                         let opt = Short(ch);
 
-                        /* In a series of potential options (eg. -aheJ), if we
-                           see one which takes an argument, we assume all
-                           subsequent characters make up the argument. This
-                           allows options such as -L/usr/local/lib/foo to be
-                           interpreted correctly
-                        */
-
                         let opt_id = match find_opt(&opts, &opt) {
                             Some(id) => id,
                             None => return Err(UnrecognizedOption(opt.to_string())),
                         };
 
-                        names.push(opt);
-
+                        // In a series of potential options (eg. -aheJ), if we
+                        // see one which takes an argument, we assume all
+                        // subsequent characters make up the argument. This
+                        // allows options such as -L/usr/local/lib/foo to be
+                        // interpreted correctly
                         let arg_follows = match opts[opt_id].hasarg {
                             Yes | Maybe => true,
                             No => false,
                         };
 
                         if arg_follows {
+                            name = Some(opt);
                             let next = j + ch.len_utf8();
                             if next < cur.len() {
                                 i_arg = Some(cur[next..].to_string());
                                 break;
                             }
+                        } else {
+                            vals[opt_id].push((arg_pos, Given));
                         }
                     }
                 }
-                let mut name_pos = 0;
-                for nm in names.iter() {
-                    name_pos += 1;
-                    let optid = match find_opt(&opts, &nm) {
+                if let Some(nm) = name {
+                    let opt_id = match find_opt(&opts, &nm) {
                         Some(id) => id,
                         None => return Err(UnrecognizedOption(nm.to_string())),
                     };
-                    match opts[optid].hasarg {
+                    match opts[opt_id].hasarg {
                         No => {
-                            if name_pos == names.len() && i_arg.is_some() {
+                            if i_arg.is_some() {
                                 return Err(UnexpectedArgument(nm.to_string()));
                             }
-                            vals[optid].push((arg_pos, Given));
+                            vals[opt_id].push((arg_pos, Given));
                         }
                         Maybe => {
-                            // Note that here we do not handle `--arg value`.
-                            // This matches GNU getopt behavior; but also
-                            // makes sense, because if this were accepted,
-                            // then users could only write a "Maybe" long
-                            // option at the end of the arguments when
-                            // FloatingFrees is in use.
+                            // Note that here we do not handle `--arg value` or
+                            // `-a value`. This matches GNU getopt behavior; but
+                            // also makes sense, because if this were accepted,
+                            // then users could only write a "Maybe" option at
+                            // the end of the arguments when FloatingFrees is in
+                            // use.
                             if let Some(i_arg) = i_arg.take() {
-                                vals[optid].push((arg_pos, Val(i_arg)));
-                            } else if was_long
-                                || name_pos < names.len()
-                                || args.peek().map_or(true, |n| is_arg(&n))
-                            {
-                                vals[optid].push((arg_pos, Given));
+                                vals[opt_id].push((arg_pos, Val(i_arg)));
                             } else {
-                                vals[optid].push((arg_pos, Val(args.next().unwrap())));
+                                vals[opt_id].push((arg_pos, Given));
                             }
                         }
                         Yes => {
                             if let Some(i_arg) = i_arg.take() {
-                                vals[optid].push((arg_pos, Val(i_arg)));
+                                vals[opt_id].push((arg_pos, Val(i_arg)));
                             } else if let Some(n) = args.next() {
-                                vals[optid].push((arg_pos, Val(n)));
+                                vals[opt_id].push((arg_pos, Val(n)));
                             } else {
                                 return Err(ArgumentMissing(nm.to_string()));
                             }
@@ -551,10 +540,6 @@ impl Options {
                     row.push_str(&short_name);
                     if long_name.width() > 0 {
                         row.push_str(", ");
-                    } else {
-                        // Only a single space here, so that any
-                        // argument is printed in the correct spot.
-                        row.push(' ');
                     }
                 }
                 // FIXME: refer issue #7.
@@ -567,16 +552,21 @@ impl Options {
                 _ => {
                     row.push_str(if self.long_only { "-" } else { "--" });
                     row.push_str(&long_name);
-                    row.push(' ');
                 }
             }
 
             // arg
             match hasarg {
                 No => {}
-                Yes => row.push_str(&hint),
+                Yes => {
+                    row.push(' ');
+                    row.push_str(&hint);
+                }
                 Maybe => {
                     row.push('[');
+                    if !long_name.is_empty() {
+                        row.push('=');
+                    }
                     row.push_str(&hint);
                     row.push(']');
                 }
@@ -979,9 +969,13 @@ fn format_option(opt: &OptGroup) -> String {
     }
 
     if opt.hasarg != No {
-        line.push(' ');
         if opt.hasarg == Maybe {
             line.push('[');
+            if opt.short_name.is_empty() {
+                line.push('=');
+            }
+        } else {
+            line.push(' ');
         }
         line.push_str(&opt.hint);
         if opt.hasarg == Maybe {
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
@@ -379,8 +379,8 @@ fn test_optflagopt() {
     let short_args = vec!["-t".to_string(), "x".to_string()];
     match opts.parse(&short_args) {
         Ok(ref m) => {
-            assert_eq!(m.opt_str("t").unwrap(), "x");
-            assert_eq!(m.opt_str("test").unwrap(), "x");
+            assert_eq!(m.opt_str("t"), None);
+            assert_eq!(m.opt_str("test"), None);
         }
         _ => panic!(),
     }
@@ -763,6 +763,8 @@ fn test_usage() {
     opts.optopt("a", "012345678901234567890123456789", "Desc", "VAL");
     opts.optflag("k", "kiwi", "Desc");
     opts.optflagopt("p", "", "Desc", "VAL");
+    opts.optflagopt("", "pea", "Desc", "VAL");
+    opts.optflagopt("c", "cherry", "Desc", "VAL");
     opts.optmulti("l", "", "Desc", "VAL");
     opts.optflag("", "starfruit", "Starfruit");
 
@@ -773,7 +775,9 @@ Options:
     -a, --012345678901234567890123456789 VAL
                         Desc
     -k, --kiwi          Desc
-    -p [VAL]            Desc
+    -p[VAL]             Desc
+        --pea[=VAL]     Desc
+    -c, --cherry[=VAL]  Desc
     -l VAL              Desc
         --starfruit     Starfruit
 ";
@@ -820,7 +824,7 @@ Options:
 
     debug!("expected: <<{}>>", expected);
     debug!("generated: <<{}>>", usage);
-    assert!(usage == expected)
+    assert_eq!(usage, expected)
 }
 
 #[test]
@@ -851,7 +855,7 @@ Options:
 
     debug!("expected: <<{}>>", expected);
     debug!("generated: <<{}>>", usage);
-    assert!(usage == expected)
+    assert_eq!(usage, expected)
 }
 
 #[test]
@@ -882,7 +886,7 @@ Options:
 
     debug!("expected: <<{}>>", expected);
     debug!("generated: <<{}>>", usage);
-    assert!(usage == expected)
+    assert_eq!(usage, expected)
 }
 
 #[test]
@@ -909,7 +913,7 @@ Options:
     -c, --brûlée        brûlée quite long description
     -k, --kiwi€         kiwi description
     -o, --orange‹       orange description
-    -r, --raspberry-but-making-this-option-way-too-long\u{0020}
+    -r, --raspberry-but-making-this-option-way-too-long
                         raspberry description is also quite long indeed longer
                         than every other piece of text we might encounter here
                         and thus will be automatically broken up
@@ -919,7 +923,7 @@ Options:
 
     debug!("expected: <<{}>>", expected);
     debug!("generated: <<{}>>", usage);
-    assert!(usage == expected)
+    assert_eq!(usage, expected)
 }
 
 #[test]
@@ -934,13 +938,13 @@ fn test_usage_short_only() {
 Options:
     -k VAL              Kiwi
     -s                  Starfruit
-    -a [TYPE]           Apple
+    -a[TYPE]            Apple
 ";
 
     let usage = opts.usage("Usage: fruits");
     debug!("expected: <<{}>>", expected);
     debug!("generated: <<{}>>", usage);
-    assert!(usage == expected)
+    assert_eq!(usage, expected)
 }
 
 #[test]
@@ -955,13 +959,13 @@ fn test_usage_long_only() {
 Options:
     --kiwi VAL          Kiwi
     --starfruit         Starfruit
-    --apple [TYPE]      Apple
+    --apple[=TYPE]      Apple
 ";
 
     let usage = opts.usage("Usage: fruits");
     debug!("expected: <<{}>>", expected);
     debug!("generated: <<{}>>", usage);
-    assert!(usage == expected)
+    assert_eq!(usage, expected)
 }
 
 #[test]
@@ -971,9 +975,10 @@ fn test_short_usage() {
     opts.optopt("a", "012345678901234567890123456789", "Desc", "VAL");
     opts.optflag("k", "kiwi", "Desc");
     opts.optflagopt("p", "", "Desc", "VAL");
-    opts.optmulti("l", "", "Desc", "VAL");
+    opts.optflagopt("", "pea", "Desc", "VAL");
+    opts.optflagopt("c", "cherry", "Desc", "VAL");
 
-    let expected = "Usage: fruits -b VAL [-a VAL] [-k] [-p [VAL]] [-l VAL]..".to_string();
+    let expected = "Usage: fruits -b VAL [-a VAL] [-k] [-p[VAL]] [--pea[=VAL]] [-c[VAL]]".to_string();
     let generated_usage = opts.short_usage("fruits");
 
     debug!("expected: <<{}>>", expected);
@@ -1026,7 +1031,7 @@ Options:
 
     debug!("expected: <<{}>>", expected);
     debug!("generated: <<{}>>", usage);
-    assert!(usage == expected)
+    assert_eq!(usage, expected)
 }
 
 #[test]
@@ -1148,7 +1153,7 @@ fn test_opt_get() {
     opts.optflagopt("p", "percent", "Description", "0.0 .. 10.0");
     opts.long_only(false);
 
-    let args: Vec<String> = ["-i", "true", "-p", "1.1"]
+    let args: Vec<String> = ["--ignore=true", "-p1.1"]
         .iter()
         .map(|x| x.to_string())
         .collect();
@@ -1173,7 +1178,7 @@ fn test_opt_get_default() {
     opts.optflagopt("p", "percent", "Description", "0.0 .. 10.0");
     opts.long_only(false);
 
-    let args: Vec<String> = ["-i", "true", "-p", "1.1"]
+    let args: Vec<String> = ["--ignore=true", "-p1.1"]
         .iter()
         .map(|x| x.to_string())
         .collect();
