The cheatsheet has been showing only the first key of each binding
since Binding.Key became Binding.Keys; collapse the list back into a
single comma-separated cell so users can see all the alternates at a
glance once bindings start carrying more than one key.
This is a pure refactor in preparation for letting users configure multiple
alternate bindings for a single command. Every Binding still has exactly one
key, so nothing changes visibly: the cheatsheet, the on-screen options bar,
and the keybindings menu all render identically.
When a Binding ends up with multiple keys, the on-screen options bar will
show only the first (to avoid clutter); the cheatsheet will show all of them (in
a later commit). For now both paths take Key[0].
MenuItem.Key is changed in the same way, it also has a slice of keys now.
In this commit we keep the name `Key` in Binding, KeybindingOpts and MenuItem,
instead of renaming them to `Keys` right away, in order to keep the diff a bit
more readable. We'll do the rename separately in the next commit.
Constructing a menu item key from a literal character requires
gocui.NewKeyRune('r'), which is a bit noisy. Add a private menuKey helper in
both the controllers and helpers packages so the common case in either reads as
menuKey('r'). Duplicating the one-liner is cheaper than a cross-package import
dependency and avoids forcing every controller file to qualify the call.
The reason for doing this now is that we are going to change MenuItem.Key to a
slice of keys later in the branch, which means we'd have to add `[]gocui.Key{`
at each call site, making them even more noisy. With the menuKey helper we can
just change its signature and leave all clients unchanged.
For legacy reasons, OptionMenu was set to `<disabled>`, and OptionMenuAlt1 to
`?`. This doesn't make a lot of sense any more; get rid of OptionMenuAlt1 and
bind OptionMenu to `?` by default. This is a breaking change for users who
rebound OptionMenuAlt1 in their config, but it doesn't strike me as very likely,
and it's easy enough to fix.
Shell.RunShellCommand was passing os.Environ() to its child, while its
sibling runCommandWithOutputAndEnv has used the minimal
NewTestEnvironment since late 2023 when env isolation was introduced;
the sh path was just missed.
This matters when integration tests run from inside a `git rebase -x`
exec in a linked worktree: git sets GIT_DIR=<main>/.git/worktrees/<name>
for the exec, and it leaks all the way down through bash, just, go test,
and the test process, into every git invocation RunShellCommand spawns.
cmd.Dir becomes irrelevant — git resolves GIT_DIR over cwd-based
discovery, with the work-tree taken from the gitdir file (i.e. the
worktree root). So `git checkout -b conflict` in a test fixture creates
the branch on the real worktree and switches its HEAD, hijacking the
in-progress rebase and trashing the working tree. (In the main worktree
git doesn't set GIT_DIR for rebase exec, which is why the bug was only
visible from linked worktrees.)
Using self.env also incidentally restores GIT_CONFIG_GLOBAL for shell
commands, so commits made via RunShellCommand are now authored by the
test config's CI identity rather than whatever the host's ~/.gitconfig
resolves to.
AutoForwardBranches relies on the worktree model to skip any branch
that's currently checked out in another worktree (so we don't update
its ref behind the worktree's back). The post-fetch refresh wasn't
including the worktrees scope, so any external change to the worktree
list between lazygit's startup and the fetch — a `git worktree add`,
a `git checkout` in a linked worktree, a branch rename — left the
in-memory model stale and the skip check returned false negatives.
Add WORKTREES to the post-fetch refresh scope when auto-forwarding is
enabled. We gate on the config so users with auto-forward disabled
don't pay for an extra `git worktree list` plus per-worktree rev-parse
on every fetch tick.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the linked worktree's branch is changed externally — by another
shell, by another tool, or by git running outside lazygit — lazygit's
worktrees model goes stale. The next post-fetch auto-forward then
doesn't realise the branch is now checked out elsewhere, and advances
its ref behind the worktree's back. The worktree's HEAD then resolves
to a commit its index/working tree haven't been updated to, and the
user sees that diff as the inverse of what was fetched — files
appearing as pending changes that they didn't make.
The test sets up a linked worktree initially on a side branch, then
externally checks out master in it before pressing fetch. Two
EXPECTED/ACTUAL pairs capture the symptoms: the branches view shows
master as `✓` rather than `↓1`, and switching to the linked worktree
shows master's would-be incoming file as a pending deletion against
HEAD.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The post-fetch logic was duplicated in `backgroundFetch` and the manual
fetch handler: refresh a fixed set of views, then auto-forward branches
if the fetch succeeded. The two had already drifted on the refresh
scope; folding them into a single helper makes the duplication go
away and prevents it from drifting again.
Pass the fetch error through so we preserve the previous behaviour of
refreshing unconditionally but only auto-forwarding on success.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The background fetch path already includes PULL_REQUESTS in its
post-fetch refresh scope, but the manual fetch from the files view
doesn't. As far as I can tell that's an oversight from when
PULL_REQUESTS was added — there's no reason the two paths should
differ. Align them so both refresh PRs after fetching.
This also sets up the next commit to extract a shared helper for
the post-fetch refresh.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
If `git worktree list` fails, we want the Worktrees model to fall back
to an empty slice so callers iterating over it stay correct. The error
branch was setting it to `[]`, but the line below unconditionally
overwrote it with the nil `worktrees` value from the failed call.
Use an else branch so the empty-slice fallback actually sticks.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Compile them only once at startup. I didn't measure if this makes a difference,
but it's easy to do, and now that we potentially need to check them more often,
it might be worth it.
The branches-panel PR icons only worked for github.com remotes. There was no
fundamental reason — the auth library we already vendor (cli/go-gh) supports
enterprise tokens out of the box (GH_ENTERPRISE_TOKEN, gh auth's keyring),
and the user-facing 'services' config has long been the documented way to
tell lazygit "this domain is a github service" for the View-PR-URL feature.
The fetcher just hardcoded github.com in three places:
- a substring check on the remote URL to decide we're "in a github repo",
- the GraphQL endpoint (always api.github.com/graphql), and
- the auth lookup (always against the default host).
Plumb the resolved web domain through instead. Detection now goes through
the hosting_service ("is this remote's provider 'github'?"), which means a
user with services: { 'git.acme.com': 'github:git.acme.com' } configured
gets PR icons on their GHE remotes too.
Replacing the substring check with a provider check also tightens a latent
bug in getGithubRemotes: it previously accepted any remote whose URL parsed
with the default regex, including gitlab and bitbucket — masked today only
by the InGithubRepo gate, but exposed once the gate goes away.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The github pull-request fetcher needs to know whether a given remote is a
github-type service, which host its API lives on, and which owner/repo to
query against. Today the fetcher hardcodes the first two ("does the URL
contain github.com" and "https://api.github.com/graphql") and re-derives
owner/repo from the remote, which precludes GitHub Enterprise and makes
the fetch entry point take more arguments than it needs.
Add an accessor on the hosting service manager that exposes the already-
resolved service domain together with the parsed owner/repo, so callers
can answer all of these questions without reaching into the manager's
internals.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After a successful paste DidPaste is true, hiding the "X commits copied"
indicator but leaving the buffer populated. From the user's perspective
this looks like a clean slate, so a new shift+C should start fresh. It is
important to reset DidPaste first, before populating the buffer with the new
commits, because otherwise each loop iteration would overwrite the previous one
since Add() rebuilds the set via SelectedHashSet() which returns empty while
DidPaste is set.
After a successful paste, the "X commits copied" indicator hides and
DidPaste is set, but the buffer is not cleared. If the user then
range-selects multiple commits and presses shift+C, every Add() call
rebuilds the set via SelectedHashSet(), which returns empty while
DidPaste is true; each iteration of the copy loop therefore overwrites
the previous one and only the last commit in the range survives.
Now that HandleRender no longer does an implicit Render(), we can use it inside
OnUIThreadContentOnly to save performance, on the assumption that rerendering a
view that contains an inline spinner never changes the layout.
self.c.Render() at the end of HandleRender was there to schedule a gocui Update
tick so the view content modified above would actually get drawn. For UI-thread
callers (the great majority -- keybinding handlers, the layout function itself,
popup resize, etc.) this was unnecessary work, since gocui already runs a
layout/redraw cycle after every event. SimpleContext.HandleRender doesn't call
Render() either, so this aligns the two implementations.
The few callers that drove HandleRender from a worker goroutine and relied on
Render() for the flush were wrapped in OnUIThread in the preceding commits, so
the implicit Render is no longer needed.
Also, Render() being called *before* setFooter() looks like it might have been a
theoretical race; this is no longer an issue now.
The redraw of the selection color (using <space>) would be tied to the
spinner drawing. To reproduce, having a high spinner refresh rate and
toggling a file would see a delay equivalent to the time spinner refresh
rate.
SetSuggestions has two callers: prepareConfirmationPanel calls it directly on
the UI thread, while editors.promptEditor and
SuggestionsContext.RefreshSuggestions call it via AsyncHandler, which runs the
result closure on a worker goroutine. The worker path currently relies on
HandleRender's self.c.Render() to flush the view update. Wrap the body in
OnUIThread so the worker path stays correct when Render() is removed; for the
UI-thread caller the extra bounce is harmless.
refreshBranches runs on a worker goroutine and re-renders the commits view
directly to refresh the branch-head visualization. As with refreshView, this
currently only flushes because HandleRender ends with self.c.Render(). Wrap the
explicit HandleRender call (along with the LocalCommitsMutex pair around it) in
OnUIThread so it keeps working once Render() is removed.
An async refresh dispatches refreshXyz on a worker goroutine, which then calls
refreshView -> PostRefreshUpdate -> HandleRender. Today the final
self.c.Render() inside ListContextTrait.HandleRender is what triggers a UI flush
from the worker. We're going to remove that Render() call, so prepare by
wrapping refreshView's body in OnUIThread.
This moves the entire rendering of the view (and the ReApplyFilter/ReApplySearch
stuff) to the UI thread, not just the layout. I don't expect this to make a
difference in practice, and it is already one step towards my long-term goal of
moving all view rendering to the UI thread (see
https://github.com/jesseduffield/lazygit/issues/2974#issuecomment-1729154768).
Unrelated to this branch, just because we're touching this code: there's little
reason to set the color on the background thread but the text on the UI thread.
Set them both together on the UI thread. Avoids a data race (unlikely to be a
problem in practice, we're talking about a 64-bit int, but still).
In 0d195077e4 we improved the performance of the status bar spinner by
avoiding a layout. This is fine from one spinner tick to the next, but it's a
problem when spinning starts or ends (or in the hypothetical case that the
status text changes in the middle of the operation, which we never do in
lazygit, but theoretically could). In this case a layout is needed so that the
rest of the status bar gets pushed over appropriately (or moves back to the left
when the spinner ends), and also so that the bottom line is shown or hidden
properly for users who set gui.showBottomLine to false.
To fix this, keep track of the status string width and force a layout whenever
it changes. This includes the beginning and end of an operation when it changes
from empty to non-empty or vice versa.
There is currently no observable misbehavior from this bug, but that's only
because we must have a HandleRender call somewhere that forces a full layout
when an operation starts or ends. We will remove the Render() call from
HandleRender at the end of this branch, at which point the misbehavior would be
visible if we didn't fix it here.
This prevents views from drawing over higher z-order views.
Currently this is not an issue in practice, because we use
ForceFlushViewsContentOnly only for the bottom line status spinner, and there
are never views on top of it. However, later in the branch we will use the
mechanism to redraw the inline spinners in panels (e.g. the "Pushing..." status
next to a branch name), and there could be a popup on top of it.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
With a very slow spinner rate (seconds), you can see that at the beginning of an
operation the bottom line leaves a gap for where the status will go, but the
status (and spinner) is only drawn the first time the spinner ticks. The reason
is that layout looks at GetStatusString to decide how much room to leave, rather
than at the actual content of the AppStatus view; the view content is only set
by renderAppStatus the first time the spinner ticks.
Fix this by making layout look at the actual content of the view so that the
layout is in sync with what is drawn. This also avoids flicker if an operation
is so fast that it finishes before the spinner ticks for the first time,
especially for users who set gui.showBottomLine to false.
This didn't cause a bug so far because switching repos always happens from
within an OnWaitingStatus, so the spinner would take care of calling layout and
draw. However, later in this branch we are going to optimize the spinner so that
it no longer calls layout, at which point this would break, so make sure we
rerender at the point where it's needed.
Modifiers were moved into Key in 22169e22f, but the separate Modifier field
on types.Binding and gocui.keybinding was left behind. The keypress matcher
already compares modifiers via Key.Equals, so the old field is never read on
the dispatch path; it just got passed through SetKeybinding and stored.
Drop it from gocui.keybinding, types.Binding, and the SetKeybinding /
DeleteKeybinding signatures, and remove every now-redundant Modifier:
gocui.ModNone struct field. Mouse bindings keep their own Modifier (on
ViewMouseBinding) since that path still consults it.
I noticed that it compares key.keyName and key.str separately instead of calling
key.Equals, but instead of deciding whether that's a problem, just delete the
function when nobody needs it.
While I strongly prefer the alt-arrow bindings myself, I'm worried that existing
users might not be happy about the change, so I'm reverting it.
I'm working on supporting multiple keybindings for every command, so once that's
in, we can change the default to include both.
The commit graph used '⏣' (U+23E3 BENZENE RING WITH CIRCLE) for merge
commits and '◯' (U+25EF LARGE CIRCLE) for regular commits. Both have
very poor coverage in popular monospace fonts:
- '⏣' lives in the Misc Technical block and is essentially absent from
every common monospace font (Source Code Pro, JetBrains Mono, Fira
Code, Cascadia Code, Hack, Iosevka, Menlo, Consolas, Monaco, IBM
Plex Mono, Ubuntu Mono, Noto Sans Mono, Inconsolata). It is always
drawn from a system fallback font.
- '◯' is the late-addition LARGE CIRCLE codepoint. It is present in
some fonts (Cascadia, Fira Code, Hack, Iosevka, Menlo, Noto Sans
Mono) but missing from many others, including Source Code Pro and
most Nerd Font derivatives based on it.
This is why the graph renders inconsistently across platforms even
when the same monospace font is configured: each OS picks a different
fallback font (Apple Symbols on macOS, Segoe UI Symbol on Windows,
Noto/DejaVu/Symbola on Linux), and the substituted glyphs differ in
shape, weight, and advance width. '◯' is also East Asian Ambiguous
width, so some terminals render it wider than one cell, exaggerating
the misalignment.
Replace the symbols with codepoints from the foundational 1991
Geometric Shapes block, which has far broader font coverage:
- Merge: '◎' U+25CE BULLSEYE -- concentric circles, the visually
closest cousin to the previous benzene-ring glyph.
- Commit: '○' U+25CB WHITE CIRCLE -- the same hollow-circle silhouette
as before, just a more universally available codepoint.
The new symbols are present in the font directly in significantly
more cases; and when fallback is still required, they are universally
well-drawn (unlike '⏣', which many fallback fonts also lack).
Co-Authored-By: Claude Opus 4.7 (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.
The call was meant to guarantee a UTC time zone to make the tests deterministic,
but as the previous commit explained, this only worked on Mac and Linux, not on
Windows. Since we now have a better fix, this workaround is no longer needed.
