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

split: split -1 in should be accepted #5222

Closed
wants to merge 12 commits into from

Conversation

zhitkoff
Copy link
Contributor

Handling of obsolete lines option value + tests including possible use cases/combinations with other short options

Fixes #5033

@github-actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail-2/inotify-dir-recreate
GNU test failed: tests/split/suffix-length. tests/split/suffix-length is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/split/line-bytes. tests/split/line-bytes is passing on 'main'. Maybe you have to rebase?

@zhitkoff zhitkoff marked this pull request as draft August 29, 2023 20:04
@zhitkoff zhitkoff marked this pull request as ready for review August 29, 2023 21:34
@zhitkoff
Copy link
Contributor Author

@tertsdiepraam here is the new PR for this issue, replacing #5170
Reworked to cover more scenarios where obsolete lines option could come up, some edge cases + tests

@@ -52,14 +52,86 @@ const AFTER_HELP: &str = help_section!("after help", "split.md");

#[uucore::main]
pub fn uumain(args: impl uucore::Args) -> UResult<()> {
let args = args.collect_lossy();
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to avoid this where possible. I think we should be operating on OsStr if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was a direct copy from how it is handled in fold , but I will try to see if we can switch to operating on OsStr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I had to keep the same implementation as in fold , i.e. using uucore::Argss collect_lossy() method. There is also collect_ignore() defined, but I think to stay consistent we should use the same approach as fold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, took another pass at it and in the latest commit removed the need for collect_lossy() and let clap panic on invalid non UTF-8 arguments + tests to cover that

let mut obs_lines = None;
let filtered_args = args
.iter()
.filter_map(|slice| {
Copy link
Member

Choose a reason for hiding this comment

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

This method is still no bulletproof unfortunately. The problem is that something that looks like a short option chunk could be an argument to a long flag. For example:

split --additional-suffix -x400a4

With this PR, this is the output I get:

error: a value is required for '--additional-suffix <SUFFIX>' but none was supplied

For more information, try '--help'.

That's quite confusing...

I think that case is more important to get exactly right than the shortcut.

I think the way that GNU split does it, is by storing a number and defining short flags for each digit and when that digit in encountered multiply the number by 10 and adding the digit. You can see this too because split -1 -2 behaves like split -12. I personally think that behaviour is more confusing than helpful, maybe it's the way to go?

You've done a great job with all the case you have taken into account here already, but this method might be a dead end if we want full compatibility. I'd be happy to be proven wrong though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that! I will go deeper into the rabbit hole :)
On the split -1 -2 file example though - the GNU split would treat it as split -2 file , i.e. it will not combine -1 with -2 for a total -12, so at least that part works now :)

Copy link
Member

Choose a reason for hiding this comment

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

It won't? Oh interesting! It's even more clever than I thought then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the code to handle this edge case correctly (in a generic way, should work for all options where it applies) + tests

split --additional-suffix -x400a4

@zhitkoff
Copy link
Contributor Author

zhitkoff commented Sep 4, 2023

@tertsdiepraam checking in - if there're any other suggestions/modification you have for PR before it can be merged?

@zhitkoff
Copy link
Contributor Author

zhitkoff commented Sep 7, 2023

Closing this one as #5252 has been merged instead and it includes all the same code changes as this one

@zhitkoff zhitkoff closed this Sep 7, 2023
@zhitkoff zhitkoff deleted the split-issue5033-updated branch September 7, 2023 15:46
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.

split -1 in should be accepted
3 participants