pr: add -e/--expand-tabs option parsing

[ChrisDryden]

The option for adding -e and --expand-tabs was not added to PR and we have a bunch of failed exit code mismatch errors in the GNU tests for this option. There are many issues outstanding with the implementation of the PR utility and I was hoping to start with the parsing of all of the missing options and once that's added to work away at fixing all of the issues in the implementation.

This also includes the option alias of column for columns.

I added all of the possible option combinations for -e to make sure that it is not failing.

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/shuf/shuf-reservoir (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/sort/sort-stale-thread-mem (passes in this run but fails in the 'main' branch)

[github-actions]

GNU testsuite comparison:

Congrats! The gnu test tests/stty/bad-speed is now passing!

[codspeed-hq]

CodSpeed Performance Report

Merging this PR will degrade performance by 3.09%

_{Comparing ChrisDryden:pr-expand-tabs-option (049f52a) with main (a17e814)}

Summary

❌ 1 regressed benchmark
✅ 141 untouched benchmarks
⏩ 37 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
❌	tsort_input_parsing_heavy[5000]	70.2 ms	72.5 ms	-3.09%

Footnotes

1. 37 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[cerdelen]

I was trying to tackle this problem with a friend too and we encountered problems with clap trying to parse the -e flag.

After a while we came up with a similar approach with you but found out that clap has some major problems for this kind of flag.
I saw someone else was also working this problem (you) thats why I am commenting.

I tested your solution too and you have the same errors.

If you run
pr -e testfile

It will consider 'testfile' being an argument for the -e flag. This is obviously not intended. I looked for a while in the clap library and did not encounter a way to enforce clap not allowing the argument for the short -e flag to be seperated by a space.

edit
also malformed parameters to -e flag like the following are parsed but wont throw an error.
./target/debug/pr -esdgjiojiosdgjiogd test_file
it considers 'sdgjiojiosdgjiogd' a parameter to -e and since the .parse() will fail but you dont propagate the errors since you unwrap_or(8) for the width.

[ChrisDryden]

I see, yea this will be bigger than I thought. In this utility and in others we do preparsing of the arguments before passing it to clap so probably in here somewhere we will have to add custom logic to match all of the -e parsing rules:

https://github.com/ChrisDryden/coreutils/blob/049f52a4b96fbc228e5fe5d0a08b752b7935551c/src/uu/pr/src/pr.rs#L403C4-L403C22

[cerdelen]

Yes this might be a bigger problem indeed.

Even trying to parse the free_args in the option builder doesn't seem to fix all the problems as it will throw errors if -e is not declared as an arg to the command builder. Having looked through the Command builder docu it doesnt have the option to disable parameters to be "free" as a next argument seperated by a space.

The parsing of the numbers is somewhat more trivial and manageable (and i think i solved it on my fork) but im not sure how to approach the space problem.

[cerdelen]

You are right you can fix it in that Part of the code you referenced.

I dont wan't to steal your PR so feel free to reference my branch https://github.com/cerdelen/coreutils/tree/pr_e_flag

[ChrisDryden]

No worries, do you want to make a PR? I have a bunch of other ones I need to follow up on, that approach you have seems to work if you can go ahead and make it?

One thing I was thinking of was adding a:

fn prevent_next_arg_consumption(
    arguments: &mut Vec<String>,
    option_regex: &Regex,
    valid_arg_regex: &Regex,
    default_value: &str,
) {
    let arguments_clone = arguments.clone();
    let option_pos = arguments_clone
        .iter()
        .find_position(|x| option_regex.is_match(x.trim()));
    if let Some((pos, _)) = option_pos {
        if let Some(next_arg) = arguments_clone.get(pos + 1) {
            if !next_arg.starts_with('-') && !valid_arg_regex.is_match(next_arg) {
                let could_be_file = arguments.remove(pos + 1);
                arguments.insert(pos + 1, default_value.to_string());
                arguments.insert(pos + 2, could_be_file);
            }
        }
    }
}

Since we're going to have to do this for a few different args and it can be referenced like:

    let e_short_regex = Regex::new(r"^-e\s*$").unwrap();
    let e_long_regex = Regex::new(r"^--expand-tabs$").unwrap();

    prevent_next_arg_consumption(&mut arguments, &e_short_regex, &e_valid_arg_regex, "");
    prevent_next_arg_consumption(&mut arguments, &e_long_regex, &e_valid_arg_regex, "");

[cerdelen]

I would follow this up and also implement the actual behaviour of the expand tabs flag.

Until now i only have a somewhat naive PoW which would still need to have proper variable names etc since for now i just copied the rest of the regex and changed everything to -e.

Anyways i will gladly take this up and continue working on this.