fish completions: fix double-evaluation of commandline #2095
Conversation
|
|
fish_completions.go
Outdated
| @@ -45,7 +45,7 @@ function __%[1]s_perform_completion | |||
| __%[1]s_debug "Starting __%[1]s_perform_completion" | |||
|
|
|||
| # Extract all args except the last one | |||
| set -l args (commandline -opc) | |||
| set -l args (commandline -opc | string escape) | |||
There was a problem hiding this comment.
cc @marckhouzam
FWIW the blame for this mistake is on me, I made the suggestion to use it
|
@krobelus, do you know what needs to be done for this PR to be merged and included in the next release? |
|
The API to avoid this downside while still fixing the issue at hand has been merged in fish-shell/fish-shell#10212, which will be in the upcoming fish release. |
We capture the commandline tokens using fish's "commandline --tokenize" (-o). Given a commandline of some-cobra-command 'some argument $(123)' "$HOME" into three arguments while removing the quotes some-cobra-command some argument $(123) $HOME Later we pass "some argument $(123)" without quotes to the shell's "eval". This is wrong and causes spurious evaluation of the parenthesis as command substitution. Fix this by escaping the arguments. The upcoming fish 3.8 has a new flag, "commandline -x" which will expand variables like $HOME properly, see fish-shell/fish-shell#10212 Use that if available. Reproduce the issue by pasting the completion script at fish-shell/fish-shell#10194 (comment) to a file "grafana-manager.fish" and running function grafana-manager; end source grafana-manager.fish Then type (without pressing Enter) grafana-manager public-dashboards delete --organization-id 3 --dashboard-name "k8s (public)" <TAB> Fixes fish-shell/fish-shell#10194
krobelus
force-pushed
the
fix-fish-completions
branch
from
eabe4e5 to
4e07448
Compare
|
Circling back on this issue - do you think this can be merged? |
|
Thanks for resurrecting this old PR. I'm a bit lost, not only because I'm new to the project. I tried to read the issue, the discussion that happened on it. Then this PR and what was told here too. Then I looked at the fish issue. From my understanding, and I could be totally wrong:
For now, I think I'm good. But then, from what I can see:
Here is where I am. So now, let me ask:
|
|
Hello @ccoVeille , thank you for looking into this. I understand that we still need this PR to address the issue of fish shell completions. The fix in Fish shell was necessary to avoid the undesirable side effects that this PR would introduce if merged as-is. As noted in the PR description:
Now that the upstream fix has been merged in Fish, this PR can be updated to take advantage of the new API—avoiding the side effects while still resolving the original problem. |
|
OK thanks @ichekrygin. It's way clearer now. I might be wrong but based on your comment, I understand the PR shouldn't be merged like this, and requires more work. |
|
I can update it but we'll also need someone with commit access |
We capture the commandline tokens using fish's "commandline --tokenize" (-o).
That function turns
into two arguments while removing the quotes
Later we pass "some argument $(123)" without quotes to the shell's
"eval". This is wrong and causes spurious evaluation of the parenthesis
as command substitution.
Fix this by escaping the arguments.
The downside of this change is that things like "$HOME" or "~" will
no longer be escaped. Changing this requires changes in fish, which
I'm working on.
Reproduce the issue by pasting the completion script at
fish-shell/fish-shell#10194 (comment)
to a file "grafana-manager.fish" and running inside
fishThen type (without pressing Enter)
Fixes fish-shell/fish-shell#10194