fix(windows): set console title via SetConsoleTitleW instead of shelling out to cmd /c title#5850
Open
myUdav4iik wants to merge 1 commit into
Open
Conversation
…ing out to `cmd /c title` Fixes jesseduffield#5766. UpdateWindowTitle built a `title <repo-name> - Lazygit` string and ran it through `cmd /c "title ..."`. When the current directory's basename contains a cmd.exe metacharacter such as `&`, cmd.exe treats it as a command separator: "test&aaa" gets split into `title test` and `aaa`, and cmd.exe then tries to execute `aaa` as a program. That fails with: 'aaa' n'est pas reconnu en tant que commande interne ou externe... which lazygit's error handling surfaced as an uncaught error, crashing the whole run (and, per the second half of the report, closing the parent shell). This reproduces both ways described in the issue: launching lazygit directly inside a folder with '&' in its name, and opening such a repo from the recent-repos menu. Fix UpdateWindowTitle now calls the Win32 SetConsoleTitleW API directly via syscall.NewLazyDLL/kernel32.dll (the same mechanism pkg/gocui/gui_windows.go already uses for GetConsoleScreenBufferInfo), instead of building a shell command string and handing it to cmd.exe. This sidesteps cmd.exe's argument parsing entirely — the title is passed to the OS as a UTF-16 string, so it's set verbatim regardless of what characters the directory name contains. No shell, no quoting rules, no injection surface. Testing go build ./... GOOS=windows GOARCH=amd64 go build ./... GOOS=windows GOARCH=amd64 go vet ./pkg/commands/oscommands/... GOOS=windows GOARCH=amd64 go test -c ./pkg/commands/oscommands/ # compiles go test ./pkg/commands/oscommands/... -v # passes on darwin go test <all non-integration packages> -count=1 # all green I don't have a Windows machine to run the test binary on directly, but I cross-compiled the package (and its test binary) for windows/amd64 to confirm it builds and type-checks cleanly, and the two new tests below will execute for real on the windows-latest CI runner this repo already uses. New tests in pkg/commands/oscommands/os_windows_test.go (windows-only, build-tagged, same as the existing file): - TestUpdateWindowTitle_NameWithAmpersand: chdirs into a directory named "test&aaa" (the exact string from the report) and asserts UpdateWindowTitle no longer errors, then reads the title back with GetConsoleTitleW and asserts it's the literal, unsplit directory name - reproducing jesseduffield#5766 and proving it's fixed. - TestUpdateWindowTitle_PlainName: sanity check that the ordinary case (no special characters) still produces the expected "<name> - Lazygit" title. Both tests skip gracefully (rather than fail) if GetConsoleTitleW is unavailable in the CI environment (e.g. no attached console), so they can't produce a false failure unrelated to this fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What / Why
Fixes #5766: lazygit crashes when the current folder's name contains
&(e.g.Rapports&Notes,test&aaa), whether launched directly in such a folder or opened via the recent-repos menu.UpdateWindowTitlebuilt atitle <repo-name> - Lazygitstring and ran it throughcmd /c "title ...".cmd.exetreats&as a command separator, so"test&aaa"gets split into two commands:title testandaaa.cmd.exethen tries to executeaaaas a program, fails, and that failure propagates up as an uncaught error that crashes the whole run (matching the exact error message and stack trace in the report).Fix
UpdateWindowTitlenow calls the Win32SetConsoleTitleWAPI directly viasyscall.NewLazyDLL("kernel32.dll")- the same mechanismpkg/gocui/gui_windows.goalready uses forGetConsoleScreenBufferInfo- instead of building a shell command string and handing it tocmd.exe. This sidestepscmd.exe's argument parsing entirely: the title is passed to the OS as a UTF-16 string and set verbatim, regardless of what characters the directory name contains. No shell, no quoting rules, no injection surface for this code path.Testing
I don't have a Windows machine to execute the test binary on directly, but I cross-compiled the package and its test binary for
windows/amd64to confirm it builds and type-checks cleanly, and the CI matrix already includeswindows-latest, so the two new tests below will run for real there.New tests in
pkg/commands/oscommands/os_windows_test.go(windows-only, build-tagged, same file the existingTestOSCommandOpenFileWindowslives in):TestUpdateWindowTitle_NameWithAmpersand- chdirs into a directory literally namedtest&aaa(the exact string from the report), assertsUpdateWindowTitleno longer errors, then reads the title back withGetConsoleTitleWand asserts it's the unsplit, literal directory name. This reproduces Crashes when folder/repo has a "&" in the name #5766 and proves it's fixed.TestUpdateWindowTitle_PlainName- sanity check that the ordinary case (no special characters) still produces"<name> - Lazygit".Both new tests skip gracefully rather than fail if
GetConsoleTitleWis unavailable in the CI sandbox (e.g. no attached console), so they can't produce a false failure unrelated to this change.Fixes #5766