Skip to content
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.

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

cmd/gosh: echo * in interactive mode requires further input #835

Closed
theclapp opened this issue Mar 30, 2022 · 5 comments · Fixed by #895
Closed

cmd/gosh: echo * in interactive mode requires further input #835

theclapp opened this issue Mar 30, 2022 · 5 comments · Fixed by #895
Labels

Comments

@theclapp
Copy link
Collaborator

Running echo * in gosh in interactive mode hangs forever until you press enter (or ^D):

% ./gosh
$ pwd
/Users/lmc/src/goget/src/github.com/mvdan/sh/cmd/gosh
$ echo *
<hangs>
<press ^D>
gosh main.go main_test.go
$ % <gosh prints its prompt and exits without further input after ^D>

Running echo ** requires pressing enter twice:

% ./gosh
$ shopt
expand_aliases	off
globstar	off
nullglob	off
$ echo *
<hangs; press enter>

gosh main.go main_test.go
$ $ echo **
<hangs; press enter twice>


gosh main.go main_test.go
$ $ ls -l
total 3456
-rwxr-xr-x 1 lmc staff 3523536 Mar 30 09:22 gosh
-rw-r--r-- 1 lmc staff    1880 Feb 21  2020 main.go
-rw-r--r-- 1 lmc staff    4201 Feb  3 09:45 main_test.go
$ cd ..
$ echo *
<hangs; press enter>

gosh shfmt
$ $ echo **
<hangs; press enter twice>


gosh shfmt
$ $ shopt -s globstar
$ echo **
<hangs; press enter twice>


gosh gosh/gosh gosh/main.go gosh/main_test.go shfmt shfmt/Dockerfile shfmt/docker-entrypoint.sh shfmt/json.go shfmt/main.go shfmt/main_test.go shfmt/shfmt.1.scd shfmt/testdata shfmt/testdata/scripts shfmt/testdata/scripts/atomic.txt shfmt/testdata/scripts/basic.txt shfmt/testdata/scripts/diff.txt shfmt/testdata/scripts/editorconfig.txt shfmt/testdata/scripts/flags.txt shfmt/testdata/scripts/tojson.txt shfmt/testdata/scripts/walk.txt
$ $

Weird.

Originally posted by @theclapp in #834 (comment)

@mvdan mvdan added the bug label Mar 30, 2022
@prologic
Copy link

prologic commented Jul 3, 2022

Hey @mvdan can we get this fixed? I've run into this as well with my experimental Go Linux(ish) 😅

@mvdan mvdan changed the title gosh: echo * in interactive mode requires further input cmd/gosh: echo * in interactive mode requires further input Jul 3, 2022
mvdan added a commit that referenced this issue Jul 3, 2022
The lexer wants to know if literal characters like `*` or `@` are
followed by `(`, because then they would be an extended globbing
expression like `@(pattern-list)`.

However, the current implementation first peeked for the bytes `()`,
to detect function declarations like `@() { ... }`.
Unfortunately, this caused interactive shells to hang on `echo *`
followed by a newline, as a newline is a single character.

To work around the problem, only peek `()` if we first peek `(`.

Moreover, the logic to call Parser.fill in Parser.peekBytes seems wrong.
The conditional definitely needs to use len(s), as that is the number of
bytes we want to look for. The new logic seems clearly better; we call
Parser.fill when p.bs contains fewer remaining bytes than len(s).

Note that we no longer loop on that logic, because we have zero tests
exercising that edge case, and I am slightly worried about endless loops
until we properly test those edge cases.

Fixes #835.
@mvdan
Copy link
Owner

mvdan commented Jul 3, 2022

Took a brief look, wasn't as tricky to fix as I initially thought. It looks like this was a somewhat recent regression, from #771.

mvdan added a commit that referenced this issue Jul 3, 2022
The lexer wants to know if literal characters like `*` or `@` are
followed by `(`, because then they would be an extended globbing
expression like `@(pattern-list)`.

However, the current implementation first peeked for the bytes `()`,
to detect function declarations like `@() { ... }`.
Unfortunately, this caused interactive shells to hang on `echo *`
followed by a newline, as a newline is a single character.

To work around the problem, only peek `()` if we first peek `(`.

Moreover, the logic to call Parser.fill in Parser.peekBytes seems wrong.
The conditional definitely needs to use len(s), as that is the number of
bytes we want to look for. The new logic seems clearly better; we call
Parser.fill when p.bs contains fewer remaining bytes than len(s).

Note that we no longer loop on that logic, because we have zero tests
exercising that edge case, and I am slightly worried about endless loops
until we properly test those edge cases.

Fixes #835.
@mvdan mvdan closed this as completed in #895 Jul 3, 2022
mvdan added a commit that referenced this issue Jul 3, 2022
The lexer wants to know if literal characters like `*` or `@` are
followed by `(`, because then they would be an extended globbing
expression like `@(pattern-list)`.

However, the current implementation first peeked for the bytes `()`,
to detect function declarations like `@() { ... }`.
Unfortunately, this caused interactive shells to hang on `echo *`
followed by a newline, as a newline is a single character.

To work around the problem, only peek `()` if we first peek `(`.

Moreover, the logic to call Parser.fill in Parser.peekBytes seems wrong.
The conditional definitely needs to use len(s), as that is the number of
bytes we want to look for. The new logic seems clearly better; we call
Parser.fill when p.bs contains fewer remaining bytes than len(s).

Note that we no longer loop on that logic, because we have zero tests
exercising that edge case, and I am slightly worried about endless loops
until we properly test those edge cases.

Fixes #835.
@prologic
Copy link

prologic commented Jul 3, 2022

I can confirm your patch fixed this bug. But now I'm wondering, does this library support globbing on the shell? It doens't seem to 🤔

@mvdan
Copy link
Owner

mvdan commented Jul 3, 2022

It certainly should. The test I added shows it, since it matches main.go and main_test.go.

@prologic
Copy link

prologic commented Jul 4, 2022

It certainly should. The test I added shows it, since it matches main.go and main_test.go.

Yeah I saw. Don't worry it's likely a bug in my implementation of ls 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants