ls: overrideable -n option (#1917)

src/uu/ls/src/ls.rs

@@ -84,6 +84,7 @@ pub mod options {
         pub static COMMAS: &str = "m";
         pub static LONG_NO_OWNER: &str = "g";
         pub static LONG_NO_GROUP: &str = "o";
+        pub static LONG_NUMERIC_UID_GID: &str = "numeric-uid-gid";
     }
     pub mod files {
         pub static ALL: &str = "all";
@@ -114,7 +115,6 @@ pub mod options {
     pub static CLASSIFY: &str = "classify";
     pub static INODE: &str = "inode";
     pub static DEREFERENCE: &str = "dereference";
-    pub static NUMERIC_UID_GID: &str = "numeric-uid-gid";
     pub static REVERSE: &str = "reverse";
     pub static RECURSIVE: &str = "recursive";
     pub static COLOR: &str = "color";
@@ -167,7 +167,6 @@ struct Config {
     classify: bool,
     ignore_backups: bool,
     size_format: SizeFormat,
-    numeric_uid_gid: bool,
     directory: bool,
     time: Time,
     #[cfg(unix)]
@@ -183,6 +182,8 @@ struct LongFormat {
     author: bool,
     group: bool,
     owner: bool,
+    #[cfg(unix)]
+    numeric_uid_gid: bool,
 }
 
 impl Config {
@@ -210,7 +211,7 @@ impl Config {
             (Format::Columns, options::format::COLUMNS)
         };
 
-        // The -o and -g options are tricky. They cannot override with each
+        // The -o, -n and -g options are tricky. They cannot override with each
         // other because it's possible to combine them. For example, the option
         // -og should hide both owner and group. Furthermore, they are not
         // reset if -l or --format=long is used. So these should just show the
@@ -223,42 +224,26 @@ impl Config {
         // which always applies.
         //
         // The idea here is to not let these options override with the other
-        // options, but manually check the last index they occur. If this index
+        // options, but manually whether they have an index that's greater than
-        // is larger than the index for the other format options, we apply the
+        // the other format options. If so, we set the appropriate format.
-        // long format.
+        if format != Format::Long {
-        match options.indices_of(opt).map(|x| x.max().unwrap()) {
+            let idx = options.indices_of(opt).map(|x| x.max().unwrap()).unwrap_or(0);    
-            None => {
+            if [options::format::LONG_NO_OWNER, options::format::LONG_NO_GROUP, options::format::LONG_NUMERIC_UID_GID]
-                if options.is_present(options::format::LONG_NO_GROUP)
+                .iter()
-                    || options.is_present(options::format::LONG_NO_OWNER)
+                .flat_map(|opt| options.indices_of(opt))
-                {
+                .flatten()
-                    format = Format::Long;
+                .any(|i| i >= idx)
-                } else if options.is_present(options::format::ONELINE) {
+            {
-                    format = Format::OneLine;
+                format = Format::Long;
-                }
+            } else {
-            }
+                if let Some(mut indices) = options.indices_of(options::format::ONELINE) {
-            Some(mut idx) => {
+                    if indices.any(|i| i > idx) {
-                if let Some(indices) = options.indices_of(options::format::LONG_NO_OWNER) {
-                    let i = indices.max().unwrap();
-                    if i > idx {
-                        format = Format::Long;
-                        idx = i;
-                    }
-                }
-                if let Some(indices) = options.indices_of(options::format::LONG_NO_GROUP) {
-                    let i = indices.max().unwrap();
-                    if i > idx {
-                        format = Format::Long;
-                        idx = i;
-                    }
-                }
-                if let Some(indices) = options.indices_of(options::format::ONELINE) {
-                    let i = indices.max().unwrap();
-                    if i > idx && format != Format::Long {
                         format = Format::OneLine;
                     }
                 }
             }
         }
+        
 
         let files = if options.is_present(options::files::ALL) {
             Files::All
@@ -328,10 +313,14 @@ impl Config {
             let group = !options.is_present(options::NO_GROUP)
                 && !options.is_present(options::format::LONG_NO_GROUP);
             let owner = !options.is_present(options::format::LONG_NO_OWNER);
+            #[cfg(unix)]
+            let numeric_uid_gid = options.is_present(options::format::LONG_NUMERIC_UID_GID);
             LongFormat {
                 author,
                 group,
                 owner,
+                #[cfg(unix)]
+                numeric_uid_gid,
             }
         };
 
@@ -355,7 +344,6 @@ impl Config {
             classify: options.is_present(options::CLASSIFY),
             ignore_backups: options.is_present(options::IGNORE_BACKUPS),
             size_format,
-            numeric_uid_gid: options.is_present(options::NUMERIC_UID_GID),
             directory: options.is_present(options::DIRECTORY),
             time,
             #[cfg(unix)]
@@ -444,22 +432,36 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
                     options::format::COLUMNS,
                 ]),
         )
-        // The next three arguments do not override with the other format
+        // The next four arguments do not override with the other format
         // options, see the comment in Config::from for the reason.
+        // Ideally, they would use Arg::override_with, with their own name
+        // but that doesn't seem to work in all cases. Example:
+        // ls -1g1
+        // even though `ls -11` and `ls -1 -g -1` work.
         .arg(
             Arg::with_name(options::format::ONELINE)
                 .short(options::format::ONELINE)
                 .help("List one file per line.")
+                .multiple(true)
         )
         .arg(
             Arg::with_name(options::format::LONG_NO_GROUP)
                 .short(options::format::LONG_NO_GROUP)
                 .help("Long format without group information. Identical to --format=long with --no-group.")
+                .multiple(true)
         )
         .arg(
             Arg::with_name(options::format::LONG_NO_OWNER)
                 .short(options::format::LONG_NO_OWNER)
                 .help("Long format without owner information.")
+                .multiple(true)
+        )
+        .arg(
+            Arg::with_name(options::format::LONG_NUMERIC_UID_GID)
+                .short("n")
+                .long(options::format::LONG_NUMERIC_UID_GID)
+                .help("-l with numeric UIDs and GIDs.")
+                .multiple(true)
         )
 
         // Time arguments
@@ -657,12 +659,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
                 file the link references rather than the link itself.",
                 ),
         )
-        .arg(
+        
-            Arg::with_name(options::NUMERIC_UID_GID)
-                .short("n")
-                .long(options::NUMERIC_UID_GID)
-                .help("-l with numeric UIDs and GIDs."),
-        )
         .arg(
             Arg::with_name(options::REVERSE)
                 .short("r")
@@ -852,7 +849,7 @@ fn pad_left(string: String, count: usize) -> String {
 }
 
 fn display_items(items: &[PathBuf], strip: Option<&Path>, config: &Config) {
-    if config.format == Format::Long || config.numeric_uid_gid {
+    if config.format == Format::Long {
         let (mut max_links, mut max_size) = (1, 1);
         for item in items {
             let (links, size) = display_dir_entry_size(item, config);
@@ -994,7 +991,7 @@ use uucore::entries;
 
 #[cfg(unix)]
 fn display_uname(metadata: &Metadata, config: &Config) -> String {
-    if config.numeric_uid_gid {
+    if config.long.numeric_uid_gid {
         metadata.uid().to_string()
     } else {
         entries::uid2usr(metadata.uid()).unwrap_or_else(|_| metadata.uid().to_string())
@@ -1003,7 +1000,7 @@ fn display_uname(metadata: &Metadata, config: &Config) -> String {
 
 #[cfg(unix)]
 fn display_group(metadata: &Metadata, config: &Config) -> String {
-    if config.numeric_uid_gid {
+    if config.long.numeric_uid_gid {
         metadata.gid().to_string()
     } else {
         entries::gid2grp(metadata.gid()).unwrap_or_else(|_| metadata.gid().to_string())

tests/by-util/test_ls.rs

@@ -297,15 +297,24 @@ fn test_ls_long_formats() {
     // Regex for three names, so all of author, group and owner
     let re_three = Regex::new(r"[xrw-]{9} \d ([-0-9_a-z]+ ){3}0").unwrap();
 
+    #[cfg(unix)]
+    let re_three_num = Regex::new(r"[xrw-]{9} \d (\d+ ){3}0").unwrap();
+
     // Regex for two names, either:
     // - group and owner
     // - author and owner
     // - author and group
     let re_two = Regex::new(r"[xrw-]{9} \d ([-0-9_a-z]+ ){2}0").unwrap();
 
+    #[cfg(unix)]
+    let re_two_num = Regex::new(r"[xrw-]{9} \d (\d+ ){2}0").unwrap();
+
     // Regex for one name: author, group or owner
     let re_one = Regex::new(r"[xrw-]{9} \d [-0-9_a-z]+ 0").unwrap();
 
+    #[cfg(unix)]
+    let re_one_num = Regex::new(r"[xrw-]{9} \d \d+ 0").unwrap();
+
     // Regex for no names
     let re_zero = Regex::new(r"[xrw-]{9} \d 0").unwrap();
 
@@ -329,6 +338,19 @@ fn test_ls_long_formats() {
     println!("stdout = {:?}", result.stdout);
     assert!(re_three.is_match(&result.stdout));
 
+    #[cfg(unix)]
+    {
+        let result = scene
+            .ucmd()
+            .arg("-n")
+            .arg("--author")
+            .arg("test-long-formats")
+            .succeeds();
+        println!("stderr = {:?}", result.stderr);
+        println!("stdout = {:?}", result.stdout);
+        assert!(re_three_num.is_match(&result.stdout));
+    }
+
     for arg in &[
         "-l",                     // only group and owner
         "-g --author",            // only author and group
@@ -344,6 +366,19 @@ fn test_ls_long_formats() {
         println!("stderr = {:?}", result.stderr);
         println!("stdout = {:?}", result.stdout);
         assert!(re_two.is_match(&result.stdout));
+
+        #[cfg(unix)]
+        {
+            let result = scene
+                .ucmd()
+                .arg("-n")
+                .args(&arg.split(" ").collect::<Vec<_>>())
+                .arg("test-long-formats")
+                .succeeds();
+            println!("stderr = {:?}", result.stderr);
+            println!("stdout = {:?}", result.stdout);
+            assert!(re_two_num.is_match(&result.stdout));
+        }
     }
 
     for arg in &[
@@ -364,6 +399,19 @@ fn test_ls_long_formats() {
         println!("stderr = {:?}", result.stderr);
         println!("stdout = {:?}", result.stdout);
         assert!(re_one.is_match(&result.stdout));
+
+        #[cfg(unix)]
+        {
+            let result = scene
+                .ucmd()
+                .arg("-n")
+                .args(&arg.split(" ").collect::<Vec<_>>())
+                .arg("test-long-formats")
+                .succeeds();
+            println!("stderr = {:?}", result.stderr);
+            println!("stdout = {:?}", result.stdout);
+            assert!(re_one_num.is_match(&result.stdout));
+        }
     }
 
     for arg in &[
@@ -387,6 +435,19 @@ fn test_ls_long_formats() {
         println!("stderr = {:?}", result.stderr);
         println!("stdout = {:?}", result.stdout);
         assert!(re_zero.is_match(&result.stdout));
+
+        #[cfg(unix)]
+        {
+            let result = scene
+                .ucmd()
+                .arg("-n")
+                .args(&arg.split(" ").collect::<Vec<_>>())
+                .arg("test-long-formats")
+                .succeeds();
+            println!("stderr = {:?}", result.stderr);
+            println!("stdout = {:?}", result.stdout);
+            assert!(re_zero.is_match(&result.stdout));
+        }
     }
 }