Skip to content

cut: show error for multiple mode args (-b, -c, -f)#5962

Merged
cakebaker merged 3 commits intouutils:mainfrom
wolimst:cut/fix/multiple-mode-args
Feb 14, 2024
Merged

cut: show error for multiple mode args (-b, -c, -f)#5962
cakebaker merged 3 commits intouutils:mainfrom
wolimst:cut/fix/multiple-mode-args

Conversation

@wolimst
Copy link
Copy Markdown
Contributor

@wolimst wolimst commented Feb 9, 2024

Overview

uutils-cut runs without an error when there are multiple cutting mode arguments of one type, i.e. more than one --bytes, or --characters, or --fields, while GNU cut shows an error as described in GNU cut manual - "Use one, and only one of -b, -c or -f."

For example:

# uutils-cut
$ echo "12345" | cargo run cut -c1 -c5
5

# GNU cut
$ echo "12345" | cut -c1 -c5
cut: only one list may be specified
Try 'cut --help' for more information.

Cause

Duplicated arguments were overriding the previous ones, therefore only the last cutting mode argument (-b, -c, -f) was handled, as you can see on the example above.

Fix

Disable overriding of cutting mode arguments (-b, -c, -f) using ArgAction::APPEND. Count the number of the cutting mode arguments and show an error when there are more than one -b, -c, or -f.

fixes #5925

@wolimst
Copy link
Copy Markdown
Contributor Author

wolimst commented Feb 9, 2024

I used existing error message which was already being used when different types of mode args were given

cut: invalid usage: expects no more than one of --fields (-f), --chars (-c) or --bytes (-b)

but it is not exactly equal to the GNU's error message. Should the error message equal to GNU's?

Copy link
Copy Markdown
Collaborator

@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.

Excellent! Could you annotate this a bit with some comments explaining the issue and how we work around it. For now, the error message is fine, we can change it later if we want to.

Comment on lines +362 to +369
let mode_args_count = [
matches.indices_of(options::BYTES),
matches.indices_of(options::CHARACTERS),
matches.indices_of(options::FIELDS),
]
.into_iter()
.filter_map(|mode| mode.map(|indices| indices.len()))
.sum();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let mode_args_count = [
matches.indices_of(options::BYTES),
matches.indices_of(options::CHARACTERS),
matches.indices_of(options::FIELDS),
]
.into_iter()
.filter_map(|mode| mode.map(|indices| indices.len()))
.sum();
let mode_args_count = [
matches.indices_of(options::BYTES),
matches.indices_of(options::CHARACTERS),
matches.indices_of(options::FIELDS),
]
.into_iter()
.filter_map(|mode| mode.map(|indices| indices.len()))
.sum();

I tried to find a way of expressing this without the nested map. This is what I landed on. Still the still in functionality though. I think this is a pretty good way of doing it!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was also wondering for more clean way to do it to remove nested map. It looks like the suggested change is identical to the original one, could you edit it to what you landed on?

Copy link
Copy Markdown
Contributor

@cakebaker cakebaker Feb 14, 2024

Choose a reason for hiding this comment

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

I don't know what @tertsdiepraam had in mind, here's an idea of how to get rid of the nested map:

let mode_args_count = [
    matches.indices_of(options::BYTES),
    matches.indices_of(options::CHARACTERS),
    matches.indices_of(options::FIELDS),
]
.into_iter()
.map(|indices| indices.unwrap_or_default().count())
.sum();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @cakebaker! It looks like a nice way to transform Option::None to an empty iterator! Also, using count() instead of len() seems more correct way to do it, since it looks like the intended use case of len() is to get the remaining length of the iterator.
Just updated the code!

Copy link
Copy Markdown
Collaborator

@tertsdiepraam tertsdiepraam Feb 14, 2024

Choose a reason for hiding this comment

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

I must've made a mistake while copying code from my editor. I'll take another look in a bit. Sorry for the confusion 😄

Copy link
Copy Markdown
Collaborator

@tertsdiepraam tertsdiepraam Feb 14, 2024

Choose a reason for hiding this comment

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

let mode_args_count = [
    matches.indices_of(options::BYTES),
    matches.indices_of(options::CHARACTERS),
    matches.indices_of(options::FIELDS),
]
.into_iter()
.flatten()
.map(|indices| indices.len()))
.sum();

This is what I had in mind I think.

@wolimst
Copy link
Copy Markdown
Contributor Author

wolimst commented Feb 9, 2024

Thanks, added some comments about the issue and the fix.

@cakebaker cakebaker merged commit 6adaf31 into uutils:main Feb 14, 2024
@cakebaker
Copy link
Copy Markdown
Contributor

Thanks for your PR :)

@wolimst wolimst deleted the cut/fix/multiple-mode-args branch February 21, 2024 11:15
@wolimst wolimst restored the cut/fix/multiple-mode-args branch February 21, 2024 11:15
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.

cut: -c3 -c7 doesn't return anything with GNU but we do

3 participants