After fetching, we auto-forward main branches that have fallen behind
their upstream, but skip any that are currently checked out in another
worktree — otherwise we'd update the ref behind that worktree's back,
leaving its working copy showing the inverse of what was just fetched.
The skip check had a hole though: when starting lazygit while no
worktrees have a main branch checked out, leaving it running in the
background, and then checking out a main branch in one of the other
worktrees outside of lazygit (e.g. in a lazygit instance in another
terminal, or using `git checkout` in the shell, or using some other git
client or IDE), then lazygit wouldn't notice the change, and the next
fetch would auto-forward main even though it is now checked out in a
worktree.
The fix is to include `WORKTREES` in 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.
A few small things picked up along the way landed as separate commits
first:
- Preserve the empty-slice fallback in `loadWorktrees` when `git
worktree list` fails — the fallback was being overwritten by the nil
return value on the next line.
- Add `PULL_REQUESTS` to the manual fetch refresh scope to match the
background fetch; looks like an oversight from when PR support was
added.
- Extract `BranchesHelper.PostFetchRefresh` so the two fetch paths can't
drift again.
Fixes#5020.
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 services config has been the path for GHE for a while (for the
View-PR-URL feature) but it now does double duty: it's also what enables
the branches-panel PR icons for non-github.com hosts. Worth calling out
explicitly so users don't assume it's still github.com-only.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 cherry-picking a commit, copying a range-selection of commits was
broken in that it would copy only the last selected commit rather than
all of them.
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.
As a followup to #5571, improve performance for inline spinners too.
This requires a bit of preparation; in particular, we change
ListContextTrait.HandleRender to no longer force a UI layout/redraw, so
callers need to take care of this themselves where needed. This requires
careful testing to see that we didn't miss any situation where redrawing
only worked by accident; see the first commit of the branch for an
example.
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.
### PR Description
This fixes#4734, where lazygit would use a lot of CPU when the spinner
was shown due to redrawing the entire screen every time the spinner
needed a redraw.
The change adds the ability to request a UI redraw without invalidating
the full UI, thus only redrawing the spinner, re-routing the spinner to
this new flow.
CPU usage on a chromebook went from 60% to 5% when the spinner is shown.
Changing the refresh rate of the spinner from 50ms to 200ms brings it
down from 5% to 1% if ever we would like to change the default,
personally I prefer the slower moving spinner, but that's subjective.
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.
Our most recent change to the script (58309b02a9) broke it because the
anchored regex's no longer match the beginning of the subject. Fix this
by omitting the hash, which is a bit unfortunate but probably acceptable
(I rarely look at the output of the script anyway).
Our most recent change to the script (58309b02a9) broke it because the
anchored regex's no longer match the beginning of the subject. Fix this by
omitting the hash, which is a bit unfortunate but probably acceptable (I rarely
look at the output of the script anyway).
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.
Stefan Haller
Antoine Gaudreau Simard