fix: Posix compliant argument parsing for -c mode

[39555]

Before

Brush misses -- because of clap's approach to argument parsing. See: clap-rs/clap/clap_builder/src/parser/parser.rs if arg_os.is_escape() { ...; continue } // means arg_os == '--' and -- is skipped.

But Posix can handle -- as an operand or a command_name just fine.

cargo run -- -c 'echo $0: $1 $2 \"$@\"' --  -1 -2
#> CommandLineArgs.script_path = Some('-1')
#> CommandLineArgs.script_args = ['-2']
#> -1: -2 "-2"

bash -c 'echo $0: $1 $2 \"$@\"' --  -1 -2
#> --: -1 -2 "-1 -2"

cargo run -- -c 'echo $0: $1 $2 \"$@\"'  -1 -2
#> error: unexpected argument '-1' found

bash -c 'echo $0: $1 $2 \"$@\"'  -1 -2
#> -1: -2 "-2"

# cat script.sh
#> echo "$0: $1 $2 \"$@\""

cargo run -- script.sh --  -1 -2
#> CommandLineArgs.script_path = Some('script.sh')
#> CommandLineArgs.script_args = ['-1', '-2']
#> script.sh: -1 -2 "-1 -2"

bash script.sh -- -1 -2
#> script.sh: -- -1 "-- -1 -2"

cargo run -- script.sh -1 -2
#> error: unexpected argument '-1' found

bash script.sh  -1 -2
#> script.sh: -1 -2 "-1 -2"

After This PR

cargo run -- -c 'echo $0: $1 $2 \"$@\"' --  -1 -2
#> --: -1 -2 "-1 -2"

bash -c 'echo $0: $1 $2 \"$@\"' --  -1 -2
#> --: -1 -2 "-1 -2"

cargo run -- -c 'echo $0: $1 $2 \"$@\"'  -1 -2
#> -1: -2 "-2"

bash -c 'echo $0: $1 $2 \"$@\"'  -1 -2
#> -1: -2 "-2"

cargo run -- script.sh --  -1 -2
#> script.sh: -- -1 "-- -1 -2"

bash script.sh -- -1 -2
#> script.sh: -- -1 "-- -1 -2"

cargo run -- script.sh -1 -2
#> script.sh: -1 -2 "-1 -2"

bash script.sh  -1 -2
#> script.sh: -1 -2 "-1 -2"

[39555]

I haven't write integration tests yet. Are test cases compare its output to the bash equivalent?

[github-actions]

Performance Benchmark Report

Benchmark name	Baseline (μs)	Test/PR (μs)	Delta (μs)	Delta %
expand_one_string	3.63 μs	3.63 μs	0.00 μs	⚪ Unchanged
instantiate_shell	60.18 μs	59.91 μs	-0.27 μs	⚪ Unchanged
instantiate_shell_with_init_scripts	25238.69 μs	25321.72 μs	83.03 μs	⚪ Unchanged
parse_bash_completion	5562.19 μs	5596.37 μs	34.18 μs	⚪ Unchanged
parse_sample_script	8.87 μs	8.95 μs	0.09 μs	⚪ Unchanged
run_echo_builtin_command	92.06 μs	93.10 μs	1.04 μs	⚪ Unchanged
run_one_builtin_command	111.85 μs	112.96 μs	1.12 μs	⚪ Unchanged
run_one_external_command	1874.73 μs	1892.53 μs	17.80 μs	🟠 +0.95%
run_one_external_command_directly	948.88 μs	947.31 μs	-1.57 μs	⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
brush-core/src/builtins.rs	🔴 31.21%	🔴 45.81%	🟢 14.6%
brush-core/src/builtins/echo.rs	🟢 81.82%	🟢 86.05%	🟢 4.23%
brush-core/src/namedoptions.rs	🟠 51.92%	🟠 52.4%	🟢 0.48%
brush-core/src/options.rs	🟢 79.17%	🟢 89.58%	🟢 10.41%
brush-core/src/shell.rs	🟢 78.21%	🟢 78.06%	🔴 -0.15%
brush-shell/src/args.rs	🟠 61.9%	🟠 66.67%	🟢 4.77%
brush-shell/src/main.rs	🟢 89.6%	🟢 92.14%	🟢 2.54%
Overall Coverage	🟢 76.31%	🟢 76.51%	🟢 0.2%

Minimum allowed coverage is 70%, this run produced 76.51%

[reubeno]

Thanks for contributing! I'd like to find a few minutes to read through your changes to make sure I understand them, but it looks like a good find.

There's a (very) minimal write-up on the integration tests under docs/reference but the full schema of the .yaml case files still needs to be properly documented. As you called out, the test cases are run under bash and brush with exit code, stderr, and stdout compared. There's also some options for specifying command-line arguments to the shell, ignoring stderr differences, etc. The existing test cases are the best examples.

If the test case schema isn't sufficiently expressive to handle a new case, we can always change the test harness to support more options.

[39555]

I will write test cases today soon

[39555]

Tests for argument handling are ready. I've created a new file arguments.yaml for various tests related to argument handling.

While testing I found another bug/difference with bash. I think it is also related to clap

# bash
echo -- 1
#> -- -1

# brush
echo -- -1
#> -1

[reubeno]

@39555 thanks for adding test coverage! For each new bug we find, I'd very much like us to follow that pattern to ensure the fix doesn't regress.

As for the other case you mention (echo) I'd agree with your assessment. This seems like another instance of what you had to fix with brush's command line, due to clap's special handling of --.

Given that we're now seeing this in multiple places, do you think there's a more general fix or at least some common helper function/code we should extract to solve this problem across built-ins and brush's command line?

[39555]

I found an opened issue in clap clap-rs/clap#5055. Maybe we could resolve it. uutils's echo has the same problem https://github.com/uutils/coreutils/blob/main/src/uu/echo/src/echo.rs

[reubeno]

Good to know that uutils has the same challenge. I like the idea of working to get this supported upstream in clap, thanks for posting there.

In the meantime, I'm supportive of moving forward with a fix for this issue and filing a separate issue on echo and the other built-ins affected (for us to resolve separately).

Regarding the actual code change here, would you please factor out the parsing logic that you've updated into a separate helper function? I think that will at least help to contain the workaround. Thanks!

[39555]

@reubeno To which module can I place such a function? brush_core::builtins?

[reubeno]

@39555 Sorry, I wasn't very clear. For now, I was just thinking of extracting a helper function in the same .rs file to keep main() easier to read. We can always move it somewhere more reusable in a future PR if we'd like to start using it in echo and other built-ins.

[39555]

I meant that I've already tried it in echo and it works perfectly! In the last commit:

• I created two helper functions (for now in brush_core::builtins)
• I overrode Parser::parse_from in CommandLineArgswith the custom parse_from and moved the temporary solution out of main, so we can safely remove it when the issue is resolved and do not affect the code base.
• Similarly in echo, I overrode Command::new so we can just remove it in the future
• Added another tests and some tests for echo

What do you think?

[reubeno]

Ah! Thanks for clarifying and continuing to work on it. I'll look more closely at your latest rev.

[reubeno]

Thanks for making these changes! (And apologies for taking so long to get back to you.) Hopefully we'll be able to remove the workaround in the future if clap-rs adds support for this case...

[reubeno]

Hi @39555 -- are you still willing and interested to help get these changes in?

[39555]

Hi! Sorry! I've rebased onto the main and updated my branch

[reubeno]

Looks like the clippy check error will need to get fixed, but the test failure seems to be related to my most recent changes in main. I will look at disabling the offending test until I can sort out what's going on--thanks for your patience on this.

[39555]

All done

[reubeno]

Thanks for the quick turnaround! Looks like all checks are passing fine. I'll complete the PR shortly.

I plan to create a new release sometime this week and would like to include this fix in it. Thanks again for your contribution 🙂