Skip to content

echo: fix duplication of -- arguments#7559

Closed
Expertcoderz wants to merge 4 commits intouutils:mainfrom
Expertcoderz:main
Closed

echo: fix duplication of -- arguments#7559
Expertcoderz wants to merge 4 commits intouutils:mainfrom
Expertcoderz:main

Conversation

@Expertcoderz
Copy link

@Expertcoderz Expertcoderz commented Mar 24, 2025

Fixes #7558.

Previously, a workaround for handling double hyphens had the erroneous side effect of echoing -- twice in a row if the -- is the first double hyphen to be passed on the argument list and it is also not the first argument to echo. This commit fixes the aforementioned issue.

@Expertcoderz
echo: fix duplication of -- arguments
fcf00a0 
Fixes #7558.

Previously, a workaround for handling double hyphens had the erroneous side
effect of echoing `--` twice in a row if the `--` is the first double hyphen to
be passed on the argument list and it is also not the first argument to `echo`.
This commit fixes the aforementioned issue.
@cakebaker
Copy link
Contributor

cakebaker commented Mar 24, 2025

Can you please add a test to ensure that we don't regress in the future?

@Expertcoderz
Copy link
Author

Expertcoderz commented Mar 24, 2025

Can you please add a test to ensure that we don't regress in the future?

Sure, working on it. Give me a moment as this is my first time on a Rust project.

@Expertcoderz
test_echo: check for -- arguments between
fb2a276 
This adds a test for passing `--` as a non-first argument to `echo`.

Fixes #7558.
@Expertcoderz
Copy link
Author

Expertcoderz commented Mar 24, 2025

A test has been added. It successfully catches the -- duplication bug prior to this PR's commits.

@github-actions
Copy link

github-actions bot commented Mar 24, 2025

GNU testsuite comparison:

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

@cakebaker
Copy link
Contributor

cakebaker commented Mar 24, 2025

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

It looks like you introduced a new bug ;-)

$ /usr/bin/echo -n -e -- 'foo\n'
-- foo

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

cakebaker
cakebaker reviewed Mar 24, 2025
View reviewed changes
@Expertcoderz
test_echo: Fix spellcheck failure
34ea55c 
Adds a word to the `spell-checker:ignore` to prevent spellcheck from failing
due to said word being introduced in fb2a276.
@Expertcoderz
Copy link
Author

Expertcoderz commented Mar 25, 2025

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

It looks like you introduced a new bug ;-)

$ /usr/bin/echo -n -e -- 'foo\n'
-- foo

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

Whoopsies, I'll have a look at the code and see what other changes must be made.

@github-actions
Copy link

github-actions bot commented Mar 25, 2025

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/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@Expertcoderz Expertcoderz marked this pull request as draft March 26, 2025 14:16
@Expertcoderz Expertcoderz mentioned this pull request Mar 27, 2025
Merged
@Expertcoderz
echo: fix omission of double-hyphen arguments
98252b4 
Fixes a bug introduced by fcf00a0
as mentioned by @cakebaker.

`echo -n -e --` no longer eats the double hyphen.

However, this commit also reduces the double-hyphen duplication fix that
fcf00a0 was supposed to introduce to a partial
fix. The following example will cause double-hyphen duplication:

```sh
$ cargo run -q echo -nonsense --
-nonsense -- --
```

To fix that as well, a more involved workaround may be necessary.
@Expertcoderz
Copy link
Author

Expertcoderz commented Mar 27, 2025

I've pushed a commit that fixes the bug triggered when specifying echo flags. But due to its naïve approach it also limits this PR to a partial fix...

# works:
$ cargo run -q echo -n -e -- hi
-- hi

# works:
$ cargo run -q echo -- hi
-- hi

# works:
$ cargo run -q echo hi --
hi --

# fails:
# (where `-x` is an arbitrary argument that starts with `-` but is not a valid flag)
$ cargo run -q echo -x -- hi
-x -- -- hi

It looks like what we need is either:

  1. A stronger workaround that injects the double-hyphen after option parsing has been done by Clap.
  2. A stronger workaround that takes echo's options (currently, -n, -e and -E) into account.
  3. Parse arguments without relying on Clap, which might be the more efficient solution. (I'm not confident of how to go about doing this, though.)

@github-actions
Copy link

github-actions bot commented Mar 27, 2025

GNU testsuite comparison:

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)

@cerdelen cerdelen mentioned this pull request Mar 27, 2025
Closed
@cakebaker
Copy link
Contributor

cakebaker commented Mar 28, 2025

Closing this PR, superseded by #7581.

@Expertcoderz thanks for your work on this PR!

@cakebaker cakebaker closed this Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cakebaker cakebaker cakebaker left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

echo: -- duplicated when passed as non-first argument

2 participants

@Expertcoderz @cakebaker