From 682586bfd0427d838669d7eca43a3acdd8ce9f39 Mon Sep 17 00:00:00 2001 From: Abhishek Chandramouli Sharma Date: Wed, 2 Feb 2022 17:53:13 -0800 Subject: [PATCH 1/4] ls: add new optional arguments to --classify flag The --classify flag in ls now takes an option when argument that may have the values always, auto and none. Modified clap argument to allow an optional parameter and changed the classify flag value parsing logic to account for this change. --- src/uu/ls/src/ls.rs | 27 ++++++++++++++++++++++++--- tests/by-util/test_ls.rs | 1 + 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 7fdec53f08d..bf62cae41f9 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -583,8 +583,19 @@ impl Config { "slash" => IndicatorStyle::Slash, &_ => IndicatorStyle::None, } - } else if options.is_present(options::indicator_style::CLASSIFY) { - IndicatorStyle::Classify + } else if let Some(field) = options.value_of(options::indicator_style::CLASSIFY) { + match field { + "none" => IndicatorStyle::None, + "always" => IndicatorStyle::Classify, + "auto" => { + if atty::is(atty::Stream::Stdout) { + IndicatorStyle::Classify + } else { + IndicatorStyle::None + } + } + &_ => IndicatorStyle::None, + } } else if options.is_present(options::indicator_style::SLASH) { IndicatorStyle::Slash } else if options.is_present(options::indicator_style::FILE_TYPE) { @@ -1209,8 +1220,18 @@ only ignore '.' and '..'.", "Append a character to each file name indicating the file type. Also, for \ regular files that are executable, append '*'. The file type indicators are \ '/' for directories, '@' for symbolic links, '|' for FIFOs, '=' for sockets, \ - '>' for doors, and nothing for regular files.", + '>' for doors, and nothing for regular files. when may be omitted, or one of:\n\ + \tnone - Do not classify. This is the default.\n\ + \tauto - Only classify if standard output is a terminal.\n\ + \talways - Always classify.\n\ + Specifying --classify and no when is equivalent to --classify=always. This will not follow\ + symbolic links listed on the command line unless the --dereference-command-line (-H),\ + --dereference (-L), or --dereference-command-line-symlink-to-dir options are specified.", ) + .takes_value(true) + .value_name("when") + .possible_values(&["none", "auto", "always"]) + .default_missing_value("always") .overrides_with_all(&[ options::indicator_style::FILE_TYPE, options::indicator_style::SLASH, diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index e3fd99e0090..cb4192f7a34 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -1558,6 +1558,7 @@ fn test_ls_indicator_style() { "--indicator-style=slash", "--ind=slash", "--classify", + "--classify=always", "--class", "--file-type", "--file", From 36cf09d2ae99b3f46e4365898be788685f47e3dc Mon Sep 17 00:00:00 2001 From: Abhishek Chandramouli Sharma Date: Wed, 2 Feb 2022 23:50:42 -0800 Subject: [PATCH 2/4] ls: add test for indicator-style, ind and classify with value none --- tests/by-util/test_ls.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index cb4192f7a34..b72ae8b7fc6 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -1568,6 +1568,18 @@ fn test_ls_indicator_style() { scene.ucmd().arg(opt).succeeds().stdout_contains(&"/"); } + // Classify, Indicator options should not contain any indicators when value is none. + for opt in ["--indicator-style=none", "--classify=none", "--ind=none"] { + // Verify that there are no indicators for any of the file types. + scene + .ucmd() + .arg(opt) + .succeeds() + .stdout_does_not_contain(&"/") + .stdout_does_not_contain(&"@") + .stdout_does_not_contain(&"|"); + } + // Classify and File-Type all contain indicators for pipes and links. let options = vec!["classify", "file-type"]; for opt in options { From 602a81d9c082050606e27cd026e31d0f386e0e58 Mon Sep 17 00:00:00 2001 From: Abhishek Chandramouli Sharma Date: Thu, 3 Feb 2022 02:29:33 -0800 Subject: [PATCH 3/4] ls: require option paramter to --classify to use a = to specify flag value --- src/uu/ls/src/ls.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index bf62cae41f9..fdc706ccb8c 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1232,6 +1232,8 @@ only ignore '.' and '..'.", .value_name("when") .possible_values(&["none", "auto", "always"]) .default_missing_value("always") + .require_equals(true) + .min_values(0) .overrides_with_all(&[ options::indicator_style::FILE_TYPE, options::indicator_style::SLASH, From 1064fc6f06b8cc731a13822dceb334388a3ea5a8 Mon Sep 17 00:00:00 2001 From: Abhishek Chandramouli Sharma Date: Thu, 3 Feb 2022 10:11:13 -0800 Subject: [PATCH 4/4] ls: account for all the undocumented possible values for the --classify flag Added the other values for the --classify flag along with modifications to tests. Also documented the inconsistency between GNU coreutils because we accept the flag value even for the short version of the flag. --- src/uu/ls/src/ls.rs | 15 +++++++++++---- tests/by-util/test_ls.rs | 10 +++++++++- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index fdc706ccb8c..d82915c853a 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -585,9 +585,9 @@ impl Config { } } else if let Some(field) = options.value_of(options::indicator_style::CLASSIFY) { match field { - "none" => IndicatorStyle::None, - "always" => IndicatorStyle::Classify, - "auto" => { + "never" | "no" | "none" => IndicatorStyle::None, + "always" | "yes" | "force" => IndicatorStyle::Classify, + "auto" | "tty" | "if-tty" => { if atty::is(atty::Stream::Stdout) { IndicatorStyle::Classify } else { @@ -1213,6 +1213,11 @@ only ignore '.' and '..'.", ]), ) .arg( + // The --classify flag can take an optional when argument to + // control its behavior from version 9 of GNU coreutils. + // There is currently an inconsistency where GNU coreutils allows only + // the long form of the flag to take the argument while we allow it + // for both the long and short form of the flag. Arg::new(options::indicator_style::CLASSIFY) .short('F') .long(options::indicator_style::CLASSIFY) @@ -1230,7 +1235,9 @@ only ignore '.' and '..'.", ) .takes_value(true) .value_name("when") - .possible_values(&["none", "auto", "always"]) + .possible_values(&[ + "always", "yes", "force", "auto", "tty", "if-tty", "never", "no", "none", + ]) .default_missing_value("always") .require_equals(true) .min_values(0) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index b72ae8b7fc6..85403f27f03 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -1559,6 +1559,8 @@ fn test_ls_indicator_style() { "--ind=slash", "--classify", "--classify=always", + "--classify=yes", + "--classify=force", "--class", "--file-type", "--file", @@ -1569,7 +1571,13 @@ fn test_ls_indicator_style() { } // Classify, Indicator options should not contain any indicators when value is none. - for opt in ["--indicator-style=none", "--classify=none", "--ind=none"] { + for opt in [ + "--indicator-style=none", + "--ind=none", + "--classify=none", + "--classify=never", + "--classify=no", + ] { // Verify that there are no indicators for any of the file types. scene .ucmd()