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

[Feature Request] An option to set SHELL for preview command on Windows #2638

Closed
4 of 10 tasks
rashil2000 opened this issue Oct 17, 2021 · 6 comments · Fixed by #2641
Closed
4 of 10 tasks

[Feature Request] An option to set SHELL for preview command on Windows #2638

rashil2000 opened this issue Oct 17, 2021 · 6 comments · Fixed by #2641
Labels

Comments

@rashil2000
Copy link
Contributor

rashil2000 commented Oct 17, 2021

  • I have read through the manual page (man fzf)
  • I have the latest version of fzf
  • I have searched through the existing issues

Info

  • OS
    • Linux
    • Mac OS X
    • Windows
    • Etc.
  • Shell
    • bash
    • zsh
    • fish

Problem / Steps to reproduce

Most of the interesting and useful tooling around fzf is built around POSIXy shells such as bash and zsh. I can think of three examples (I'm sure there are thousands others) on the top of my head:

Bash and zsh shells are readily available on Windows (Git Bash, MSYS2, cygwin), so the shell is not really an issue. The problem arises with preview commands in these tools, which always assume bash-like shells. So the preview in these tools does not work on Windows because fzf unconditionally uses cmd for the preview command, and doesn't respect SHELL (like it does on nix platforms).

Proposed solutions

  1. When using MSYS2/Cygwin, the SHELL environment variable is set by them. Fzf should respect that, or optionally have a configuration to respect that.
  2. Another solution is to implement an environment variable FZF_PREVIEW_SHELL that only applies to Windows platform, and this can be used to inform fzf about the interpreter (or its path). If the interpreter (or its path) is not found, fallback to using cmd.

Would love to hear more thoughts on this. The lack of preview support extremely limits the amount of fzf tools that can be used in MSYS2/Cygwin.

@rashil2000
Copy link
Contributor Author

@junegunn I'm interested in working on this if it seems to be a reasonable addition.

@junegunn
Copy link
Owner

Solution 1 sounds good; $SHELL -c {command} if $SHELL is defined, or cmd /v:on/s/c {command} as before.
No need to introduce another variable if there's no risk of name collision. (I presume $SHELL is not used on Windows environment in general)

@rashil2000
Copy link
Contributor Author

rashil2000 commented Oct 18, 2021

You're right, SHELL is not set/used by Windows outside of Cygwin/Msys2. But solution 1 presents a problem: the SHELL variable would be set to a unix path, something like /usr/bin/bash or /bin/bash, which are paths that only Cygwin/Msys2 will understand. Go will error out on finding these paths on Windows.
This is why I had to think of solution 2.

(note that there's a builtin utility called cygpath available inside Cygwin/Msys2, which can be used to convert unix/windows paths to and fro. If we really have to go with solution 1, we will have to use this utility to convert the interpreter's path before passing it to Go.)

@vovcacik
Copy link
Contributor

@junegunn To illustrate the above problem:

git bash
cmd> git-bash.exe
$ echo $SHELL
/usr/bin/bash
$ ./echo-foobar
foobar
echo-foobar.go
package main

import (
	"os/exec"
)

func main() {
	// windows path is required
	cmd := exec.Command(`E:\Git\usr\bin\bash.exe`, "-c", "echo foobar")
	cmd.Run()
}

@vovcacik
Copy link
Contributor

vovcacik commented Oct 19, 2021

@rashil2000 I might be running old version of WSL, but I am getting fork/exec /bin/bash: invalid argument when I cmd.Run() this cmd := exec.Command("/bin/bash", "-c", "echo foobar").

@rashil2000
Copy link
Contributor Author

Hi @vovcacik, I'm not sure what you mean, this issue is not related to WSL at all.

@junegunn I've implemented solution 1 and raised a PR #2641

junegunn pushed a commit that referenced this issue Oct 22, 2021
This makes fzf respect SHELL environment variable on Windows, like it does on *nix, whenever defined.

Close #2638
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