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

only add -l when default_shell is bash #320

Closed
wants to merge 2 commits into from

Conversation

tessus
Copy link
Contributor

@tessus tessus commented Dec 5, 2019

@@ -100,7 +100,11 @@ tmux_socket() {
# tmux server each time.
cache_tmux_default_command() {
local default_shell="$(get_tmux_option "default-shell" "")"
export TMUX_DEFAULT_COMMAND="$(get_tmux_option "default-command" "-l $default_shell")"
local opt=""
if [ "`basename $default_shell`"x == "bash"x ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use $(basename $default_shell) instead of backticks? Btw. what's x at the end of the strings used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use $(basename $default_shell) instead of backticks?

Sure.

Btw. what's x at the end of the strings used for?

It's an old habit to avoid test errors (unary operator issues), if one side is empty or undefined.
Theoretically default_shell should never be empty, also the double-quotes should prevent the error as well, but it all depends on the bash version and it doesn't hurt to be on the safe side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want I can remove the x.

@tessus
Copy link
Contributor Author

tessus commented Dec 6, 2019

I suspect you squash the commits anyway. If not, I can create only one commit. Please let me know.

@bruno-
Copy link
Member

bruno- commented Dec 6, 2019

This PR is merged via 1160c1d, I've removed the x suffix to strings.
Thanks

@bruno- bruno- closed this Dec 6, 2019
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.

None yet

2 participants