Code review feedback

Author: benjyw

src/rust/engine/options/src/args.rs

@@ -145,9 +145,7 @@ impl OptionsSource for Args {
 
     fn get_dict(&self, id: &OptionId) -> Result<Option<DictEdit>, String> {
         match self.find_flag(Self::arg_names(id, Negate::False))? {
-            Some((name, ref value, _)) => parse_dict(value)
-                .map(Some)
-                .map_err(|e| e.render(name)),
+            Some((name, ref value, _)) => parse_dict(value).map(Some).map_err(|e| e.render(name)),
             None => Ok(None),
         }
     }

src/rust/engine/options/src/lib.rs

@@ -34,7 +34,6 @@ use std::collections::{BTreeMap, HashMap, HashSet};
 use std::fmt::Debug;
 use std::hash::Hash;
 use std::os::unix::ffi::OsStrExt;
-use std::path;
 use std::path::Path;
 use std::rc::Rc;
 
@@ -259,22 +258,20 @@ impl OptionParser {
         };
 
         fn path_join(prefix: &str, suffix: &str) -> String {
-            if prefix.ends_with(path::MAIN_SEPARATOR) {
-                format!("{}{}", prefix, suffix)
-            } else {
-                format!("{}{}{}", prefix, path::MAIN_SEPARATOR, suffix)
-            }
+            // TODO: The calling code should traffic in Path, or OsString, not String.
+            //  For now we assume the paths are valid UTF8 strings, via unwrap().
+            Path::new(prefix).join(suffix).to_str().unwrap().to_string()
         }
 
         fn path_strip(prefix: &str, path: &str) -> String {
-            match path.strip_prefix(prefix) {
-                Some(suffix) => match suffix.strip_prefix(path::MAIN_SEPARATOR) {
-                    Some(suffix_suffix) => suffix_suffix,
-                    _ => suffix,
-                },
-                _ => path,
-            }
-            .to_string()
+            // TODO: The calling code should traffic in Path, or OsString, not String.
+            //  For now we assume the paths are valid UTF8 strings, via unwrap().
+            Path::new(path)
+                .strip_prefix(prefix)
+                .unwrap()
+                .to_str()
+                .unwrap()
+                .to_string()
         }
 
         let repo_config_files = match config_paths {
@@ -316,7 +313,7 @@ impl OptionParser {
                     ordinal,
                     path: path_strip(&buildroot_string, path),
                 },
-                Rc::new(config.clone()),
+                Rc::new(config),
             );
             ordinal += 1;
         }

src/rust/engine/options/src/tests.rs

@@ -73,6 +73,33 @@ fn with_setup(
     do_check(option_parser);
 }
 
+#[test]
+fn test_source_ordering() {
+    assert!(
+        Source::Default
+            < Source::Config {
+                ordinal: 0,
+                path: "pants.toml".to_string()
+            }
+    );
+    assert!(
+        Source::Config {
+            ordinal: 0,
+            path: "pants.toml".to_string()
+        } < Source::Config {
+            ordinal: 1,
+            path: "extra_pants.toml".to_string()
+        }
+    );
+    assert!(
+        Source::Config {
+            ordinal: 1,
+            path: "extra_pants.toml".to_string()
+        } < Source::Env
+    );
+    assert!(Source::Env < Source::Flag);
+}
+
 #[test]
 fn test_parse_single_valued_options() {
     fn check(