Git doesn't always write conflict markers of seven characters: the
conflict-marker-size gitattribute overrides that per file, and it is set
for good reasons — for file types whose regular content tends to contain
marker-looking lines, such as documentation about merging, or test
scripts. We hard-code seven characters everywhere we look for markers,
so none of that works.
Prepare for honoring the attribute by threading the marker size through
everything that recognizes a marker, carried on the file model. Nothing
fills it in yet, so we still use git's default size of seven everywhere,
and matching is unchanged: a marker consists of exactly that many marker
characters, and all but the "=======" one are followed by a space and a
label.
Creating and applying a stash both touch every changed file, so in a
large repo they can take long enough to be noticeable — and running them
on the UI thread meant the confirmation popup stayed on screen, frozen,
for the whole operation. Run them on a worker instead, with a spinner,
and keep blocking input for their duration so that the type-ahead
guarantee the refresh used to provide still holds.
Dropping stays on the UI thread: it only rewrites the stash reflog, so
it's fast no matter how big the stashes are.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stashing and popping change both the stash list and the files list.
With each scope updating the UI as soon as its own refresh is done, the
two panels visibly change at different times; with
gui.shrinkSidePanelsToContent that also means their sizes change at
different times than their contents.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
ResetSubmodule and fastForward each call a helper that reads the model
from inside their worker: FileForSubmodule reads Model().Files and
worktreeForBranch reads Model().Worktrees, racing the UI thread's model
writes. Hoist both lookups above the worker dispatch.
The remaining refresh scopes each still read model, context, and mode state
directly on their worker, racing the UI thread — the same class of race the
commits refresh had:
- files reads Model.Files (to detect resolved conflicts and drive the
auto-stage) and the Files context's ForceShowUntracked;
- reflog reads the existing reflog slices (for the incremental fetch),
Model.HashPool and the filtering path/author;
- branches reads Model.MainBranches and the previous branches (for the
BehindBaseBranch carry-over);
- stash reads the filtering path.
Gather each scope's inputs into an immutable snapshot on the UI thread (via
captureOnUIThread) before dispatching the git work, and have the refresh
compute from the snapshot — for branches, threaded through both the immediate
and the recency-sorted startup loads, which share one snapshot (the
BehindBaseBranch carry-over is identical either way). Status, tags and
worktrees read nothing UI-owned, so they're left alone.
For the snapshots to actually run on the UI thread, the worker callers that
reach these scopes must announce themselves: convert the submodule
operations, the submodule stash-and-reset, and the background files poller
to RefreshFromWorker.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FileTreeViewModel.RWMutex is removed along with the
withFileTreeViewModelMutex wrapper in FilesController that RLocked it:
every writer (the bounce closure, previous commit) and every reader (key
handlers, disabled-reason callbacks) now runs on the UI thread, so the
mutex is redundant.
RefreshingFilesMutex is removed entirely, including its last use in
repos_helper's DispatchSwitchTo. That use predates the bounce and was
never about FilesController's optimistic-rendering concern; it serialized
a repo switch's onNewRepo() against an in-flight FILES refresh for the
repo being switched away from, so that a slow refresh from the old repo
couldn't write into the freshly-reset model for the new one. Bouncing the
write already broke that guarantee on its own terms — the mutex's critical
section never covered the bounced closure's actual execution, only the
(now-removed) code that enqueued it — so by this point it was only still
locked here without protecting anything real; the previous commit's
repo-generation guard is what now actually closes that race, making this
lock fully redundant rather than just relocated.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GetOnRenderToMain had grown to handle five distinct rendering cases inline (no
selection, submodule conflict, inline text conflict, non-textual text conflict,
and the normal working-tree diff), which made it hard to follow. Split each case
into its own method so the function reads as a short dispatcher, and pull the
repeated main-view boilerplate into renderToMainWithTask. Pure refactor; no
behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a conflicted submodule is selected, the main view shows the commits
each side added relative to their common ancestor as two indented logs,
labelled current and incoming, so it's clear which commit each side would
resolve to.
The logs aren't truncated (the view scrolls). If a side added no commits
of its own (e.g. it was rewound to an ancestor of the other), its head
commit is shown instead.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When both sides of a merge moved a submodule's gitlink, git reports it as "UU".
Pressing space used to fall into the submodule no-op guard and pop the confusing
"Nothing to stage..." error, and enter just entered the submodule, which does
nothing to resolve the superproject conflict.
Treat a conflicted submodule like the other non-textual conflicts: both space
and enter now open a picker offering the two candidate commits, "current" and
"incoming", each labelled with its summary. `git checkout --ours/--theirs` is a
no-op on gitlinks, so we resolve by checking the submodule out at the chosen
commit and staging it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
For a non-textual conflict (e.g. DD/AU/UA/UD/DU), pressing space used to run the
normal stage path, which did something unclear: `git add` happens to resolve the
conflict by keeping the file, but that's neither obvious nor symmetric. Route a
single such file to the same Keep/Delete picker that enter opens, so space and
enter agree.
For a range selection that includes one of these conflicts, staging makes no
sense, so disable it with a toast that points the user at resolving them one at
a time. (Entering a range was already disabled with the standard toast.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Some merge conflicts can't be resolved by editing markers in the merge view;
they require a dialog that picks one side (the "non-textual" conflicts like
DD/AU/UA/UD/DU). Both `enter` and, soon, `space` need to recognize these, so
pull the test into a shared predicate and rename handleNonInlineConflict to
openConflictResolutionMenu to match.
Restructure EnterFile so the predicate is checked first, ahead of the submodule
and inline-conflict branches. This is its final shape: upcoming commits only add
the submodule case to the predicate, with no further reordering. Behavior is
unchanged here, since the predicate is currently false for submodules.
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>
A submodule that only has dirty or untracked content (no new commit) can't
be staged from the parent repo, but it still shows up as having unstaged
changes. Pressing stage on it therefore briefly flashed as staged and then
reverted, without explaining why nothing was staged.
Detect this case (via `git submodule status`, where a '+' prefix marks a
stageable commit change) in the shared stage/unstage decision: if the only
thing that looks stageable is such a submodule, don't try to stage it.
Instead unstage if there's anything staged to unstage, so the toggle stays
symmetric; otherwise show an error explaining that there's nothing to stage.
Because the decision is shared, this covers both the stage (space) and
stage-all (a) keybindings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This map only feeds the optimistic rendering that makes staging feel
instant; it doesn't affect the eventual status, which git reports after
the refresh. The "MM" entry can never be reached for a regular file: a
file at "MM" has stageable unstaged changes, so pressing space stages it
rather than unstaging, and the unstage path is where this map is used. The
only thing that reaches the unstage path at "MM" is a submodule whose
commit is staged on top of dirty content, so this entry exists purely to
update that submodule instantly instead of waiting for the next git
status.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The stage/unstage toggle decides what to do based on whether a node has
unstaged changes: if it does, it stages; otherwise it unstages. For a
submodule this breaks down, because dirty or untracked content inside the
submodule always reports as an unstaged change in the parent repo but can
never be staged from there. Once such a submodule's commit pointer is
staged it sits at "MM", and every subsequent press keeps trying to stage
the unstageable dirty content, so it can never be unstaged.
Treat a submodule's unstaged change as stageable only when its commit
isn't already staged, so that a staged submodule unstages on the next
press regardless of leftover dirty content. Because the decision is now
shared by press and stage-all, this fixes both at once.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pressWithLock (acting on the selection) and toggleStagedAllWithLock (acting
on the whole tree) each independently decided whether to stage or unstage,
ran the optimistic update, and logged the action. That duplicated decision
has already drifted: the tracked-files filter was added to press months
before it was applied to stage-all, and fixes to one have repeatedly had to
be chased into the other.
Extract that shared decision into toggleStaged, leaving each caller to
supply only the git commands it runs (per-path for the selection, bulk
add -A / reset for the whole tree — the latter is required because the tree
root node has an empty path, so a per-path stage wouldn't work). This is a
pure refactor: the two callers' decisions were already equivalent, so
behavior is unchanged. It exists so the next change to the staging logic
only has to be made once.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
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>
This bundles the keyName and a rune, so that we don't have to pass these around
separately everywhere. This should make it easier to swap out the rune for a
string when we upgrade to tcell v3.
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 is probably the less severe case, but it could still be an issue for people
who have many modified top-level files they want to discard, and have
showRootItemInFileTree set to false; they could select all those files by
pressing 'v' and '>'.
When this was originally introduced, it handled single clicks on a list entry
(treating them similar to a double-click by checking whether the click was on
the selected entry). Arguably it should have been called OnDoubleClick back then
already; but when we later changed it to do actual double-click detection (see
37197b8e9a), we should have renamed the methods.
Change working tree files and commit files panels to use filtering
(reducing the list) instead of search (highlighting matches). This
matches the behavior of other filterable views.
The text filter matches against the full file path, not just the
filename, which is more useful for navigating large directory trees.
When toggling a directory for a custom patch while a text filter is
active, only the visible filtered files in the directory are affected,
consistent with how staging a directory in the files panel works.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This avoids a naming collision with GetFilter from the
IFilterableContext interface, which will be implemented by
FileTreeViewModel in the next commit.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Now that we can use 'k' as a menu item binding (this was fixed in #5131), use it
for the "keep" entry in the merge menu. I don't think this will be a problem for
people's muscle memory, given that this menu is not encountered every day; and
it's simply the better keybinding.
This reverts commit b32b55201e.
Users have filed issues with crash reports that seem to indicate that the
FileTreeViewModel gets swapped out (by a refresh) while a call to itemsSelected
is in progress, iterating over the previous items. Guard against this by locking
the mutex that we already have for this for the duration of the call.
I don't have a good way of testing whether the fix helps, because the crashes
only occurred very infrequently. Let's just see if the crash reports stop coming
in after we ship this.
Note also that this is only the minimal fix for the crashes that were reported.
Theoretically, the same problem could happen for a key handler itself, but we
never saw reports about that, so we don't bother doing anything about that yet.
Note also that long-term I envision a different solution to this class of
problems (discussed in https://github.com/jesseduffield/lazygit/issues/2974),
that's why I want to avoid locking mutexes more than necessary now.
Most of our prompts don't (shouldn't) allow empty input, but most callers didn't
check, and would run into cryptic errors when the user pressed enter at an empty
prompt (e.g. when creating a new branch). Now we simply don't allow hitting
enter in this case, and show an error toast instead.
This behavior is opt-out, because there are a few cases where empty input is
supported (e.g. creating a stash).
Stashing doesn't affect submodules, so if you have a working copy that has
out-of-date submodules but no other changes, and then you revert or paste a
commit (or invoke one of the many other lazygit commands that auto-stash, e.g.
undo), lazygit would previously try to stash changes (which did nothing, but
also didn't return an error), perform the operation, and then pop the stash
again. If no stashes existed before, then this would only cause a confusing
error popup ("error: refs/stash@{0} is not a valid reference"), but if there
were stashes, this would try to pop the newest one of these, which is very
undesirable and confusing.
Replace merge-tool with merge options menu that allows resolving all
conflicts for selected files as ours, theirs, or union, while still
providing access to the merge tool.
The root item's path is ".", and the path of a file at top level is "./file".
When using GetPath, this gives us "." and "file", respectively, and
isDescendentOfSelectedNodes would return false for these.
Working with the internal paths (i.e. without stripping the leading "./") fixes
this.
Also, fix two other commands that stage all files under the hood:
- when continuing a rebase after resolving conflicts, we auto-stage all files,
but in this case we never want to include untracked files, regardless of the
filter
- likewise, pressing ctrl-f to find a base commit for fixup stages all files for
convenience, but again, this should only stage files that are already tracked
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 took the set of enabled checks from revive's recommended configuration [1],
and removed some that I didn't like. There might be other useful checks in
revive that we might want to enable, but this is a nice improvement already.
The bulk of the changes here are removing unnecessary else statements after
returns, but there are a few others too.
[1] https://github.com/mgechev/revive?tab=readme-ov-file#recommended-configuration
