It existed for the incremental re-render: a shorter render left the previous
one's view lines in the tail (deliberately, to avoid a blank frame), and this
cleared them once the new content was fully read. Async renders now build
off-screen and swap in whole, so refreshViewLinesIfNeeded truncates the view
lines to the buffer and no tail can form. All the call at end-of-input still
did was discard every wrapped line and force the whole buffer to be re-wrapped
on the next draw, which is pure work on a large diff.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PipeCommands ran every command in its own goroutine, each doing
Start/read-stderr/Wait, with nothing ordering one goroutine's Start
against another's Wait. That ordering matters: StdoutPipe registers the
parent's read end in cmd.parentIOPipes, and Cmd.Wait closes those
descriptors when it returns. The next command's Stdin is that very
*os.File, and exec passes a user-supplied *os.File through untouched, so
Start hands the child whatever the fd happens to be at that moment. If
the producer finished and got reaped before the consumer's goroutine
reached Start, that fd was already closed, File.Fd() returned -1, and
the child was started with fd 0 closed -- reading nothing at all.
The only caller is the pre-2.35 fallback in SaveStagedChanges, which
pipes `git stash show -p` into `git apply -R`. Losing that race left
git apply with an empty patch, so it failed with "unrecognized input",
the following `git stash drop` never ran, and the user was left with a
stray stash entry. This turned up as a flaky stash/stash_staged on the
git 2.32.0 CI job; the newer-git jobs take the `git stash push --staged`
path and never reach this code.
Starting every command up front removes the race, and collecting stderr
into buffers lets exec's own copying goroutines do the work. That also
fixes two lesser problems in the same function: finalErrors was appended
to from several goroutines without synchronization, and a failed Start
was only logged, so a pipeline that never ran reported success.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pty teardown in Close runs on a background goroutine that doesn't
get to finish when lazygit is quitting: the process exits milliseconds
after the view buffer managers are closed. The job objects still cover
the clients -- KILL_ON_JOB_CLOSE reaps them when the process's handles
are rundown at exit -- but nothing reaps the conhost, so on Windows
builds whose conhost fails to run down on its own, quitting leaks one
conhost per live pty.
This is not a rare timing window: a diff longer than what has been
read keeps its git process (and thus its pty and conhost) running for
the entire time it is displayed, so that scrolling can read more.
Quitting while looking at a long diff is therefore the common case,
and with an external differ configured it leaks a conhost on affected
builds on almost every quit.
Fix this by having the gui's shutdown path wait synchronously for the
in-flight teardowns after closing the view buffer managers. A quit
signal makes the teardowns skip the conhost rundown wait -- the
conhost serves nothing once its clients are dead, and the exit must
not stall for its sake -- so the wait normally completes in
milliseconds, keeping quit as fast as before; a 2-second cap protects
the exit path even if a teardown wedges.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A pseudoconsole's conhost.exe is spawned by CreatePseudoConsole as a
child of lazygit, so it is not part of the job object that the pty
teardown kills. That is normally fine: a healthy conhost runs itself
down once the reference handle is closed and its clients are gone. But
conhost builds before the ConPTY overhaul that shipped with Windows 11
24H2 (confirmed on 23H2, build 22631) fail to complete the rundown
when a client attached after the close event was delivered and was
then killed -- the fate of exactly the clients the job kill exists for
-- and such a conhost lingers forever with no clients, at a rate of
about one per five fast commit navigations. These builds remain
widespread: all of Windows 10 (whose ESU tail runs into 2028, and
whose hardware often cannot run Windows 11 at all) plus pre-24H2
Windows 11 fleets.
Since Windows offers no way to obtain the conhost's pid or handle from
the HPCON, identify it by diffing lazygit's direct conhost children
around the CreatePseudoConsole call, serialized by a mutex so that two
concurrently starting ptys can't confuse each other's diff, and open a
handle immediately so that pid reuse is harmless. The teardown then
gives conhost a second to exit on its own before terminating it; on
healthy builds the wait succeeds and the reap never fires. If the
conhost can't be identified unambiguously, we simply don't reap, which
is no worse than before.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Stopping a pty task on Windows relies on ClosePseudoConsole, which
delivers CTRL_CLOSE_EVENT to the console's attached clients. But only
to those attached at that moment: when the user flicks quickly through
commits, a task is often stopped within the first few milliseconds of
its life, before the child has attached to the pseudoconsole. Such a
child misses the event and survives, running the entire diff to
completion in the background (spawning one external differ per changed
file) and keeping its conhost.exe alive; rapid navigation accumulates
these git/difft/conhost trees, and they outlive lazygit. Grandchildren
are affected too: git for Windows runs commands through a two-level
git.exe wrapper, so a single task has several attach windows, and a
grandchild spawned while the console is going down is orphaned even
when its parent got the event and exited.
Fix this by putting the child into a job object before it runs its
first instruction (created suspended, assigned, then resumed), so that
every descendant is in the job from the start; the teardown in Close
terminates the job right after initiating the pseudoconsole close.
There is no point in a grace period between the two: the close event
is not a graceful signal -- git and the common diff tools leave it to
the default handler, which calls ExitProcess at an arbitrary point --
so clients that received it are already dying, and the kill exists for
those that missed it. Killing at an arbitrary point cannot leak a
stale index.lock, because pty-rendered commands no longer take that
lock (see withPtyGitConfig in pkg/gui/pty.go).
The pseudoconsole close runs on its own goroutine because the kill
must not wait for it: on builds where ClosePseudoConsole blocks until
the console host exits (pre-24H2), the host keeps running as long as a
surviving client does, and that client only goes away through the job
kill; sequencing the kill after a blocking close would deadlock in
exactly the case the kill exists for.
KILL_ON_JOB_CLOSE doubles as a safety net: if lazygit exits without
running the teardown, the OS closes the job handle and reaps the tree.
In a harness that mimicked the stop path with randomized 0-120ms stop
delays, 3 of 30 process trees survived as orphans before this change;
none survive with it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
For a long time lazygit has used the term "custom pager" to refer to
what's really a "diff renderer". A pager is a program that allows you to
view output page by page (hence the name), e.g. less; lazygit's custom
diff renderers are not pagers. It used the term only because the feature
is implemented using git's GIT_PAGER env var, but that's an
implementation detail.
Rename the 'git.pagers' config to 'git.diffRenderers', and restructure
its elements while we're at it to make things clearer:
- Add a 'type' field to explicitly specify which type of diff renderer
it is (the two fundamentally different ones are 'stdinFilter' and
'extDiff').
- Add a third type, 'rawGit', which has an 'args' field that makes it
easy to use 'git --color-words' as a custom renderer
- Unify the old 'pager' and 'externalDiffCommand' fields to a single
'command' field for both types
Existing config files are migrated automatically.
CreatePseudoConsole and ResizePseudoConsole reject zero dimensions with
E_INVALIDARG, but we legitimately request them: the pty is sized after
the main view, and that view is zero-sized while hidden, e.g. in
full-screen mode with a side panel focused. Entering that mode while a
custom pager is configured therefore made StartPty fail (degrading to
unpaged output now that the fallback works), and resizing a live pty
from onResize would fail layout. The Unix pty accepts zero sizes, so
the clamp lives in the Windows implementation only.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CreatePseudoConsole rejects zero dimensions with E_INVALIDARG, so
starting a pty sized after a hidden (and thus zero-sized) view fails.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
runAndStreamAux reads the stdout buffer (and, when output is suppressed,
the combinedOutput buffer) for its error message after handler.wait()
returns, but the goroutine that fills those buffers by draining the
command's output isn't awaited, so the reads raced its final writes.
Own the goroutine here rather than letting the onRun callbacks spawn it,
and join it before reading the buffers. The pty reader reaches EOF on its
own once the process exits, but the non-pty pipe never does, so its
handler now closes the read end to unblock the reader; the pipe is
synchronous, so by the time the command has exited all of its output has
already been read and nothing is lost. This also plugs the goroutine that
the non-pty streaming path previously leaked on every command.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
runAndStreamAux funnels a command's stdout and stderr into a single
cmdWriter (the command-log panel, or a buffer when output is suppressed)
from two separate goroutines: stderr through the MultiWriter set on
cmd.Stderr, and stdout through the onRun callback. Those goroutines
wrote the shared writer without any synchronization, racing on the
prefixWriter's prefixWritten flag and interleaving the two streams.
Wrap the writer so its writes are serialized.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
winPty.Close could block indefinitely, and it is called while holding
the global PtyMutex and while the task's onDone sync.Once is
executing, so blocking there wedges the task's entire cleanup chain:
the next NewTask call blocks on <-notifyStopped while holding
waitingMutex, every later task for that view queues up behind it, and
onResize blocks on PtyMutex — a full UI freeze. (Reported by a user
via go-deadlock's 30s watchdog; a regression from the ConPTY support
introduced for v0.63.0.)
ClosePseudoConsole is what blocks; before Windows 11 24H2 it can do
so in two ways. It flushes the client's pending output into the out
pipe, but a stopped task's scanner goroutine has already quit
draining, so with a client that's still producing output the flush
never completes; this can also wedge the background waiter's
closeHpc, which runs with the pipes deliberately left open. And it
waits for the console host to exit, but closing only delivers
CTRL_CLOSE_EVENT to the attached client without terminating it, so a
client that keeps running (git still computing an expensive diff, a
pager waiting for input) keeps the host alive arbitrarily long.
Run the teardown on a background goroutine so Close returns
immediately no matter which of these strikes, and within it close our
pipe ends before the pseudoconsole, without taking p.mu: breaking the
pipes fails a pending flush fast, which also unblocks a waiter
already stuck in one.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The per-platform getCmdHandlerPty split existed because the Unix side
had creack/pty and the Windows side had nothing — so it fell back to a
non-pty handler. Now that oscommands.StartPty provides a pty on both
platforms, the two files collapse into one cross-platform
implementation and the stub is gone.
cmdHandler grows a 'wait' field because the pty path on Windows spawns
via CreateProcess and never runs exec.Cmd.Start — so cmd.Wait wouldn't
work there. Non-pty handlers set wait = cmd.Wait; pty handlers set it
to the wait closure StartPty returns.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the StartPty stub with a real ConPTY implementation:
CreatePipe + CreatePseudoConsole + PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE
+ CreateProcess. Pagers and external diff tools now get real terminal
behavior instead of being handed pipes.
One Windows-specific quirk worth flagging: ConPTY does not EOF the
output pipe when the child exits; conhost keeps it alive until
ClosePseudoConsole is called explicitly. A background waiter goroutine
calls ClosePseudoConsole as soon as proc.Wait returns, so callers see
EOF on outRead — restoring the Unix master-fd-EOFs-when-slave-closes
semantics they depend on.
The ErrPtyUnsupported sentinel and the no-pty fallback in newPtyTask
are gone now that both platforms have a real implementation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the pty master behind a small interface (Read/Write/Close/Resize),
and push the actual startup into a platform-specific StartPty function
in pkg/commands/oscommands. The Unix implementation still uses
creack/pty; the Windows implementation is a stub that returns
ErrPtyUnsupported, at which point newPtyTask falls back to a plain cmd
task — matching the existing Windows behavior.
The primitive lives in oscommands rather than pkg/gui because the
cmd_obj_runner pty handler (also in oscommands) is going to consume it
too, and tasks → oscommands is the existing dependency direction.
Same observable behavior on every platform; this just carves out a seam
for a real ConPTY implementation on Windows.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Windows ConPTY can't attach a child process to a pseudoconsole via
os/exec — Go's stdlib doesn't expose PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE
(golang/go#62708). The ConPTY path has to call CreateProcess directly,
so it can't hand an *exec.Cmd back to the task runner.
Widen NewCmdTask to accept a small Cmd interface satisfied by both
*exec.Cmd (via the ExecCmd adapter) and the Windows ConPTY command type
we're about to add. Change TerminateProcessGracefully to take
*os.Process, which both cmd shapes can provide.
Behavior is unchanged on every platform.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit d94f2f05 dropped the GIT_OPTIONAL_LOCKS=0 env var that we used
to set on every git command, and re-added lock suppression only as a
--no-optional-locks flag on the background files refresh. The intent
was sound — a foreground `git status` should persist git's refreshed
stat-cache — but the change was too broad: it stopped suppressing
optional locks for every other command too.
The one that bites is the main-view diff. When a folder containing
submodules is selected, we render `git diff --submodule -- <dir>`, and
`--submodule` makes git run `git status` inside each submodule to
describe its "modified" state. That status now grabs the submodule's
index.lock. It runs as a PTY task on its own goroutine, so it races
any submodule-mutating action the user triggers — e.g. resetting a
submodule runs `git -C <submodule> stash`, which then fails with
"index.lock: File exists". This is what made submodule/reset_folder
flaky. `git status` is in fact the only command that takes the
optional lock, but the env var also covered its use inside `git diff
--submodule`, inside PTY-run commands, and inside git's own submodule
child processes — none of which a per-command flag reaches cleanly.
Invert the polarity to match how it worked before d94f2f05: the git
command builder disables optional locks on every command by default,
and the single command that benefits from taking the lock — the
foreground files refresh — opts back in. This restores the original
contention avoidance (including against the user's terminal git) while
keeping d94f2f05's stat-cache-persistence win for the foreground
refresh.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The unit tests only assert the arguments lazygit constructs; they can't
catch cmd.exe's own quote-stripping, which is where #5560 actually
manifested. This test builds a small editor executable, places it and
the file it opens at paths containing spaces, runs it through real
cmd.exe via NewShell, and checks the editor received the intended args.
It runs only on Windows.
Co-Authored-By: Antoine Gaudreau Simard <a.simard@multidev.net>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lazygit builds a shell command by interpolating Quote'd arguments into a
template and running the result via `cmd /c`. Several things were wrong
on Windows:
- Quote emitted bash-style `\"…\"` quoting, which cmd.exe doesn't
understand. Making it usable at all previously required a fragile
round-trip through str.ToArgv and re-escaping.
- The assembled command line was handed to `cmd /c` without `/s`, so
cmd's default rules stripped the wrong quotes once the line contained
more than two of them (e.g. a quoted editor path at a location with
spaces, plus a quoted filename that also contains spaces).
- Shell metacharacters were escaped with `^` (`&` → `^&`, etc.), which
neutralised command chaining, pipes, redirection and `%VAR%` expansion
in custom commands.
Quote now emits the standard Windows convention directly, and NewShell
hands cmd.exe the fully-assembled line verbatim via SysProcAttr.CmdLine,
wrapped as `cmd /s /c "<command>"`. The /s flag strips exactly the outer
quote pair we add, leaving each argument's own quoting intact. With the
`^` escaping gone, metacharacters in a custom command reach cmd as the
author intended; this also removes the spurious `^` reported in #3092.
Fixes#5560Fixes#2427Fixes#4147
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On Windows, NewShell escapes shell metacharacters (`&`, `|`, `<`, `>`,
`%`) with `^` and splits the command into separate arguments. The
operators in a custom command therefore never reach cmd as operators,
so command chaining (`&&`), pipes, redirection and `%VAR%` expansion all
silently break (#2427, #4147, #5113; the stray `^` is also what #3092
reports).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On Windows, Quote wraps arguments in bash-style `\"…\"` and rewrites
embedded double quotes as `"'"'"`. Neither convention is understood by
cmd.exe or CommandLineToArgvW, so commands built from quoted arguments
are mis-parsed once they contain quotes or spaces (#5560).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Windows cannot remove files while handles are still open. In
TestOSCommandFileType we created files and immediately called RemoveAll without
closing handles, which left untracked artifacts (e.g. "testFile" and "file with
spaces") in the working tree. Close the handles and assert RemoveAll succeeds so
cleanup failures are visible.
I copied all files except dot files (.github and .gitignore), the _examples
folder, and go.mod/go.sum.
At some point we may want to copy the files back to the gocui repo when other
clients (e.g. lazydocker) want to use the newer versions of them.
However, show it when there was an error. This is important for the case that a
fork that you have as a remote was deleted, in which case the command log is the
only way to get notified about that.
Some users reported that en_US.UTF-8 is not available on their systems, leading
to warnings in the command log. "C" also forces the language to English, and is
guaranteed to be available everywhere.
The previous commit already fixed the user-visible lag, but there's still a
problem with multiple background git processes consuming resources calculating
diffs that we are never going to show. Improve this by terminating those
processes (by sending them a TERM signal).
Unfortunately this is only possible on Linux and Mac, so Windows users will have
to live with the higher CPU usage. The recommended workaround is to not use
"diff.algorithm = histogram".
As we just did for tasks, close their stdout pipe instead. This makes the called
process terminate more gracefully.
This isn't a change that we *need* to make, it's just a bit nicer.
Previously we would enter a newline at the password prompt, which would cause
the fetch to fail. The problem with this was that if you have many remotes, the
fetch would sometimes hang for some reason; I don't totally understand how that
happened, but I guess the many ssh processes requesting passwords would somehow
interfere with each other. Avoid this by simply killing the git fetch process
the moment it requests the first password.
This is a regression introduced with a199ed1396c; it is important to use a PTY
even with credentialStrategy=FAIL, otherwise the fetch command will spew the
credentials request into the UI and then hang.
This fixes the problem that background fetching makes lazygit hang when the
fetch request needs to prompt for a passphrase. For Mac users who use the
keychain to store their ssh passphrases, this can happen when lazygit is running
while the machine goes to sleep, because macOS looks the keychain in that case.
This decouples StreamOutput from whether a PTY is used. In most cases we just
want to see the output in the log window, but don't have to use a PTY, e.g. for
the bisect commands.
This has the implication that custom commands that are using "stream: true" no
longer use a PTY. In most cases that's probably a good thing, but we're going to
add a separate pty config for those who really wanted this.
