From 6c626056774d1d6b9e6b6b8000fed0046d15b514 Mon Sep 17 00:00:00 2001 From: omartelo Date: Tue, 28 Jul 2026 17:58:33 -0300 Subject: [PATCH 1/2] fix: give Windows its own font list and link opener Two services reached for Linux binaries on every platform. fonts.List shelled out to fc-list, which Windows has no equivalent of, so the font picker offered only the bundled default and the current selection. Windows now enumerates the Fonts registry key, machine-wide and per-user, folding a family's faces onto one entry. The value-name parsing is pure and sits in the untagged file, so it is tested everywhere; only the registry read is per-OS. OpenExternal hardcoded xdg-open even though the package already had a per-OS seam for opening files. It now goes through that seam. Windows opens a URL with rundll32 rather than the `cmd /c start` used for files: cmd strips metacharacters off its command line before start sees them, so a `&` in a query string would end the command and run what followed it. --- CHANGELOG.md | 8 ++ internal/fonts/fonts.go | 76 +++++++++++++++---- internal/fonts/fonts_test.go | 49 +++++++++++- internal/fonts/list_fontconfig.go | 18 +++++ internal/fonts/list_windows.go | 44 +++++++++++ .../{opendefault_darwin.go => open_darwin.go} | 6 ++ .../{opendefault_linux.go => open_linux.go} | 6 ++ internal/system/open_windows.go | 17 +++++ internal/system/opendefault_windows.go | 9 --- internal/system/system.go | 4 +- internal/system/system_test.go | 27 ++++++- 11 files changed, 237 insertions(+), 27 deletions(-) create mode 100644 internal/fonts/list_fontconfig.go create mode 100644 internal/fonts/list_windows.go rename internal/system/{opendefault_darwin.go => open_darwin.go} (58%) rename internal/system/{opendefault_linux.go => open_linux.go} (51%) create mode 100644 internal/system/open_windows.go delete mode 100644 internal/system/opendefault_windows.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fc20d0f4..942c87e90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Settings › Font lists your installed fonts on Windows.** The picker read the + font list from fontconfig, which Windows does not have, so it offered nothing + beyond the bundled default and whatever was already selected. It now reads the + fonts Windows itself records, folding every weight of a family onto one entry. +- **Links open in your browser on Windows.** Clicking an external link — a pull + request, a release page — called the Linux opener and did nothing at all, with + only a line in `lich.log` to show for it. Each platform now uses its own + opener. - **lich no longer vanishes mid-session on Windows.** Tracking a session's working directory reads it out of the child process's own memory, and a length that memory reported odd — which a 32-bit child, or one exiting mid-read, does diff --git a/internal/fonts/fonts.go b/internal/fonts/fonts.go index a91e6b2e0..c10d9736f 100644 --- a/internal/fonts/fonts.go +++ b/internal/fonts/fonts.go @@ -1,12 +1,11 @@ // Package fonts enumerates the operating system's installed font families so the -// frontend can offer them as terminal fonts. Chromium resolves fonts through -// the same fontconfig database on Linux, so any family listed here is -// renderable by name in the terminal without bundling it. +// frontend can offer them as terminal fonts. Chromium resolves fonts through the +// same database this package reads — fontconfig on Linux and macOS, the Fonts +// registry key on Windows — so any family listed here is renderable by name in +// the terminal without bundling it. package fonts import ( - "fmt" - "os/exec" "sort" "strings" ) @@ -19,15 +18,10 @@ func New() *Service { return &Service{} } -// List returns the sorted, de-duplicated set of installed font families. It -// enumerates via fontconfig (fc-list), covering Linux/macOS; Windows would need -// a registry / DirectWrite path. +// List returns the sorted, de-duplicated set of installed font families, +// enumerated the way this OS records them (see the list_* files). func (s *Service) List() ([]string, error) { - out, err := exec.Command("fc-list", ":", "family").Output() - if err != nil { - return nil, fmt.Errorf("fc-list failed: %w", err) - } - return parseFamilies(string(out)), nil + return listFamilies() } // parseFamilies extracts the sorted, de-duplicated family names from fc-list's @@ -43,7 +37,63 @@ func parseFamilies(out string) []string { } seen[name] = struct{}{} } + return sorted(seen) +} + +// parseRegistryFamilies extracts the sorted, de-duplicated family names from the +// value names under the Windows Fonts registry key. A name reads +// "[ ] ()" — "Arial Bold Italic (TrueType)" — and one +// entry may register several families joined by " & ". +func parseRegistryFamilies(names []string) []string { + seen := make(map[string]struct{}) + for _, name := range names { + for _, part := range strings.Split(stripFontFormat(name), "&") { + family := trimFontFaces(strings.TrimSpace(part)) + if family == "" { + continue + } + seen[family] = struct{}{} + } + } + return sorted(seen) +} + +// stripFontFormat drops the trailing format parenthetical every Fonts registry +// value carries ("(TrueType)", "(OpenType)", "(VGA res)"). A name that opens with +// one keeps it: the parenthetical is a suffix, never the whole name. +func stripFontFormat(name string) string { + if i := strings.LastIndex(name, "("); i > 0 { + return name[:i] + } + return name +} + +// fontFaceWords are the trailing words a registry entry appends to name one +// installed face, so every weight of Arial collapses onto "Arial" in the picker. +// The list is short on purpose: a word belongs here only when no real family ends +// in it, which is why "Black", "Light" and "Semibold" are absent — "Arial Black" +// and "Segoe UI Light" are families in their own right. +// +// Ceiling: a family shipping no regular face is offered under its trimmed name, +// which Chromium may then fail to resolve; the terminal falls back to the +// default font when that happens. +var fontFaceWords = map[string]bool{ + "bold": true, "italic": true, "oblique": true, "regular": true, +} + +// trimFontFaces drops the face words trailing a registry family name. The last +// word is never dropped: a name that is nothing but a face word is a family +// called that. +func trimFontFaces(name string) string { + fields := strings.Fields(name) + for len(fields) > 1 && fontFaceWords[strings.ToLower(fields[len(fields)-1])] { + fields = fields[:len(fields)-1] + } + return strings.Join(fields, " ") +} +// sorted returns the set's members in order. +func sorted(seen map[string]struct{}) []string { families := make([]string, 0, len(seen)) for family := range seen { families = append(families, family) diff --git a/internal/fonts/fonts_test.go b/internal/fonts/fonts_test.go index 57e3a3622..6abf74505 100644 --- a/internal/fonts/fonts_test.go +++ b/internal/fonts/fonts_test.go @@ -3,12 +3,17 @@ package fonts import ( "os/exec" "reflect" + "runtime" "testing" ) func TestListSmoke(t *testing.T) { - if _, err := exec.LookPath("fc-list"); err != nil { - t.Skip("fc-list not installed") + // Windows reads the registry, which is always there; everywhere else the + // enumeration is fc-list, and a machine without fontconfig has nothing to say. + if runtime.GOOS != "windows" { + if _, err := exec.LookPath("fc-list"); err != nil { + t.Skip("fc-list not installed") + } } got, err := New().List() if err != nil { @@ -27,3 +32,43 @@ func TestParseFamilies(t *testing.T) { t.Errorf("parseFamilies = %v, want %v", got, want) } } + +// TestParseRegistryFamilies proves the Windows Fonts key's value names collapse +// onto the family list the picker offers: the format parenthetical goes, faces +// of one family fold together, a multi-family entry splits, and a family whose +// name merely ends in a face-like word survives whole. +func TestParseRegistryFamilies(t *testing.T) { + names := []string{ + "Arial (TrueType)", + "Arial Bold (TrueType)", + "Arial Bold Italic (TrueType)", + "Arial Black (TrueType)", + "Cascadia Mono (TrueType)", + "Segoe UI Light (TrueType)", + "MS Gothic & MS UI Gothic & MS PGothic (TrueType)", + "Courier 10,12,15 (VGA res)", + " ", + "", + } + want := []string{ + "Arial", + "Arial Black", + "Cascadia Mono", + "Courier 10,12,15", + "MS Gothic", + "MS PGothic", + "MS UI Gothic", + "Segoe UI Light", + } + if got := parseRegistryFamilies(names); !reflect.DeepEqual(got, want) { + t.Errorf("parseRegistryFamilies =\n%v\nwant\n%v", got, want) + } +} + +// TestTrimFontFacesKeepsALoneFaceWord proves the trim never empties a name: a +// family called "Bold" is a family, not a face of nothing. +func TestTrimFontFacesKeepsALoneFaceWord(t *testing.T) { + if got := trimFontFaces("Bold"); got != "Bold" { + t.Errorf("trimFontFaces(\"Bold\") = %q, want %q", got, "Bold") + } +} diff --git a/internal/fonts/list_fontconfig.go b/internal/fonts/list_fontconfig.go new file mode 100644 index 000000000..bb65a0bfc --- /dev/null +++ b/internal/fonts/list_fontconfig.go @@ -0,0 +1,18 @@ +//go:build !windows + +package fonts + +import ( + "fmt" + "os/exec" +) + +// listFamilies enumerates installed fonts through fontconfig, the database +// Chromium itself resolves against on Linux and macOS. +func listFamilies() ([]string, error) { + out, err := exec.Command("fc-list", ":", "family").Output() + if err != nil { + return nil, fmt.Errorf("fc-list failed: %w", err) + } + return parseFamilies(string(out)), nil +} diff --git a/internal/fonts/list_windows.go b/internal/fonts/list_windows.go new file mode 100644 index 000000000..2aaf4b242 --- /dev/null +++ b/internal/fonts/list_windows.go @@ -0,0 +1,44 @@ +package fonts + +import ( + "errors" + "fmt" + + "golang.org/x/sys/windows/registry" +) + +// fontsKeyPath is where Windows records every installed font. The value name +// carries the family and face ("Arial Bold (TrueType)"), the data the file. +const fontsKeyPath = `SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts` + +// listFamilies enumerates installed fonts from the registry — Windows has no +// fontconfig. Machine-wide and per-user fonts sit under the same path in +// different hives, and both feed the DirectWrite database Chromium resolves +// against. +func listFamilies() ([]string, error) { + machine, err := fontValueNames(registry.LOCAL_MACHINE) + if err != nil { + return nil, fmt.Errorf("read installed fonts: %w", err) + } + user, err := fontValueNames(registry.CURRENT_USER) + if err != nil { + return nil, fmt.Errorf("read per-user fonts: %w", err) + } + return parseRegistryFamilies(append(machine, user...)), nil +} + +// fontValueNames returns every value name under hive's Fonts key. A user who +// never installed a font for themselves has no key at all, which is an empty +// list rather than a failure. +func fontValueNames(hive registry.Key) ([]string, error) { + key, err := registry.OpenKey(hive, fontsKeyPath, registry.QUERY_VALUE) + if err != nil { + if errors.Is(err, registry.ErrNotExist) { + return nil, nil + } + return nil, err + } + defer key.Close() + // A negative count asks for every value; the key holds one per installed face. + return key.ReadValueNames(-1) +} diff --git a/internal/system/opendefault_darwin.go b/internal/system/open_darwin.go similarity index 58% rename from internal/system/opendefault_darwin.go rename to internal/system/open_darwin.go index 529755c26..ea5b91408 100644 --- a/internal/system/opendefault_darwin.go +++ b/internal/system/open_darwin.go @@ -6,3 +6,9 @@ package system func (s *Service) openDefault(full string) error { return s.run("open", "-t", full) } + +// openURL hands an external URL to the default browser. No -t here: that flag +// forces the text editor, which is right for a source file and wrong for a link. +func (s *Service) openURL(rawURL string) error { + return s.run("open", rawURL) +} diff --git a/internal/system/opendefault_linux.go b/internal/system/open_linux.go similarity index 51% rename from internal/system/opendefault_linux.go rename to internal/system/open_linux.go index 100b95333..490745cc8 100644 --- a/internal/system/opendefault_linux.go +++ b/internal/system/open_linux.go @@ -5,3 +5,9 @@ package system func (s *Service) openDefault(full string) error { return s.run("xdg-open", full) } + +// openURL hands an external URL to the desktop's default browser — the same +// resolver, and the reason a link opens outside the app window. +func (s *Service) openURL(rawURL string) error { + return s.run("xdg-open", rawURL) +} diff --git a/internal/system/open_windows.go b/internal/system/open_windows.go new file mode 100644 index 000000000..b9f5d1a92 --- /dev/null +++ b/internal/system/open_windows.go @@ -0,0 +1,17 @@ +package system + +// openDefault hands the file to the shell's file association when no +// $VISUAL/$EDITOR is set. `start` is a cmd builtin, hence `cmd /c`; the empty +// first argument is start's title slot, which a quoted path would otherwise +// consume. +func (s *Service) openDefault(full string) error { + return s.run("cmd", "/c", "start", "", full) +} + +// openURL hands an external URL to the default browser. Deliberately not the +// `cmd /c start` above: cmd parses metacharacters out of its command line before +// start ever sees them, so a `&` in a query string would end the command and run +// what follows it. rundll32 is launched directly, with no shell in between. +func (s *Service) openURL(rawURL string) error { + return s.run("rundll32", "url.dll,FileProtocolHandler", rawURL) +} diff --git a/internal/system/opendefault_windows.go b/internal/system/opendefault_windows.go deleted file mode 100644 index 22dcc8db2..000000000 --- a/internal/system/opendefault_windows.go +++ /dev/null @@ -1,9 +0,0 @@ -package system - -// openDefault hands the file to the shell's file association when no -// $VISUAL/$EDITOR is set. `start` is a cmd builtin, hence `cmd /c`; the empty -// first argument is start's title slot, which a quoted path would otherwise -// consume. -func (s *Service) openDefault(full string) error { - return s.run("cmd", "/c", "start", "", full) -} diff --git a/internal/system/system.go b/internal/system/system.go index c1679bff4..71e43e517 100644 --- a/internal/system/system.go +++ b/internal/system/system.go @@ -33,12 +33,12 @@ func New(env []string) *Service { // OpenExternal opens an http(s) URL in the default browser. Scheme-gated so a // crafted terminal escape can never turn a click into a file:// or custom -// scheme launch. +// scheme launch. The launcher itself is per-OS (see the open_* files). func (s *Service) OpenExternal(rawURL string) error { if err := ValidateExternalURL(rawURL); err != nil { return err } - return s.run("xdg-open", rawURL) + return s.openURL(rawURL) } // OpenInEditor decides how to open a work-tree file. rel is validated against diff --git a/internal/system/system_test.go b/internal/system/system_test.go index c3664136e..2c9e61a51 100644 --- a/internal/system/system_test.go +++ b/internal/system/system_test.go @@ -27,11 +27,15 @@ func TestValidateExternalURL(t *testing.T) { } } +// TestOpenExternalGatesBeforeLaunching proves the scheme gate runs before +// anything is launched, and that an accepted URL reaches the launcher. The +// opener command is per-OS, so only the URL (always the last argument) is +// portable to assert. func TestOpenExternalGatesBeforeLaunching(t *testing.T) { launched := "" s := &Service{run: func(_ string, args ...string) error { if len(args) > 0 { - launched = args[0] + launched = args[len(args)-1] } return nil }} @@ -43,6 +47,27 @@ func TestOpenExternalGatesBeforeLaunching(t *testing.T) { } } +// TestOpenExternalPassesURLAsOneArgument proves the URL is handed to the +// launcher whole, as a single argument. On Windows that is what keeps a `&` in +// a query string out of a shell's hands — the opener there must not route +// through cmd, whose parser would end the command at the ampersand. +func TestOpenExternalPassesURLAsOneArgument(t *testing.T) { + const raw = "https://example.com/x?a=1&b=2" + var name string + var args []string + s := &Service{run: captureRun(&name, &args)} + + if err := s.OpenExternal(raw); err != nil { + t.Fatalf("OpenExternal: %v", err) + } + if name == "cmd" { + t.Errorf("launcher is %q: a URL must not go through the shell", name) + } + if len(args) == 0 || args[len(args)-1] != raw { + t.Errorf("args = %v, want the whole url %q as the last argument", args, raw) + } +} + // captureRun records the last command the service launched. func captureRun(name *string, args *[]string) func(string, ...string) error { return func(n string, a ...string) error { From 9cb2059e022b2cac8f763c267c71472ba982dd54 Mon Sep 17 00:00:00 2001 From: omartelo Date: Tue, 28 Jul 2026 18:12:55 -0300 Subject: [PATCH 2/2] fix: stop Windows composing shell command lines it cannot quote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more places where Windows got a POSIX assumption, both in OpenInEditor and both reachable from a filename a checked-out branch chose. The default opener ran `cmd /c start`. cmd reads & | < > as command separators and Go quotes an argument only when it holds a space, so opening a file named "a&calc.txt" with no $EDITOR set ran calc. explorer takes the path as a plain argument instead, with no shell in between. The terminal-editor line was quoted with shquote, whose own package doc says it is POSIX-only and is why the worktree setup wrapper skips Windows — this call site did not. cmd.exe does not strip single quotes, so every path with a space (C:\Users\First Last\...) reached the editor cut in half. Quoting is now the session shell's, behind the same per-OS seam as the openers. cmd cannot express a path holding " or %, or one ending in a backslash: the quote cannot be put back, % expands inside quotes, and the backslash escapes the closing quote. Those are refused, and the file opens with the default handler — a line that would run something else is worse than not using the editor. --- CHANGELOG.md | 11 +++++ internal/system/open_darwin.go | 8 ++++ internal/system/open_linux.go | 8 ++++ internal/system/open_windows.go | 17 +++++-- internal/system/system.go | 33 +++++++++++-- internal/system/system_test.go | 83 ++++++++++++++++++++++++++++++++- 6 files changed, 151 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 942c87e90..8e28b6274 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 request, a release page — called the Linux opener and did nothing at all, with only a line in `lich.log` to show for it. Each platform now uses its own opener. +- **Open in editor works on a Windows path with a space in it.** The command + handed to the session was quoted for a Unix shell, which cmd.exe does not + understand, so a file under `C:\Users\First Last\` opened as a path cut at the + space — and a file whose own name held one never opened at all. The quoting is + now the session shell's own. +- **A file cannot open something else on Windows.** Opening a file with no + `$EDITOR` set passed its path through a shell that reads `&` as the end of a + command, so a file named `a&calc.txt` — which any branch you check out is free + to carry — ran `calc` when you opened it. The path is now handed over as a + plain argument, with no shell in between. A path cmd.exe cannot express at all + opens with the default handler rather than being run. - **lich no longer vanishes mid-session on Windows.** Tracking a session's working directory reads it out of the child process's own memory, and a length that memory reported odd — which a 32-bit child, or one exiting mid-read, does diff --git a/internal/system/open_darwin.go b/internal/system/open_darwin.go index ea5b91408..14c9230cd 100644 --- a/internal/system/open_darwin.go +++ b/internal/system/open_darwin.go @@ -1,5 +1,7 @@ package system +import "github.com/omartelo/lich/internal/shquote" + // openDefault opens the file in the default text editor when no $VISUAL/$EDITOR // is set. `open -t` targets the editor bound to the Default Editor, not the // file type's app, so a source file lands in an editor rather than a viewer. @@ -12,3 +14,9 @@ func (s *Service) openDefault(full string) error { func (s *Service) openURL(rawURL string) error { return s.run("open", rawURL) } + +// quoteForShell quotes a path for the POSIX shell a macOS session runs. Every +// path can be expressed, so this never refuses. +func (s *Service) quoteForShell(full string) (string, bool) { + return shquote.Quote(full), true +} diff --git a/internal/system/open_linux.go b/internal/system/open_linux.go index 490745cc8..662bb7dfc 100644 --- a/internal/system/open_linux.go +++ b/internal/system/open_linux.go @@ -1,5 +1,7 @@ package system +import "github.com/omartelo/lich/internal/shquote" + // openDefault hands the file to the desktop's default handler when no // $VISUAL/$EDITOR is set. xdg-open resolves the user's chosen application. func (s *Service) openDefault(full string) error { @@ -11,3 +13,9 @@ func (s *Service) openDefault(full string) error { func (s *Service) openURL(rawURL string) error { return s.run("xdg-open", rawURL) } + +// quoteForShell quotes a path for the POSIX shell a Linux session runs. Every +// path can be expressed, so this never refuses. +func (s *Service) quoteForShell(full string) (string, bool) { + return shquote.Quote(full), true +} diff --git a/internal/system/open_windows.go b/internal/system/open_windows.go index b9f5d1a92..2f48f740b 100644 --- a/internal/system/open_windows.go +++ b/internal/system/open_windows.go @@ -1,11 +1,18 @@ package system -// openDefault hands the file to the shell's file association when no -// $VISUAL/$EDITOR is set. `start` is a cmd builtin, hence `cmd /c`; the empty -// first argument is start's title slot, which a quoted path would otherwise -// consume. +// openDefault hands the file to its shell association when no $VISUAL/$EDITOR +// is set. explorer takes the path as a plain argument, which is the point: the +// `cmd /c start` this replaced handed it to a shell that reads & | < > as +// command separators, and Go quotes an argument only when it holds a space — so +// a file named "a&calc.txt", which a checked-out branch is free to carry, ran +// calc. explorer exits 1 even when it worked; nothing waits on it. func (s *Service) openDefault(full string) error { - return s.run("cmd", "/c", "start", "", full) + return s.run("explorer", full) +} + +// quoteForShell quotes a path for cmd.exe, the shell a Windows session runs. +func (s *Service) quoteForShell(full string) (string, bool) { + return quoteCmdPath(full) } // openURL hands an external URL to the default browser. Deliberately not the diff --git a/internal/system/system.go b/internal/system/system.go index 71e43e517..d1e0d987b 100644 --- a/internal/system/system.go +++ b/internal/system/system.go @@ -11,7 +11,6 @@ import ( "strings" "github.com/omartelo/lich/internal/relpath" - "github.com/omartelo/lich/internal/shquote" ) type Service struct { @@ -63,8 +62,16 @@ func (s *Service) OpenInEditor(dir, rel string) (string, error) { } if editor != "" && isTerminalEditor(editor) { // The command runs in the session's shell, so the file path — the - // caller-influenced part — must survive spaces and metacharacters. - return editor + " " + shquote.Quote(full), nil + // caller-influenced part — must survive spaces and metacharacters. The + // quoting rule is the shell's, and the session's shell is per-OS (see + // the open_* files). A path that shell cannot express at all opens with + // the default handler instead: a line that would run something else is + // worse than not using the editor. + quoted, ok := s.quoteForShell(full) + if !ok { + return "", s.openDefault(full) + } + return editor + " " + quoted, nil } if editor != "" { return "", s.runEditor(editor, full) @@ -104,6 +111,26 @@ func isTerminalEditor(editor string) bool { return terminalEditors[filepath.Base(fields[0])] } +// quoteCmdPath renders full as one argument of a cmd.exe command line, and +// reports false when cmd cannot express it at all. Double quotes cover the +// command separators (& | < >), the grouping parentheses, ^ and spaces — but +// cmd has no escape for three cases, so they are refused rather than mangled: +// +// - a literal " ends the quoted run and nothing puts one back; +// - % is expanded inside quotes too, so a defined variable's name between +// percents silently becomes a different path; +// - a trailing backslash escapes the closing quote for the target's own argv +// parser. filepath.Join cleans that away before we get here, so this is the +// belt to the caller's braces. +// +// Kept out of the build-tagged file so the rule is tested on any OS. +func quoteCmdPath(full string) (string, bool) { + if strings.ContainsAny(full, `"%`) || strings.HasSuffix(full, `\`) { + return "", false + } + return `"` + full + `"`, true +} + // getenv reads a key from the resolved shell env, "" when absent. func (s *Service) getenv(key string) string { prefix := key + "=" diff --git a/internal/system/system_test.go b/internal/system/system_test.go index 2c9e61a51..9c0cb5143 100644 --- a/internal/system/system_test.go +++ b/internal/system/system_test.go @@ -2,6 +2,7 @@ package system import ( "path/filepath" + "runtime" "testing" ) @@ -87,7 +88,14 @@ func TestOpenInEditorTerminalReturnsCommand(t *testing.T) { if err != nil { t.Fatalf("OpenInEditor: %v", err) } - if want := "nvim '" + filepath.Join("/repo", "src/main.go") + "'"; cmd != want { + // The quoting belongs to the session's shell: sh everywhere, cmd.exe on + // Windows, and the two spell a quoted argument differently. + full := filepath.Join("/repo", "src/main.go") + want := "nvim '" + full + "'" + if runtime.GOOS == "windows" { + want = `nvim "` + full + `"` + } + if cmd != want { t.Errorf("cmd = %q, want %q", cmd, want) } if launched { @@ -95,6 +103,79 @@ func TestOpenInEditorTerminalReturnsCommand(t *testing.T) { } } +// TestQuoteCmdPath proves the cmd.exe rule: the separators a shell would act on +// are covered by the quotes, and the three characters cmd cannot express are +// refused outright rather than handed over half-escaped. +func TestQuoteCmdPath(t *testing.T) { + quoted := []string{ + `C:\repo\a.txt`, + `C:\Users\First Last\repo\a.txt`, + `C:\repo\a&calc.txt`, + `C:\repo\a|b>c.txt`, + `C:\repo\a^b(c).txt`, + } + for _, full := range quoted { + got, ok := quoteCmdPath(full) + if !ok || got != `"`+full+`"` { + t.Errorf("quoteCmdPath(%q) = (%q, %v), want the path in quotes", full, got, ok) + } + } + + refused := []string{ + `C:\repo\a"b.txt`, // no way to put a quote back inside a quoted run + `C:\repo\a%PATH%.txt`, // expanded even inside quotes + `C:\repo\dir\`, // would escape the closing quote + } + for _, full := range refused { + if got, ok := quoteCmdPath(full); ok { + t.Errorf("quoteCmdPath(%q) = (%q, true), want refused", full, got) + } + } +} + +// TestOpenInEditorFallsBackWhenTheShellCannotQuote proves an unquotable path +// opens with the default handler instead of being pasted into a shell. Only +// cmd.exe refuses anything, so only Windows has the branch to exercise. +func TestOpenInEditorFallsBackWhenTheShellCannotQuote(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("every path is expressible in a POSIX shell") + } + var name string + var args []string + s := &Service{env: []string{"EDITOR=nvim"}, run: captureRun(&name, &args)} + + cmd, err := s.OpenInEditor(`C:\repo`, `a"b.txt`) + if err != nil { + t.Fatalf("OpenInEditor: %v", err) + } + if cmd != "" { + t.Errorf("cmd = %q, want empty: an unquotable path must not reach a shell", cmd) + } + if want := filepath.Join(`C:\repo`, `a"b.txt`); len(args) == 0 || args[len(args)-1] != want { + t.Errorf("args = %v, want the file %s handed to the default opener", args, want) + } +} + +// TestOpenDefaultKeepsThePathOutOfAShell proves the default opener passes the +// file as its own argument. On Windows that is what stops a checked-out file +// named "a&calc.txt" from ending the command and running calc. +func TestOpenDefaultKeepsThePathOutOfAShell(t *testing.T) { + const rel = "a&calc.txt" + var name string + var args []string + s := &Service{run: captureRun(&name, &args)} + + if _, err := s.OpenInEditor("/repo", rel); err != nil { + t.Fatalf("OpenInEditor: %v", err) + } + if name == "cmd" || name == "sh" || name == "bash" { + t.Errorf("launcher is %q: the default opener must not route through a shell", name) + } + if want := filepath.Join("/repo", rel); len(args) == 0 || args[len(args)-1] != want { + t.Errorf("args = %v, want the whole path %q as one argument", args, want) + } +} + func TestOpenInEditorPrefersVisual(t *testing.T) { // VISUAL wins over EDITOR: a terminal EDITOR is ignored when VISUAL is GUI. var name string