Merge pull request #4734 from Joining7943/tail-fix-args-parsing-follow-mode

tail/args: Fix parsing when -F is used together with --retry or --follow

Cargo.lock

@@ -3166,6 +3166,7 @@ dependencies = [
  "libc",
  "memchr",
  "notify",
+ "rstest",
  "same-file",
  "uucore",
  "winapi-util",

Cargo.toml

@@ -311,6 +311,7 @@ rand_core = "0.6"
 rayon = "1.7"
 redox_syscall = "0.2"
 regex = "1.7.3"
+rstest = "0.17.0"
 rust-ini = "0.18.0"
 same-file = "1.0.6"
 selinux = "0.4"

src/uu/tail/Cargo.toml

@@ -29,6 +29,9 @@ fundu = { workspace=true }
 windows-sys = { workspace=true, features = ["Win32_System_Threading", "Win32_Foundation"] }
 winapi-util = { workspace=true }
 
+[dev-dependencies]
+rstest = { workspace=true }
+
 [[bin]]
 name = "tail"
 path = "src/main.rs"

src/uu/tail/src/args.rs

@@ -8,7 +8,7 @@
 use crate::paths::Input;
 use crate::{parse, platform, Quotable};
 use clap::crate_version;
-use clap::{parser::ValueSource, Arg, ArgAction, ArgMatches, Command};
+use clap::{Arg, ArgAction, ArgMatches, Command};
 use fundu::DurationParser;
 use is_terminal::IsTerminal;
 use same_file::Handle;
@@ -182,19 +182,44 @@ impl Settings {
     }
 
     pub fn from(matches: &clap::ArgMatches) -> UResult<Self> {
-        let mut settings: Self = Self {
+        // We're parsing --follow, -F and --retry under the following conditions:
-            follow: if matches.get_flag(options::FOLLOW_RETRY) {
+        // * -F sets --retry and --follow=name
-                Some(FollowMode::Name)
+        // * plain --follow or short -f is the same like specifying --follow=descriptor
-            } else if matches.value_source(options::FOLLOW) != Some(ValueSource::CommandLine) {
+        // * All these options and flags can occur multiple times as command line arguments
-                None
+        let follow_retry = matches.get_flag(options::FOLLOW_RETRY);
-            } else if matches.get_one::<String>(options::FOLLOW)
+        // We don't need to check for occurrences of --retry if -F was specified which already sets
-                == Some(String::from("name")).as_ref()
+        // retry
+        let retry = follow_retry || matches.get_flag(options::RETRY);
+        let follow = match (
+            follow_retry,
+            matches
+                .get_one::<String>(options::FOLLOW)
+                .map(|s| s.as_str()),
+        ) {
+            // -F and --follow if -F is specified after --follow. We don't need to care about the
+            // value of --follow.
+            (true, Some(_))
+                // It's ok to use `index_of` instead of `indices_of` since -F and  --follow
+                // overwrite themselves (not only the value but also the index).
+                if matches.index_of(options::FOLLOW_RETRY) > matches.index_of(options::FOLLOW) =>
             {
                 Some(FollowMode::Name)
-            } else {
+            }
-                Some(FollowMode::Descriptor)
+            // * -F and --follow=name if --follow=name is specified after -F
-            },
+            // * No occurrences of -F but --follow=name
-            retry: matches.get_flag(options::RETRY) || matches.get_flag(options::FOLLOW_RETRY),
+            // * -F and no occurrences of --follow
+            (_, Some("name")) | (true, None) => Some(FollowMode::Name),
+            // * -F and --follow=descriptor (or plain --follow, -f) if --follow=descriptor is
+            // specified after -F
+            // * No occurrences of -F but --follow=descriptor, --follow, -f
+            (_, Some(_)) => Some(FollowMode::Descriptor),
+            // The default for no occurrences of -F or --follow
+            (false, None) => None,
+        };
+
+        let mut settings: Self = Self {
+            follow,
+            retry,
             use_polling: matches.get_flag(options::USE_POLLING),
             mode: FilterMode::from(matches)?,
             verbose: matches.get_flag(options::verbosity::VERBOSE),
@@ -469,7 +494,7 @@ pub fn uu_app() -> Command {
             Arg::new(options::FOLLOW)
                 .short('f')
                 .long(options::FOLLOW)
-                .default_value("descriptor")
+                .default_missing_value("descriptor")
                 .num_args(0..=1)
                 .require_equals(true)
                 .value_parser(["descriptor", "name"])
@@ -544,13 +569,14 @@ pub fn uu_app() -> Command {
             Arg::new(options::RETRY)
                 .long(options::RETRY)
                 .help("Keep trying to open a file if it is inaccessible")
+                .overrides_with(options::RETRY)
                 .action(ArgAction::SetTrue),
         )
         .arg(
             Arg::new(options::FOLLOW_RETRY)
                 .short('F')
                 .help("Same as --follow=name --retry")
-                .overrides_with_all([options::RETRY, options::FOLLOW])
+                .overrides_with(options::FOLLOW_RETRY)
                 .action(ArgAction::SetTrue),
         )
         .arg(
@@ -570,6 +596,8 @@ pub fn uu_app() -> Command {
 
 #[cfg(test)]
 mod tests {
+    use rstest::rstest;
+
     use crate::parse::ObsoleteArgs;
 
     use super::*;
@@ -616,4 +644,39 @@ mod tests {
         let result = Settings::from_obsolete_args(&args, Some(&"file".into()));
         assert_eq!(result.follow, Some(FollowMode::Name));
     }
+
+    #[rstest]
+    #[case::default(vec![], None, false)]
+    #[case::retry(vec!["--retry"], None, true)]
+    #[case::multiple_retry(vec!["--retry", "--retry"], None, true)]
+    #[case::follow_long(vec!["--follow"], Some(FollowMode::Descriptor), false)]
+    #[case::follow_short(vec!["-f"], Some(FollowMode::Descriptor), false)]
+    #[case::follow_long_with_retry(vec!["--follow", "--retry"], Some(FollowMode::Descriptor), true)]
+    #[case::follow_short_with_retry(vec!["-f", "--retry"], Some(FollowMode::Descriptor), true)]
+    #[case::follow_overwrites_previous_selection_1(vec!["--follow=name", "--follow=descriptor"], Some(FollowMode::Descriptor), false)]
+    #[case::follow_overwrites_previous_selection_2(vec!["--follow=descriptor", "--follow=name"], Some(FollowMode::Name), false)]
+    #[case::big_f(vec!["-F"], Some(FollowMode::Name), true)]
+    #[case::multiple_big_f(vec!["-F", "-F"], Some(FollowMode::Name), true)]
+    #[case::big_f_with_retry_then_does_not_change(vec!["-F", "--retry"], Some(FollowMode::Name), true)]
+    #[case::big_f_with_follow_descriptor_then_change(vec!["-F", "--follow=descriptor"], Some(FollowMode::Descriptor), true)]
+    #[case::multiple_big_f_with_follow_descriptor_then_no_change(vec!["-F", "--follow=descriptor", "-F"], Some(FollowMode::Name), true)]
+    #[case::big_f_with_follow_short_then_change(vec!["-F", "-f"], Some(FollowMode::Descriptor), true)]
+    #[case::follow_descriptor_with_big_f_then_change(vec!["--follow=descriptor", "-F"], Some(FollowMode::Name), true)]
+    #[case::follow_short_with_big_f_then_change(vec!["-f", "-F"], Some(FollowMode::Name), true)]
+    #[case::big_f_with_follow_name_then_not_change(vec!["-F", "--follow=name"], Some(FollowMode::Name), true)]
+    #[case::follow_name_with_big_f_then_not_change(vec!["--follow=name", "-F"], Some(FollowMode::Name), true)]
+    #[case::big_f_with_multiple_long_follow(vec!["--follow=name", "-F", "--follow=descriptor"], Some(FollowMode::Descriptor), true)]
+    #[case::big_f_with_multiple_long_follow_name(vec!["--follow=name", "-F", "--follow=name"], Some(FollowMode::Name), true)]
+    #[case::big_f_with_multiple_short_follow(vec!["-f", "-F", "-f"], Some(FollowMode::Descriptor), true)]
+    #[case::multiple_big_f_with_multiple_short_follow(vec!["-f", "-F", "-f", "-F"], Some(FollowMode::Name), true)]
+    fn test_parse_settings_follow_mode_and_retry(
+        #[case] args: Vec<&str>,
+        #[case] expected_follow_mode: Option<FollowMode>,
+        #[case] expected_retry: bool,
+    ) {
+        let settings =
+            Settings::from(&uu_app().no_binary_name(true).get_matches_from(args)).unwrap();
+        assert_eq!(settings.follow, expected_follow_mode);
+        assert_eq!(settings.retry, expected_retry);
+    }
 }

tests/by-util/test_tail.rs

@@ -4358,14 +4358,7 @@ fn test_follow_when_file_and_symlink_are_pointing_to_same_file_and_append_data()
         .stdout_only(expected_stdout);
 }
 
-// Fails with:
-// 'Assertion failed. Expected 'tail' to be running but exited with status=exit status: 1.
-// stdout:
-// stderr: tail: warning: --retry ignored; --retry is useful only when following
-// tail: error reading 'dir': Is a directory
-// '
 #[test]
-#[cfg(disabled_until_fixed)]
 fn test_args_when_directory_given_shorthand_big_f_together_with_retry() {
     let scene = TestScenario::new(util_name!());
     let fixtures = &scene.fixtures;
@@ -4377,9 +4370,17 @@ fn test_args_when_directory_given_shorthand_big_f_together_with_retry() {
          tail: {0}: cannot follow end of this type of file\n",
         dirname
     );
-
     let mut child = scene.ucmd().args(&["-F", "--retry", "dir"]).run_no_wait();
 
+    child.make_assertion_with_delay(500).is_alive();
+    child
+        .kill()
+        .make_assertion()
+        .with_current_output()
+        .stderr_only(&expected_stderr);
+
+    let mut child = scene.ucmd().args(&["--retry", "-F", "dir"]).run_no_wait();
+
     child.make_assertion_with_delay(500).is_alive();
     child
         .kill()