Skip to content
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.

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

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

Merged

Conversation

Joining7943
Copy link
Contributor

This pr fixes the argument parsing when -F is used together with --retry or --follow.

tail -F --retry should not erase the implicit --follow option, tail -F --follow should not erase the implicit --retry flag etc.

@Joining7943
Copy link
Contributor Author

@tertsdiepraam This pr is ready :)

@Joining7943 Joining7943 mentioned this pull request Apr 14, 2023
@Joining7943
Copy link
Contributor Author

@tertsdiepraam Could you review, please?

Comment on lines 196 to 218
(true, Some(value)) if value == "name" => (Some(FollowMode::Name), true),
(true, Some(_)) => (Some(FollowMode::Descriptor), true),
(true, None) => (Some(FollowMode::Name), true),
(false, Some(value)) if value == "name" => {
(Some(FollowMode::Name), matches.get_flag(options::RETRY))
}
(false, Some(_)) => (
Some(FollowMode::Descriptor),
matches.get_flag(options::RETRY),
),
(false, None) => (None, matches.get_flag(options::RETRY)),
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please document the various cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Joining7943 Joining7943 force-pushed the tail-fix-args-parsing-follow-mode branch from 6dd28bb to 5cc3814 Compare April 18, 2023 15:12
src/uu/tail/Cargo.toml Outdated Show resolved Hide resolved
// * plain --follow or short -f is the same like specifying --follow=descriptor and can
// overwrite itself (specified multiple times)
// * specifying --retry together with -F doesn't change the setting of `retry` to true. The
// --retry flag itself is restricted by clap to be specified only once (unlike gnu's tail)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we match GNU by allowing retry multiple times?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. That's just the way it was before this pr and I wasn't sure. I think -F should be allowed also multiple times

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fully compliant with gnu's tail, I think we should set clap_builder::builder::Command::args_override_self to true globally for all arguments and not only for specific ones. Afaik, this is the way how they parse arguments. I could do this in another pr, maybe together with unit testing the arguments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Let's do that separately indeed.

src/uu/tail/src/args.rs Outdated Show resolved Hide resolved
@Joining7943 Joining7943 force-pushed the tail-fix-args-parsing-follow-mode branch from 5cc3814 to 2f44fe5 Compare April 18, 2023 19:39
@Joining7943 Joining7943 force-pushed the tail-fix-args-parsing-follow-mode branch from 2f44fe5 to 20e3297 Compare April 18, 2023 19:47
@uutils uutils deleted a comment from github-actions bot Apr 19, 2023
@uutils uutils deleted a comment from github-actions bot Apr 19, 2023
@sylvestre sylvestre requested a review from tertsdiepraam April 19, 2023 19:28
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@tertsdiepraam tertsdiepraam merged commit d68fd68 into uutils:main Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants