split: suffix length refactor by zhitkoff · Pull Request #5449 · uutils/coreutils

zhitkoff · 2023-10-24T17:19:56Z

Refactoring suffix length processing, distinguishing between dynamic auto-widening vs. pre-calculated auto-width and addressing an edge case of passing 0 as a value for suffix length in command line option,
i.e. split -a0 file
or split --suffix-length=0 file

zhitkoff · 2023-10-25T00:54:33Z

@sylvestre would you mind reviewing this one?

tertsdiepraam

Cool! Nice tests! I have a few small suggestions, but looks good overall, especially given all the edge case we have to take into account.

tertsdiepraam · 2023-10-25T09:55:11Z

src/uu/split/src/split.rs

+            let opt = matches.get_one::<String>(OPT_NUMERIC_SUFFIXES); // if option was specified, but without value - this will return None as there is no default value
+            if opt.is_some() {
+                suffix_start = opt
+                    .unwrap()


Suggested change

let opt = matches.get_one::<String>(OPT_NUMERIC_SUFFIXES); // if option was specified, but without value - this will return None as there is no default value

if opt.is_some() {

suffix_start = opt

.unwrap()

// if option was specified, but without value - this will return None as there is no default value

if let Some(opt) = matches.get_one::<String>(OPT_NUMERIC_SUFFIXES) {

suffix_start = opt

tertsdiepraam · 2023-10-25T09:58:52Z

src/uu/split/src/split.rs

-    // and not from default value
-    if matches.value_source(OPT_SUFFIX_LENGTH) == Some(ValueSource::CommandLine) {
+    // Disable dynamic auto-widening if suffix length was specified in command line with value > 0
+    if matches.value_source(OPT_SUFFIX_LENGTH) == Some(ValueSource::CommandLine)


I'd really appreciate if we could avoid this check. I think we should remove the default value for OPT_SUFFIX_LENGTH because that default value does have the same meaning as the same value provided by the user, which is why we have to do these ugly checks.

so, for this option we need to account for the following scenarios as they all have different outcomes, especially in combination with other options (like --numeric-suffixes, --hex-suffixes and --number):

value was not provided in cmd line

value was provided in cmd line and it is > 0

value was provided in cmd line and it is == 0

and these need to be done at least in two different places.

If we drop default value on this options, we would still need to distinguish between these 3 scenarios in all applicable combinations. It would just be different checks and possibly parsing to usize more then once or introducing additional temp variables, since we cannot do all of these checks in one place - due to how it works in combination with other options.

I will try a few different ways and see if it can be simplified.

pushed new version that should address this one as well

tertsdiepraam · 2023-10-25T09:59:02Z

src/uu/split/src/split.rs

+///   set auto widening OFF AND auto pre-calculate required suffix length based on number of files needed
+/// - If suffix length is specified as 0 in any other situation
+///   keep auto widening ON and suffix length to default value
+///


I can't put a comment on that line, but if we're refactoring this, I think it makes sense to make (SuffixType, usize, bool, usize) a Suffix type.

yep, I considered it. The best way is to separate suffix (or whole Settings) and Strategy into their own modules/files though, otherwise it would be cumbersome to pass this new Suffix type to FilenameIterator in filenames.rs. And if we keep the current way suffix values are passed to it, then there is no much simplification - just in calling suffix_from.
Would you be OK with that approach?

Or ... new Suffix type goes to filenames.rs, but Strategy would need to moved into its own crate/file, since the suffix_from (which I would imagine should be part of impl Suffix) uses it.

I don't think we need separate crates. Even for modules using them in submodules should be fine, but I'll leave this up to you.

zhitkoff · 2023-10-28T00:24:46Z

@tertsdiepraam with latest changes, could you please let me know if you have other comments/suggestions?

tertsdiepraam

Looks really good! Just one small question.

Also please be careful with moving things around. If you want to do that, it's best to do it in a separate commit. That makes it easier to review.

tertsdiepraam · 2023-10-30T14:06:57Z

src/uu/split/src/filenames.rs

+            .get_one::<String>(OPT_ADDITIONAL_SUFFIX)
+            .unwrap()
+            .to_string();
+        if additional.contains('/') {


Should this be \ as well for Windows?

makes sense, pushed an update

tertsdiepraam · 2023-11-02T12:59:34Z

src/uu/split/src/filenames.rs

+            .get_one::<String>(OPT_ADDITIONAL_SUFFIX)
+            .unwrap()
+            .to_string();
+        if additional.contains('/') || additional.contains('\\') {


I think the check should probably be done with this function https://doc.rust-lang.org/std/path/fn.is_separator.html to be entirely correct, because only / should work on Unix and both \ and / should work on Windows (and there might be other exceptions for other platforms).

It would probably look something like this:

Suggested change

if additional.contains('/') || additional.contains('\\') {

if additional.chars().any(std::path::is_separator) {

Mention me again when it's done and we'll merge this :)

tertsdiepraam

Great, thanks! Can be merged once the CI is green!

github-actions · 2023-11-02T15:30:30Z

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm2 is no longer failing!
Congrats! The gnu test tests/tail/inotify-dir-recreate is no longer failing!
Skip an intermittent issue tests/rm/rm1
GNU test failed: tests/tail/truncate. tests/tail/truncate is passing on 'main'. Maybe you have to rebase?

zhitkoff · 2023-11-02T16:02:12Z

@tertsdiepraam looks like it is good to merge. The GNU test failures are unrelated - something is not right with tail

github-actions · 2023-11-02T17:10:31Z

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm2 is no longer failing!
Skip an intermittent issue tests/rm/rm1

cakebaker · 2023-11-03T07:28:58Z

Thanks!

tertsdiepraam reviewed Oct 25, 2023

View reviewed changes

zhitkoff requested a review from tertsdiepraam October 26, 2023 21:36

tertsdiepraam reviewed Oct 30, 2023

View reviewed changes

zhitkoff requested a review from tertsdiepraam November 1, 2023 19:01

tertsdiepraam reviewed Nov 2, 2023

View reviewed changes

tertsdiepraam approved these changes Nov 2, 2023

View reviewed changes

zhitkoff added 5 commits November 2, 2023 17:17

split: refactor suffix auto-widening and auto-width

733359d

split: suffix auto-widening and auto-width tests

fbb454a

split: refactor filename suffix

8372a3d

split: slash separator

f05474a

split: directory separator in additional suffix

62887c7

sylvestre force-pushed the split-suffix-length-refactor branch from 6f12c33 to 62887c7 Compare November 2, 2023 16:17

cakebaker merged commit f8c474e into uutils:main Nov 3, 2023

chenrui333 mentioned this pull request Nov 14, 2023

uutils-coreutils 0.0.23 Homebrew/homebrew-core#154288

Merged

moonfruit mentioned this pull request Nov 17, 2023

uutils-selected 0.0.23 moonfruit/homebrew-tap#59

Merged

zhitkoff deleted the split-suffix-length-refactor branch November 18, 2023 16:37

	if additional.contains('/') \|\| additional.contains('\\') {
	if additional.chars().any(std::path::is_separator) {

Uh oh!

Conversation

zhitkoff commented Oct 24, 2023

Uh oh!

zhitkoff commented Oct 25, 2023

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhitkoff commented Oct 28, 2023

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 2, 2023

Uh oh!

zhitkoff commented Nov 2, 2023

Uh oh!

github-actions bot commented Nov 2, 2023

Uh oh!

cakebaker commented Nov 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants