PostFetchRefresh's refresh is the only background refresh carrying a
Then callback, and Then callbacks are not generation-guarded: when the
background fetch's refresh crossed a repo switch, the callback still
ran — in the new repo — and auto-forwarded the new repo's branches
because the old repo's fetch had completed. That was harmless in
practice (the update-ref call compares against the expected old value,
and it only does what the next fetch's auto-forward would do anyway),
but mutating refs in a repo whose fetch never happened is not an action
the user took. Skip the auto-forward when the repo generation changed
since the fetch started.
The generation is captured by the fetch's callers before the fetch
runs, not by PostFetchRefresh itself: the background fetch doesn't
block repo switching and is a network call, so by the time
PostFetchRefresh runs a switch may already have happened — a capture
there (or the one the refresh itself takes) would compare against the
new repo's generation and let the auto-forward through. For the manual
fetch the capture point makes no difference, since a foreground
operation blocks repo switching for its entire duration.
This deliberately guards only this call site rather than making Then
callbacks generation-guarded in general: a Then is an arbitrary
callback, and whether it is safe to skip on a repo switch is a decision
for the author of the call site.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With sync vs async now derived from the calling thread, the Mode field
and its SYNC/ASYNC constants no longer carry any information: Refresh is
always async, RefreshFromWorker always sync. Drop the field, the type,
and the Mode argument at every call site, and reduce the debug log's
mode name to a plain sync/async derived from calledFromWorker.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The three branch-delete handlers and the two worktree-removal
continuations collapsed the Branches/RemoteBranches range selection from
their worker goroutine, racing the UI thread. Wrap each collapse in
OnUIThread, keeping it in the same spot relative to the refresh (FIFO
preserves the collapse-then-refresh order the name-restore depends on).
A commits refresh does its git work on a worker and then reads the model,
the contexts, and the modes for that work directly from there:
LocalCommits.GetSelectionRangeAndMode/GetLimitCommits/GetShowWholeGitGraph,
Model.Commits/MainBranches/HashPool, the filtering path/author. Those are
owned by the UI thread, which is concurrently running the cursor and render
code, so the reads race it — the dominant, confirmed source of the
commits-scope flakes (the startup ClampSelection vs GetSelectionRangeAndMode
race, for one).
Gather them into an immutable capturedCommitState on the UI thread, before
the git work is dispatched, and have refreshCommitsWithLimit compute from
that snapshot. UI-thread callers capture inline; worker callers can't (a
SYNC/BLOCK_UI refresh parks the UI thread at wg.Wait, so hopping from a
scope sub-worker would deadlock), so the capture is lifted out of the scope
worker into the refresh orchestration, and worker callers announce
themselves with a new RefreshFromWorker entry point that hops the capture to
the UI thread and blocks for it (OnUIThreadAndWait). BLOCK_UI runs the whole
refresh on the UI thread regardless of the caller, so it captures inline
too.
Every refresh issued from a worker that reaches the commits (or branches,
which pulls in commits) scope is converted: the fast-forward, branch/tag
delete, worktree remove/detach, push, reword-via-rebase, author edits,
custom-command, hard-reset-with-autostash, reset-to-ref, fetch-and-checkout,
gpg-stream, post-fetch, and external-change-poller refreshes, plus the
branch checkout and move-commits-to-new-branch refreshes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
For the busy query to be usable as a repo-switch guard it has to be
false while the ongoing background routines run, or a switch would be
refused every time a background fetch or files refresh happened to be in
flight. Mark that work as background so it's excluded from the query.
The background routine dispatch in goEvery becomes OnWorkerBackground,
and the auto-fetch waiting status renders its spinner through the
background variants. Within a refresh, the background flag (which
Refresh already carries as options.Background, and which the files path
already threaded) is now threaded through every place that enqueues a
task: the async scope workers, the model-write bounces
(onUIThreadUnlessRepoChanged), refreshView, the staging bounce, the
Then dispatch, and the branch-loader's behind-count worker. Two
single-caller chains reached by a background files refresh get the flag
too: MergeConflictsHelper.EscapeMerge and BranchesHelper.
AutoForwardBranches (whose follow-up refresh must stay background when
triggered by the background fetch).
Nothing gates on the busy query yet, so this is behavior-preserving;
background tasks still count as busy for the test idle-listener, which
looks at every task regardless of the background flag.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
refreshBranches now loads the branches (and worktrees) on the worker and
writes Model.Branches, the pull-requests map, Model.Worktrees, and the
restored branch selection in an onUIThreadUnlessRepoChanged bounce. The
selection restore and rebuildPullRequestsMap run in the bounce so they
see the branches we just wrote; the LocalCommits re-render (for branch
head visualization) moves into the same bounce.
refreshStatus is adjusted to read the checked-out branch and the linked
worktree name inside its bounce rather than on the worker: both derive
from models (Branches, Worktrees) that are now written via bounces, so
reading them on the worker would format the status from stale values —
which showed up as the status line dropping the "(worktree)" suffix right
after entering a submodule or switching worktrees. The git work
(WorkingTreeState) stays on the worker.
Two callers that read the branches model right after a SYNC branches
refresh move their reads into Then:
- BranchesHelper.PostFetchRefresh: AutoForwardBranches reads Model.Branches,
so it runs in Then (preserving that a fetch error is still returned to
the caller and that background auto-forward errors aren't surfaced as a
popup).
- BranchesController rename: the re-select-by-name loop runs in Then.
RefreshingBranchesMutex is left in place for the mutex cleanup.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pressing `d` on a worktree only ever removed the worktree, leaving its branch
behind even though deleting it too is often what you want. Turn the confirmation
into a menu: "Remove worktree", "Remove worktree and delete branch", and "Remove
worktree and delete local and remote branch". The branch-deleting items come
after the plain removal (they do more harm if picked by accident); both are
greyed out for a detached-HEAD worktree, and the local-and-remote one is also
greyed when the branch has no upstream. The plain menu pick is the confirmation,
so the standalone "remove worktree?" prompt is gone (and its now-dead
translation string with it); the dirty-worktree force prompt and the
unmerged-branch warning still appear when relevant.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Picking "Delete local and remote branch" for a single branch that's checked
out in another worktree used to fail with "Some of the selected branches are
checked out by other worktrees. Select them one by one to delete them." That
message only makes sense for a multi-selection; for a single branch there's no
reason we can't remove the worktree and delete both the local and remote branch
in one go. Route that case through the same worktree menu as the local-only
delete, with labels that spell out that the remote goes too. The multi-select
error stays for actual multi-selections.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When you delete a local branch that's checked out in another worktree, the
menu offered to remove or detach the worktree but then stopped there, leaving
the branch you asked to delete still around. Now both actions delete the branch
afterwards, and the labels say so ("Remove worktree and delete branch" /
"Detach worktree and delete branch") to avoid surprises.
Also drop the "Switch to worktree" item: switching abandons the delete the user
asked for, and it's already reachable by checking out the branch or via the
worktrees panel. And drop the now-redundant "remove worktree?" confirmation:
the explicit menu pick is the confirmation (the dirty-worktree force prompt and
the unmerged-branch warning still appear when relevant).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split the actual worktree removal out of the confirmation in Remove into a
non-confirming helper, and give both Remove and Detach an optional `then`
continuation that runs after a successful removal in place of the default
refresh. Upcoming flows need to delete the worktree's branch once the worktree
is out of the way; threading a continuation through (rather than the caller
firing branch deletion independently) keeps it ordered after the git command
that actually frees the branch. No behavior change yet.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pull the merged-check-and-force-warning step and the actual git deletion
out of ConfirmLocalDelete and ConfirmLocalAndRemoteDelete into helpers, so
that the upcoming worktree-aware delete flows can reuse them instead of
duplicating the logic. No behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
We set GIT_OPTIONAL_LOCKS=0 for every git command we run. That env var
only affects `git status`: it tells git not to take the optional lock it
would otherwise use to write the index back after refreshing the cached
stat information. The intent was to avoid contending for index.lock with
git commands the user runs in a terminal.
The downside is that our `git status` never persists the refreshed
stat-cache. So whenever the working tree's cached stat info goes stale
(e.g. editing files and discarding the changes, or a checkout), every
subsequent status re-hashes the affected files to confirm they're clean,
and stays slow until something else writes the index (such as the user
running `git status` in a terminal).
Fix this by only suppressing optional locks for refreshes that run
unattended in the background; foreground refreshes triggered by a user
action now run a plain `git status` that writes the refreshed index back,
just like the command line does. Background refreshes keep passing
--no-optional-locks so they still can't cause lock contention.
RefreshOptions gains a Background flag that the background routines set,
threaded down to the status command.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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>
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.
This makes it easier to recover them if they were deleted accidentally.
We only do this for local branches for now, we don't bother for remote branches;
it would be trickier there, because the branch on the server might not actually
point to the same hash as our local remote tracking branch does.
We only want to do this when the function is called from the remote branches
panel. It can also be called with a selection of local branches in order to
delete their remote branches, but in this case the selection shouldn't be
collapsed because the local branches stay around.
We had code already that was supposed to do this, but it didn't work. It should
have used SetSelection() instead of SetSelectedLineIdx(); the latter doesn't
actually cancel a range selection.
Introduce a new function specifically for collapsing the range after deleting
multiple items, so that clients don't need two calls (we'll add a bunch more in
this branch).
Refresh is one of those functions that shouldn't require error handling (similar
to triggering a redraw of the UI, see
https://github.com/jesseduffield/lazygit/issues/3887).
As far as I see, the only reason why Refresh can currently return an error is
that the Then function returns one. The actual refresh errors, e.g. from the git
calls that are made to fetch data, are already logged and swallowed. Most of the
Then functions do only UI stuff such as selecting a list item, and always return
nil; there's only one that can return an error (updating the rebase todo file in
LocalCommitsController.startInteractiveRebaseWithEdit); it's not a critical
error if this fails, it is only used for setting rebase todo items to "edit"
when you start an interactive rebase by pressing 'e' on a range selection of
commits. We simply log this error instead of returning it.
I'm not aware of any real scenario where this can happen, but we have seen one
stack trace where it crashed with an out-of-bounds error in the range expression
below, so there must be a way. And it seems better to guard against it anyway,
rather than assuming it can't happen.
We allow deleting remote branches (or local and remote branches) only if *all*
selected branches have one.
We show the a warning about force-deleting as soon as at least one of the
selected branches is not fully merged.
The added test only tests a few of the most interesting cases; I didn't try to
cover the whole space of possible combinations, that would have been too much.
Since we want to select multiselections, this will make it easier to pass a
slice of remote branches. It does require that for the case of the local
branches panel we need to synthesize a RemoteBranch object from the selected
local branch, but that's not hard.
Currently we try to delete a branch normally, and if git returns an error and
its output contains the text "branch -D", then we prompt the user to force
delete, and try again using -D. Besides just being ugly, this has the
disadvantage that git's logic to decide whether a branch is merged is not very
good; it only considers a branch merged if it is either reachable from the
current head, or from its own upstream. In many cases I want to delete a branch
that has been merged to master, but I don't have master checked out, so the
current branch is really irrelevant, and it should rather (or in addition) check
whether the branch is reachable from one of the main branches. The problem is
that git doesn't know what those are.
But lazygit does, so make the check on our side, prompt the user if necessary,
and always use -D. This is both cleaner, and works better.
See this mailing list discussion for more:
https://lore.kernel.org/git/bf6308ce-3914-4b85-a04b-4a9716bac538@haller-berlin.de/
This might seem controversial; in many cases the client code gets longer,
because it needs an extra line for an explicit `return nil`. I still prefer
this, because it makes it clearer which calls can return errors.
