Skip to content

echo: fix duplication of -- arguments #7559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Expertcoderz wants to merge 4 commits into from
Closed

echo: fix duplication of -- arguments #7559

Expertcoderz wants to merge 4 commits into from

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