Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tertsdiepraam merged 1 commit into from
Apr 20, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
3 changes: 3 additions & 0 deletions src/uu/tail/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
91 changes: 77 additions & 14 deletions src/uu/tail/src/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -182,19 +182,44 @@ impl Settings {
}

pub fn from(matches: &clap::ArgMatches) -> UResult<Self> {
let mut settings: Self = Self {
follow: if matches.get_flag(options::FOLLOW_RETRY) {
Some(FollowMode::Name)
} else if matches.value_source(options::FOLLOW) != Some(ValueSource::CommandLine) {
None
} else if matches.get_one::<String>(options::FOLLOW)
== Some(String::from("name")).as_ref()
// We're parsing --follow, -F and --retry under the following conditions:
// * -F sets --retry and --follow=name
// * plain --follow or short -f is the same like specifying --follow=descriptor
// * All these options and flags can occur multiple times as command line arguments
let follow_retry = matches.get_flag(options::FOLLOW_RETRY);
// We don't need to check for occurrences of --retry if -F was specified which already sets
// 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)
},
retry: matches.get_flag(options::RETRY) || matches.get_flag(options::FOLLOW_RETRY),
}
// * -F and --follow=name if --follow=name is specified after -F
// * No occurrences of -F but --follow=name
// * -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),
Expand Down Expand Up @@ -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"])
Expand Down Expand Up @@ -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(
Expand All @@ -570,6 +596,8 @@ pub fn uu_app() -> Command {

#[cfg(test)]
mod tests {
use rstest::rstest;

use crate::parse::ObsoleteArgs;

use super::*;
Expand Down Expand Up @@ -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);
}
}
17 changes: 9 additions & 8 deletions tests/by-util/test_tail.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()
Expand Down