Windows compatibility improvements#2418
Conversation
Mirrors Linux behavior where files starting with '.' are hidden
by default via gDefaultHiddenFiles = []string{".*"}.
When the shell is set to sh, bash, pwsh, or any other non-cmd shell on Windows, the default shellflag /c is invalid. Unix-style shells and PowerShell both require -c (short for -Command in PowerShell, standard flag in POSIX shells). If the user has explicitly set shellflag to something other than /c, that value is respected as-is. Only the cmd.exe default /c is replaced.
On Windows, shell scripts (e.g. sh/bash scripts with a shebang) cannot be executed directly by the OS. When the previewer has no native Windows extension (.exe, .cmd, .bat), lf now runs it via the configured shell (e.g. `sh preview path w h x y mode`) instead of exec-ing it directly. Native executables are unaffected.
…dows sh (Git Bash/MSYS2) cannot resolve Windows-style backslash paths when used as a script filename argument. Convert backslashes to forward slashes before passing the previewer path to sh.
Previously any file without a native extension (.exe, .cmd, .bat) was run through sh. Now we peek at the first two bytes to check for a #! shebang, so native binaries without extensions are executed directly.
Rename previewCommand to scriptCommand and apply it to both the previewer and cleaner calls in nav.go. Both are user-configured scripts that may have a shebang and need the same treatment.
Replace the broad "not cmd" check with an explicit allowlist of POSIX-compatible shells (sh, bash, zsh, dash). This prevents pwsh and other non-POSIX shells from being used to run shebang scripts.
vivainio
changed the title
CatsDeservePets
left a comment
CatsDeservePets
left a comment
There was a problem hiding this comment.
[See my other comments]
| if ext == ".exe" || ext == ".cmd" || ext == ".bat" { | ||
| return exec.Command(path, args...) | ||
| } | ||
| // Run scripts with a shebang through POSIX-compatible shells only. | ||
| // Convert backslashes to forward slashes so sh can resolve the path. | ||
| shellBase := strings.ToLower(strings.TrimSuffix(filepath.Base(gOpts.shell), ".exe")) | ||
| posixShells := map[string]bool{"sh": true, "bash": true, "zsh": true, "dash": true} | ||
| if posixShells[shellBase] && hasShebang(path) { | ||
| path = filepath.ToSlash(path) | ||
| return exec.Command(gOpts.shell, append([]string{path}, args...)...) | ||
| } | ||
| return exec.Command(path, args...) |
There was a problem hiding this comment.
Hardcoded checks like this will never satisfy every usage-case.
This one in particular is really dangerous and incorrect. You are just testing whether the file has a shebang but execute it in shell anyway. What if your previewer is a python script? What if your shell is set to sh but the previewer is using bash syntax?
| if strings.HasPrefix(f.Name(), ".") { | ||
| return true | ||
| } |
There was a problem hiding this comment.
I don't like this for 2 reasons:
- Coming from
Unixland myself (and much preferring it overWindows), I fully get why this behaviour might be desired (especially when using things likeGitBashorWSL). However, this is not the general mindset of Windows user. Hence, the-Hiddenand-Forceparameters ofDir/Get-ChildItemdon't care for dots in filenames at all, only thehiddenattribute. - If we were to change this behaviour, this would not be the right place at all. If you take a look at
Unixvariant ofos.go,isHiddendoes not have this logic either. Instead, we have this this initialisation at the very top of the file:
gDefaultHiddenFiles = []string{".*"}I am not sure if you are aware of the hiddenfiles setting.
| shellflag := gOpts.shellflag | ||
| if shellflag == "/c" { | ||
| shellflag = "-c" | ||
| } | ||
| args = append([]string{shellflag, s}, args...) |
There was a problem hiding this comment.
@joelim-work convinced me in #2132 (review), why implicit shenanigans like this are not a good idea. In there, I wanted to automatically update key bindings to PowerShell syntax.
| // Set config directory for relative path resolution | ||
| savedConfigDir := gConfigDir | ||
| gConfigDir = filepath.Dir(path) | ||
| defer func() { gConfigDir = savedConfigDir }() | ||
|
|
There was a problem hiding this comment.
In general, I am not opposed to relative path resolution for ruler, previewer, and cleaner. However, this is not the right approach. Temporarily modifying gConfigDir might lead to a race condition. Also, being able to successfully source files after showing an error message would just lead to confusion.
The path itself should be corrected before even passing it to readFile.
| // expandPath expands env vars, tilde, and resolves relative paths against config directory | ||
| func expandPath(s string) string { | ||
| s = os.ExpandEnv(s) | ||
| s = replaceTilde(s) | ||
|
|
||
| if !filepath.IsAbs(s) && gConfigDir != "" { | ||
| s = filepath.Join(gConfigDir, s) | ||
| } | ||
|
|
||
| return s | ||
| } | ||
|
|
There was a problem hiding this comment.
This can lead to confusing behaviour as os.ExpandEnv will remove references it cannot find:
a := []string{"foobar", "foo$bar", "$foobar"}
for _, v := range a {
fmt.Printf("%q\n", os.ExpandEnv(v))
}Output:
"foobar"
"foo"
""
Also, support for env vars is something that has been discussed for quite some time now, but is more complicated as it first seems:
| case "source": | ||
| if len(e.args) != 1 { | ||
| app.ui.echoerr("source: requires an argument") | ||
| return | ||
| } | ||
| app.readFile(replaceTilde(e.args[0])) | ||
| app.readFile(expandPath(e.args[0])) |
There was a problem hiding this comment.
I don't think the source command should ever use the config dir as a fallback. Invoking it is something that is done explicitly by the user. A user expects it to resolve paths relative to the CWD.
|
Hello @vivainio! |
Hey, it's fine to close it. I switch much less between windows and wsl right now, I'll revisit the issue when I have to use native Windows side a bit more |
2 participants
Summary
This PR improves lf usability on Windows, particularly when using a POSIX shell (sh/bash via Git Bash or similar) instead of cmd. Together these changes make it practical to share a single lf config file between Windows and Linux, without needing separate platform-specific config files.
Changes
Shell flag auto-correction (
os_windows.go)When
shellis set to a non-cmd shell (e.g.sh,bash,pwsh), the default shellflag/cis invalid. lf now automatically uses-cfor the non-cmd code path inshellCommand.Dot file hiding (
os_windows.go)On Windows, toggling hidden files (
zh) now also hides files starting with.(e.g..gitignore,.env), matching Linux behavior. Previously only the WindowsFILE_ATTRIBUTE_HIDDENattribute was checked.Shebang-aware script execution (
os_windows.go,os.go,nav.go)When
previewerorcleaneris set to a shell script (e.g. a#!/bin/shscript), lf on Windows now detects the shebang and automatically runs the script through the configured shell instead of trying to execute it directly (which fails since Windows cannot run shebang scripts natively). This only applies when the shell is a known POSIX shell (sh,bash,zsh,dash) — cmd and pwsh are excluded. Path backslashes are converted to forward slashes for sh compatibility.Relative path and env var expansion (
misc.go,app.go,eval.go)set previewer,set cleaner,set rulerfile, andsourcenow support:$VAR)~)This allows configs like
set previewer preview(resolves to~/.config/lf/previewor%APPDATA%\lf\preview) without needing absolute paths.Motivation
A common pain point for users who work on both Windows and Linux is maintaining two separate lf config files. With these changes, a single
lfrcusingset shell shand a shared previewer script works on both platforms without any platform-specific wrappers or path gymnastics.Test plan
set shell sh: verify shellflag is-cand shell commands work.gitignoreetc.) are hidden whenhiddenis off.cmdwrapperset previewer previewresolves correctly on both Linux and Windows