Skip to content

Fix tmux-plugins/tmux-copycat#121 #122

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

Merged
merged 1 commit into from
Jan 21, 2018
Merged

Conversation

gregorias
Copy link
Contributor

The issue was caused by incorrect parameter expansion.

Copycat saves the old bindings inside
/tmp/copycat_$(whoami)_recover_keys as they were provided on the command
line, e.g.

$ cat /tmp/copycat_$(whoami)_recover_keys | grep clipboard
bind-key -T copy-mode-vi y send-keys -X copy-pipe-and-cancel "xclip -selection clipboard"

When tmux-copycat restores the config it reads each line of that
temporary file into key_cmd and runs tmux $key_cmd. Now, when the
shell expands $key_cmd it expands "xclip, -selection, clipboard"
into separate words. tmux $key_cmd runs effectively as:

tmux bind-key -T copy-mode-vi y send-keys -X copy-pipe-and-cancel "\"xclip" -selection "clipboard\""

Which is not what we want.

In order to fix this bug, this commit makes sure that a proper shell
expansion happens by using sh -c.

The issue was caused by incorrect parameter expansion.

Copycat saves the old bindings inside
/tmp/copycat_$(whoami)_recover_keys as they were provided on the command
line, e.g.

    $ cat /tmp/copycat_$(whoami)_recover_keys | grep clipboard
    bind-key -T copy-mode-vi y send-keys -X copy-pipe-and-cancel "xclip -selection clipboard"

When tmux-copycat restores the config it reads each line of that
temporary file into `key_cmd` and runs `tmux $key_cmd`. Now, when the
shell expands `$key_cmd` it expands `"xclip`, `-selection`, `clipboard"`
into separate words. `tmux $key_cmd` runs effectively as:

    tmux bind-key -T copy-mode-vi y send-keys -X copy-pipe-and-cancel "\"xclip" -selection "clipboard\""

Which is not what we want.

In order to fix this bug, this commit makes sure that a proper shell
expansion happens by using `sh -c`.
@therealpxc
Copy link

this seems to fix the issue on my system :-)

@bruno-
Copy link
Member

bruno- commented Jan 11, 2018

Thank you for fixing this one. I just tried it on my system and it does the job.
Do you know if there's a way to fix this one more elegantly... without invoking the command in a subshell? Can we change the command to have single quotes..?

@gregorias
Copy link
Contributor Author

@bruno- Essentially, you need an interpreter for $key_cmd that will do all the expansions that a POSIX shell does. I'm not familiar with anything that can be more elegant than the shell itself.

Changing sh -c "tmux $key_cmd" to sh -c 'tmux $key_cmd' won't work. sh -c needs to receive the script to run as it's argument, and the script is the value of key_cmd.

@bruno-
Copy link
Member

bruno- commented Jan 12, 2018

@gregorias, what I meant was: can we get xclip -selection clipboard to be inside single quotes instead of double quotes? I'm referring code snippets from the first post in this discussion.

If yes, would that help with proper evaluation.. meaning xclip -selection clipboard would execute as a single command?

@gregorias
Copy link
Contributor Author

@bruno- We could get it into single quotes, I guess, but it would do nothing to alleviate the problem.

If you find my explanation in the first post unclear on why the change of quotes won't help, then
the bash manpage's sections on parameter expansion and word splitting provide more details.

You can also try it out for yourself and think about what the following shell scripts would print:

S='A "B C" D'; for F in $S; do echo $F; done;

S="A 'B C' D"; for F in $S; do echo $F; done;

@bruno- bruno- merged commit bcb40ac into tmux-plugins:master Jan 21, 2018
@oblitum
Copy link
Contributor

oblitum commented Dec 15, 2018

Sadly "tmux $key_cmd" will cause some expansion logic to apply before the string is passed to sh -c, so this commit also doesn't solve the problem. #134 is due to remotion of \ instead of it being escaped as \\.

Expansion issue is actually on the read command.

@oblitum
Copy link
Contributor

oblitum commented Dec 16, 2018

Do you know if there's a way to fix this one more elegantly... without invoking the command in a subshell?

eval "tmux $key_cmd"

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

Successfully merging this pull request may close these issues.

4 participants