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

shellcheck changes broke bash 3.2 and 4.4 completion and zsh '-c' flag. #12

Closed
jshort opened this issue Mar 8, 2018 · 7 comments
Closed

Comments

@jshort
Copy link
Contributor

jshort commented Mar 8, 2018

Hi,

I'm working on #7 and right before pushing my changes to my fork, I pulled the latest changes from the upstream and found that

dae1cbd

Has broken bash 3.2 (default on MacOS), bash 4.4 completion, and the -c flag (because of mapfile) for zsh.

Can we try to keep this code bash/zsh agnostic. The shellcheck gripes do offer options that don't use mapfile (only in bash 4 and above).

As to why completion is broken in bash 4.4, I'm not sure. It seems that it is trying to execute the cmd of the discovered aliases, not present them to the shell.

@iridakos
Copy link
Owner

iridakos commented Mar 8, 2018

Hi @jshort and thanks for reporting!

If I understand correctly, all the problems you described are caused due to the usage of mapfile?

@jshort
Copy link
Contributor Author

jshort commented Mar 8, 2018

Just doing some debugging of the the completion problem in bash 4.4.

It only occurs if you do

goto <TAB>

bash-4.4$ goto bash: wp: command not found
bash: blah: command not found
bash: another: command not found
bash: yo: command not found

however, this works and completes:

goto w<TAB>

iridakos added a commit that referenced this issue Mar 8, 2018
@iridakos
Copy link
Owner

iridakos commented Mar 8, 2018

Can you please pull from upstream and check if the mapfile problems are fixed?

iridakos added a commit that referenced this issue Mar 8, 2018
iridakos added a commit that referenced this issue Mar 8, 2018
@jshort
Copy link
Contributor Author

jshort commented Mar 8, 2018

So that change fixes some aspects of completion for bash 3.2.

However, the case for when you hit without even typing any characters (where you'd expect the completion logic to present you with all of the alias as options) is still not working in both bash 3.2 and 4.4.

Note there is one more mapfile in the cleanup logic. Also ${!array[@]} does not work in zsh to get the indexes. I suppose we can do for i in {0..${#matches}; do ?

@iridakos
Copy link
Owner

iridakos commented Mar 8, 2018

However, the case for when you hit without even typing any characters (where you'd expect the completion logic to present you with all of the alias as options) is still not working in both bash 3.2 and 4.4.

It should be fine now.

Note there is one more mapfile in the cleanup logic.

Fixed that too.

Also ${!array[@]} does not work in zsh to get the indexes. I suppose we can do for i in {0..${#matches}; do ?

That's your call for the time being... I am very newbie to zsh...

iridakos added a commit that referenced this issue Mar 8, 2018
@jshort
Copy link
Contributor Author

jshort commented Mar 8, 2018

Ok completion is fine now. What was the MacOS specific change needed in that last commit. The commit that worked last night (eb01f9c) seemed to have the same logic other than the double quotes.

@iridakos
Copy link
Owner

iridakos commented Mar 8, 2018

The problem was here:

al=$("${matches[$i]// */}")

As you can see, we request execution with the outer $(). So, each aliases deriving from "${matches[$i]// */}" was treated as command.

I'm closing the issue but feel free to report whatever you see.

Thank you very much for the feedback!!!

@iridakos iridakos closed this as completed Mar 8, 2018
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

No branches or pull requests

2 participants