Skip to content

Conversation

cerdelen
Copy link
Contributor

Added check to only insert addition double hyphen if at start of arguments to correctly prepend addition hyphens for clap as well as additional test case

@cakebaker cakebaker changed the title Fixes #7558 Fixed double hyphen as argument echo: fixed double hyphen as argument Mar 26, 2025
Copy link

GNU testsuite comparison:

GNU test failed: tests/misc/echo. tests/misc/echo is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

for arg in args {
if arg == "--" && is_first_double_hyphen {
for (i, arg) in args.enumerate() {
if arg == "--" && i == 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment to explain what is it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment

@sylvestre
Copy link
Contributor

GNU test failed: tests/misc/echo. tests/misc/echo is passing on 'main'. Maybe you have to rebase?

seems that it regressed upstream tests

…f at start of arguments to correctly prepend addition hyphens for clap as well as additional test case
@cerdelen
Copy link
Contributor Author

i rebased main and pushed a comment ... on my machine no tests are failing i hope the error was that i hadnt rebased main into my fork before pushing (?)

Copy link

GNU testsuite comparison:

GNU test failed: tests/misc/echo. tests/misc/echo is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@Expertcoderz
Copy link

This seems to have the same bug as my PR (#7559):

$ cargo run -q echo -n -e -- 'foo\n'
foo

The double hyphen gets eaten up if it is given immediately after a flag that is recognized by Clap. Solving this problem could be less straightforward than it seems... :(

@cerdelen cerdelen force-pushed the main branch 2 times, most recently from a357dce to ece6492 Compare March 27, 2025 08:36
@Expertcoderz
Copy link

There's a limitation in your fix in a8d6780:

# doesn't work with combined flags
$ cargo run -q echo -eE -- hi
hi

@cerdelen
Copy link
Contributor Author

cerdelen commented Mar 27, 2025

mhh ... i could modify the is_echo_flag to manually go through the given string and check if index 0 is '-' and then any following char is either 'e' 'E' 'n'

would this be too much of code or acceptable?

i see that with the gnu echo you can even give as an input flags such as '-nneE' which doesnt work as clap will throw an error error: the argument '-n' cannot be used multiple times

should i already limit checks at index 4 t hen as multiple flags are not allowed and will be caught later by clap or ignore that?

@cerdelen
Copy link
Contributor Author

cerdelen commented Mar 27, 2025

i also discovered that echo - -n -- should print -n -- but right now it prints instead - -n --. I am not sure where that error comes from, if that is part of clap or if that is from my implementation ... i will try to investigate a little bit more

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@blyxxyz
Copy link
Contributor

blyxxyz commented Mar 27, 2025

POSIX says to not apply any of the normal options processing and only check if the first arg is -n: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html

GNU is more complicated but I think the same principle applies: we shouldn't use clap to parse at all. Maybe we can use it to generate the help message but we need very simple parsing so we're better off doing it ourselves instead of trying to coerce clap.

mhh ... i could modify the is_echo_flag to manually go through the given string and check if index 0 is '-' and then any following char is either 'e' 'E' 'n'

Like that, and then you can manually set bools as you go so you no longer even need clap.

@Expertcoderz
Copy link

mhh ... i could modify the is_echo_flag to manually go through the given string and check if index 0 is '-' and then any following char is either 'e' 'E' 'n'

would this be too much of code or acceptable?

Won't work with cases such as echo -enonsense which starts with -e but is recognized simply as a regular argument by GNU echo.

i see that with the gnu echo you can even give as an input flags such as '-nneE' which doesnt work as clap will throw an error error: the argument '-n' cannot be used multiple times

This is probably why the best approach is to just not use clap at all for implementing echo. It seems to be complicating things a whole lot.

should i already limit checks at index 4 t hen as multiple flags are not allowed and will be caught later by clap or ignore that?

Yeah, makes sense to do that.

@cerdelen
Copy link
Contributor Author

I would suggest that i fix the problem at hand and removing clap would be a new issue all together?

i found even more cases where flags are printed improperly compared to gnu coreutils

echo - -neE -> -neE
cargo run echo - -neE -> - -neE

echo -gfew -n -- -> -gfew -n --
cargo run -gfew -n -- ->
error: unexpected argument '-g' found

tip: to pass '-g' as a value, use '-- -g'

Usage: cargo run [OPTIONS] [ARGS]...

For more information, try '--help'.

@blyxxyz
Copy link
Contributor

blyxxyz commented Mar 27, 2025

Huh, which version of echo are you on? For me it prints the -:

$ /bin/echo - -neE
- -neE
$ /bin/echo --version
echo (GNU coreutils) 8.32

cargo run -gfew -n --

I think the trick here is to use cargo run -- -gfew -n --. (The first -- is only seen by cargo run.) cargo run's option handling is pretty confusing but if you give it a -- as early as possible you can put whatever afterward.

If in doubt you can also check with target/debug/uu_echo.

I would suggest that i fix the problem at hand and removing clap would be a new issue all together?

That makes sense.

@cakebaker cakebaker merged commit 8c8beb9 into uutils:main Mar 28, 2025
68 checks passed
@cakebaker
Copy link
Contributor

Thanks for your work on this issue, @cerdelen and @Expertcoderz !

nickorlow pushed a commit to nickorlow/coreutils that referenced this pull request Jul 17, 2025
* Fixes uutils#7558 Added check to only insert addition double hyphen if at start of arguments to correctly prepend addition hyphens for clap as well as additional test case

* additional comment

* fixes issue where flags precedes "--" as arguments
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.

5 participants