An uppercase letter is not valid with ctrl, and only works because we lowercase
the string before parsing it. This will change later in this branch when we
start supporting bindings like <c-s-r>.
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 so that they look the same no matter what color palette the terminal is
using. (One user complained that the text for the Open state is barely readable,
because they are using a palette that has a very pale green.)
GitHub uses slightly different colors depending on light vs. dark mode;
fortunately they are very close, so hopefully we can ignore this. I picked the
ones for dark mode here, on the assumption that this is more common.
Also, not all terminals support true hex colors; for example, Terminal.app on
macOS doesn't, so it maps the colors to the closest ones in the Xterm-256
palette. This shouldn't be a huge problem, but for some reason it displays draft
PRs as something closer to Cyan than grey, and I don't understand why.
If the repo has multiple remotes, but only one of them is on Github (the others
might for example point to a self-hosted Critic server or something like that),
lazygit would still present a menu to choose the remote for pull requests, but
it would contain only that single entry. That's pointless, pick it automatically
without prompting.
We add some tests while we're at it; these wouldn't have caught the problem,
because they only test getGithubBaseRemote which already takes the filtered
github remotes. It's still better than not having any tests; the real issue
could only have been caught with an integration test, which we don't bother
adding.
If we used to have a base remote (e.g. because there was only one), but now we
don't (e.g. because the user has added a second one, and we don't know yet which
one is the base), stop showing the cached PRs. This is especially important in
the case that the user cancels the prompt, in which case we don't have a base
remote for the rest of the session, but would keep showing the (potentially
stale) cached PRs forever.
This is useful when cancelling out of the commit panel mid-sentence (after
having typed the space for the next word); when entering the commit message
panel again, the space was gone and you had to type it again. Small thing, but
it just seems better to resume the panel in exactly the state that you left it
in. (Which we actually don't do; we don't remember the cursor position, or which
of the subject/description panels was active. That would be a separate
improvement.)
The save path and the load path used to be asymmetric. On save, the textarea
getters applied strings.TrimSpace, which stripped any leading blank lines, a
trailing newline after the cursor, or indentation on the very first line of the
description — all of which are legitimate user content. On load,
SplitCommitMessageAndDescription did TrimSpace on the description as well, and
the preserved message was routed through that same git-format split because
HandleCommitPress passed it as OpenCommitMessagePanel's InitialMessage. The
result: every round-trip through "escape and reopen" silently mutated the
message.
The fix is to treat our own preservation file as its own format, distinct from
git's canonical "summary\n\nbody" format:
- The textarea getters return raw content. strings.TrimSpace moves to the one
place that still needs it: the empty-summary check in HandleCommitConfirm (git
itself strips trailing whitespace and blank lines, so no pre-trim is needed
before -m).
- SplitPreservedCommitMessage / SetPreservedMessageInView split on the single
"\n" our Join uses, without any trimming — truly lossless.
- SplitCommitMessageAndDescription keeps its git-format behavior but replaces
TrimSpace with TrimPrefix("\n"), so it strips only the blank-line separator
and leaves body indentation intact.
- HandleCommitPress now mirrors HandleWIPCommitPress: it no longer passes the
preserved message as InitialMessage. OpenCommitMessagePanel resolves the
preserved content itself, uses it for display via the preservation-format
setter, and stores it as the initial message so the close-time "did the user
change anything?" check still correctly detects a cleared panel.
- GetInitialMessage no longer trims. With raw getters on both sides of the
comparison, trimming here caused spurious non-matches (e.g. for preserved
content with trailing whitespace). The original motivation — matching a
"WIP: " prefix with trailing space — works unchanged.
- UpdateCommitPanelView becomes dead code and is removed; its one remaining
caller (history cycling, always git-format) goes directly through
SetMessageAndDescriptionInView.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Callers currently hand this function a trimmed description, so the output is
always clean. An upcoming change to the commit-panel getters will stop trimming
at the callsite (so that whitespace typed by the user round-trips through the
preservation file exactly), at which point the description can end with one or
more newlines. Without this change, a user who presses Enter after their
description body and then invokes "Add co-author" would end up with two blank
lines between the body and the trailer instead of the expected one.
This isn't possible with the default quit binding ('q'), but there's an
alternative binding (ctrl-c) that does work while the panel is open; also, users
can rebind quit to something like ctrl-q.
We'll want to call just the preservation step when the app is quitting
with the commit message panel open: at that point the view-hiding and
context-popping in CloseCommitMessagePanel are pointless (the app is
about to exit), and some of the focus-lost side effects could be
surprising during teardown.
There's no reason for this; when opening the panel next time, it will be
populated again. Also, it doesn't make sense to do this only when not preserving
the message.
Extends the existing retry logic to handle transient lock-related errors beyond just .git/index.lock. This now also retries when encountering "cannot lock ref" or "cannot update ref" errors, which can occur intermittently during fetch/pull operations in large repositories.
Related to #5124
Signed-off-by: majiayu000 <1835304752@qq.com>
Change the tests so that they run in a linked worktree; this uncovers the bug
that copying a file's absolute path uses the main repo path rather than the
worktree's path.
When no base repository is configured using gh, we have several heuristics for
choosing a likely base repo before prompting the user. One of these was that if
the origin remote exists, we'd choose that. This may be the right choice in some
cases, but in many others it's not; for example, in the common scenario of a
forking setup where the upstream repo's remote is called after its owner, and
the fork remote is called origin, we would choose origin as the base repo, which
results in not showing any PRs. Don't do that, and instead prompt the user for
the base repo in this case.
The assumption is that if a pull request exists on a main branch, it was usually
created by mistake and then closed, and showing it serves no purpose and is only
distracting.
We keep showing open pull requests for main branches though, because this allows
you to notice that there is one that you probably want to close.
This only affects the display (in the branches list and in the main view);
opening the PR in the browser using shift-G is still possible, as is copying its
URL to the clipboard.
Normalizes the repository owner to lowercase during the PR
mapping.
This ensures that PR icons and integration features work correctly even
when the local git remote URL casing differs from the official
repository casing on GitHub.
When building multi-step custom command forms, some prompts are only
relevant depending on earlier answers. Without conditional logic,
users must dismiss irrelevant prompts manually.
Prompts now accept a `condition` field with a template expression
evaluated against prior form values. Skipped prompts default to
an empty string.
The template expression is a string pre- and suffixed with double curly
braces - {{}}.
Form keys can be reused, a guard ensures that skipped prompts do not
reset already set form keys with an empty string. This allows the
conditional flow to remind a user to set a key that was left empty
because additional conditions want that key to be set. This removes the
need to have additional if checks in the command that uses the form
keys.
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.
For esthetic reasons, checking out a branch (or other ref) blocks the UI until
the refresh is done, so it's important that the refresh doesn't do unnecessary
work. Refreshing pull requests is unnecessary (but costly, when waiting for it)
when a branch is checked out that already existed locally. However, it is
required when checking out a remote branch for the first time, so that the PR
icon appears immediately when there is one.
For the branches panel we might consider unifying it with the existing `o`
command for creating a PR: it could check if there is a PR already, and open it
if so, or create a new one if not.
However, I also want the command in the local commits panel for the checked out
branch, and there's no existing "Create PR" command there; and the `o` command
opens the selected commit in the browser, so it's unrelated.
Add GitHubCommands struct with GraphQL-based PR fetching, and
GithubPullRequest model. Wire HostingService and GitHub command
structs into GitCommand.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
Extract parseRemoteUrl helper to avoid duplication between
getRepoURLFromRemoteURL and new getRepoNameFromRemoteURL.
Add repoNameTemplate to ServiceDefinition and GetRepoName() to
HostingServiceMgr. Add GetRepoInfoFromURL for callers that need
owner/repo without a full service lookup.
The only information that this carries is whether an entry is a real branch or a
detached head. Detached heads can only be at the top, and they are easy to tell
apart from the other branches by their name, so the icon is not really very
useful.
We are going to show PR icons in this column.
Not a terribly important feature for users, probably, but it is useful for
developers when testing a new feature with or without nerd fonts; and it does go
against or policy of all configs being hot-reloadable.
Note that switching from nerd fonts version 2 to 3 is *not* supported without
restarting, but I find this less important.
Set the sort order's default from the former foldersFirst to mixed, so this is a
change in behavior. I find this useful because it now matches git's order, so if
you look at the diff of a commit, the TOC at the top has the same order as the
file tree you see when entering the commit.
When you rebase a branch and there are conflicts, lazygit asks you to continue
the rebase when it detects that all conflicts have been resolved. However, it is
common to make additional changes beyond just fixing the conflicts (e.g. to fix
build failures), and when doing that while the "Continue the rebase?" prompt is
showing, lazygit detects that too and asks you if you want to stage those newly
modified files too. This is all well and good (and can be disabled for those who
don't like it); however, lazygit would treat out-of-date submodules as unstaged
changes and would offer to stage those as well, and this is pretty bad and
almost never what you want. Fix this by excluding submodules from that second
check.
Previously, when showing subcommits of a branch or tag, scrolling down, and then
going out and entering subcommits again, it would select the first commit but
keep the previous scroll position, so that the selection was not visible. Always
scroll to the top to fix this.
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 '>'.
Previously it would iterate over all changed files and call git checkout or git
reset for each one, which can take forever if there are hundreds or thousands of
files. Now it batches these into a single command if possible (taking care of
still passing the individual path names to the git call rather than just the
directory, which is necessary for making it work correctly when filtering --
this was actually broken for the "Discard unstaged changes" command, which is
fixed here).
When discarding a directory, we only want to discard those files that are
visible in the current filter view. The tests show that this already works
correctly for discarding all changes, but it doesn't for discarding only
unstaged changes: in this case, untracked files are handled correctly, but
changes to tracked files are discarded without respecting the filter.
We have integration tests for this functionality, but those only test the
behavior, not the performance. In these unit tests you can see that we make
individual calls to git checkout and git reset for each file, which is very slow
when there are lots of files.
Previously it would only check that *if* removeFile was called, the passed
argument was the expected one; but it didn't check whether it was called at all.
Improve this by recording the file names that are removed, and checking them at
the end of each scenario.
This is going to be even more important for the tests that we are about to add
in the next commit, because for those there can be several calls to removeFile
in a single scenario.
When switching to a different repo, and then back to the original one, searching
would no longer work. The reason is that our contexts set callbacks on their
views; when switching to a different repo we instantiate a new set of contexts,
so they will overwrite the views' callbacks with their new ones, but when
switching back to the original repo, we reuse the old contexts because they are
still in memory, but they won't set their callbacks again since they only do
this on construction.
To fix this, replace the view-local callbacks with a global one on the gui that
takes the view as an argument, so that the callback can look up the associated
context dynamically.
Can be used for doing additional click handling in list views.
Like the GetOnDoubleClick hook we should try to find a better design for this
than putting it in HasKeybindings and BaseContext, since it is only used by list
contexts.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
We have some documentation for the corresponding setters in IBaseContext, but
that's part of the controller infrastructure and not client facing. For somebody
implementing a new view, this is where they will probably look for what methods
they can override.
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.
If the showDivergenceFromBaseBranch config is not none, the expanded branches
panel would no longer show the remote branch and subject; the reason was that we
padded the name all the way to the right edge of the view so that the divergence
information is right-aligned.
To fix this, we simply don't right-align the divergence in half or full screen
mode. This is a bit unfortunate, but it's hard to do any better, because we
don't know the width of our column in advance. One thing I tried is to make the
divergence a separate column in that case (with an explicit right-alignment set
on it via getColumnAlignments; however, this makes it harder to properly
truncate the name when the window is too narrow.
This fixes the problem; a consequence of this change is that given the following
scenario:
@@ -1,3 +1,3 @@
1
-2
+2b
3
staging only the line `+2b` will put it *before* the unchanged `2` line, rather
than after it as you might expect (the changed unit tests demonstrate this).
Since this should be a pretty uncommon scenario, I guess it is an ok compromise.
As you can see in the changed tests, while the behavior of what gets staged is
fixed now, it doesn't always correctly select the next line to stage. We'll
address this in the next commit.
This is a pretty special case (see comment in the test for details). It is
working correctly, but I almost broke it during development, so it's good to
cover it with a test.
Given a block of consecutive changed lines, trying to stage only some of them
doesn't work correctly in all cases:
- if the staged lines are the last lines in the block of changes, it already
works
- when staging some changes in the middle of the block, it doesn't work as
desired, but we also don't try to fix this case in this branch, because it's
harder to do, and not as common as the other two
- staging the first lines of the block doesn't work as desired, and we will fix
that in this branch.
When a branch is checked out by another worktree, show the worktree
name in the label, e.g. "(worktree cosmos2)" instead of just
"(worktree)", so you can immediately see which worktree holds it.
Actually, we simply enable them in any panel, even those that can never show a
diff. Since the commands show a toast, that's enough feedback to understand what
happened; the change will take effect the next time you switch to a panel that
does show diffs.
Same for ( and ) to change the rename threshold.
This fixes two problems:
- if the previously focused view (behind the panel) was a list view, it would
look like the click would select a different row, because gocui would still
set the view's cursor position, which is used to draw the highlighted row
- it was still possible to click on tab headers, and this would dismiss the
panel
I got confused when I investigated the bug that was fixed in the previous
commit, so hopefully these will be helpful for the next person reading this
code.
The symptom of the bug was that confirmation panels with wrapped text were one
character too wide, which could sometimes result in them being one line too
heigh. One concrete example is the "Discard file changes" confirmation that
appears when pressing `d` in the commit files panel.
I got very confused when investigating this bug, and the reason is that
getPopupPanelDimensionsAux took two parameters panelWidth and panelHeight, but
the second was actually contentHeight. Rename it to contentHeight, and change
the first one to also be the content width rather than the panel width. This is
easier for clients, and less confusing.
This commit implements the ability to cycle backward through different
all branches log visualization modes, complementing the existing forward
cycling functionality. Users can now press 'A' (Shift+a) to cycle in
reverse through the available log graph views, improving navigation
efficiency when they overshoot their desired view.
Changes:
- Added RotateAllBranchesLogIdxBackward() method to BranchCommands for
backward rotation through log command candidates
- Introduced AllBranchesLogGraphReverse keybinding configuration with
default key 'A' (uppercase)
- Implemented switchToOrRotateAllBranchesLogsBackward() handler in
StatusController that mirrors forward cycling logic
- Added English translation for "Show/cycle all branch logs (reverse)"
The implementation uses modulo arithmetic with proper handling of
negative indices to ensure seamless backward cycling through the
available log visualization options.
When .git/info directory does not exist (can happen with bare clones
or manual deletion), the Exclude function failed with 'no such file
or directory'. Added os.MkdirAll to create the directory before
opening the exclude file.
Added integration test exclude_without_info_dir that removes .git/info
before attempting to exclude a file.
The worktrees tab now displays the checked-out branch (or detached HEAD
state) for each worktree, making it much easier to see which worktree
holds which branch.
This is useful when you need to "unlock" a branch that's occupied by
another worktree: you can now clearly see which worktrees are on a
branch vs detached, without having to select each one individually.
Also rename the "(main)" label to "(main worktree)" to avoid confusion
with the common "main" branch name, and move it after the branch column.
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>
Without this check, the selection was being reset to 0 whenever
ReApplyFilter was called for the current filter context, even when
the user wasn't actively typing in the search prompt (e.g. when the
model updates in the background). This was causing unexpected cursor
jumps.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When opening a filter prompt, reset PrevSearchIndex to -1 to avoid
stale search state from a previous search/filter session.
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>
I always press 'd' in the patch building view, expecting that I can do
exactly what I can do in the staging view, to find out I need to go
space -> ctrl+p -> d and I think it's time to honour the muscle memory
and support this convenience keybinding.
We don't add the commands to the menu when we're rebasing, so it's pointless to
check this in the handlers.
I guess alternatively we could add all commands always, and strike out the ones
that are not applicable using DisabledReasons. But we don't usually do this in
menus (except the global keybindings menu).
Only mention resetting the patch when there actually is one. This way users have
to read less text in the normal case, and the added note hopefully stands out
more if there is one. Also, separate the note from the previous text by a blank
line.
"Remove" can be confusing when a deleted file is selected; in this cases it
actually "un-removes" it. "Discard" hopefully makes it clearer that we are
talking about the change to a file, and not the file itself.
If the bottom line is hidden (by setting gui.showBottomLine to false in the
config), don't show the "Fetching..." status because it changes the layout,
which should happen only in response to user actions.
However, keep the status for the first fetch after startup, and right after
switching repos, because in these cases it is useful to see when the fetch is
done.
Tweak regexp such that a trailing ":" is not included in the file path
extracted from a lazygit-edit URL.
Previously, when matching a URL containing the ":" separator but no line
number, such as `lazygit-edit:///path/to/file.ext:`, the trailing
separator would be included in the matched file path, so lazygit would
open the non-existent file `/path/to/file.ext:`. Notably, such urls are
created when using delta with the hyperlink feature, as suggested in
https://github.com/jesseduffield/lazygit/blob/master/docs/Custom_Pagers.md#delta,
and clicking a file path rather than a line number.
This is a regression introduced with #5135. I didn't notice it because I am
using `gui.scrollOffBehavior: jump`, in which case the problem wouldn't appear;
it only happened with `scrollOffBehavior: margin` (which is the default). In
that case, if you used the arrow keys (or j/k) to move the selection so that the
view would start to scroll, empty space would appear.
Paths added to ignore/exclude files need to be prefixed with a forward
slash to point to a specific file in the directory tree.
Without that prefix a file at root called `file` (added to `.gitignore`
as `file`) would match with `./file` and `./src/file`.
This brings in https://github.com/jesseduffield/gocui/pull/98 with the following
fix:
Fix rendering of CRLF sequence ('\r\n')
The FirstGraphemeCluster call returns this as a single character; we want to
treat it the same way as a single \n.
This would be a problem if e.g. a progress bar used \r repeatedly to paint over
the same line, and then printed a \n to move on to the next line; the last pair
of \r and \n was swallowed.
Another scenario where this was a problem was if you stream output of a command
to the log, and the command used \r\n as line feeds. This happens for example
for a background fetch that fails with an error; in that case we print the
combined output (stdout plus stderr) to the log after the command finished, and
for some reason it uses \r\n in that case (I can't actually explain why; when I
do `git fetch --all | xxd` I see only bare \n characters). All output would
appear on one line then.
Also, filter out escape sequences for character set designation; there's nothing
useful we can do with them. In practice, the only one that you are likely to see
is `ESC ( B`, which is sent as part of tput sgr0, which is sometimes used in
scripts to reset all graphics attributes to defaults.
I've optimised for muscle memory backwards compatibility here:
- Outside interactive rebase: press 'f' then instead of a confirmation
panel, a menu appears where you can choose to keep the selected commit's
message
- Inside interactive rebase: press 'f' then press 'c' to see the menu
for keeping the message, where if you press 'c' again it will retain the
current message. so 'fcc' is the chord to press.
We're also now showing the -C flag (which is what enables the behaviour)
against the todo.
I've picked the 'c' keybinding because 'C' was taken and it corresponds
to the flag. Previously that showed a warning about a change in
keybinding for cherry picking but it's been ages since we've made that
change so I'm happy to retire it.
Not used yet, we pass an empty string everywhere, to match the previous
behavior. Just extracting this into a separate commit to make the next one
smaller.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
It's been ages since we changed the key, users should hopefully be used to it by
now, and we want to reuse the key for something else later in the branch.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
On very large screens they would get ridiculously wide.
The limits we chose here are a little arbitrary:
- 80 for confirmations and prompts
- 90 for menus
- auto-wrap width plus 25 for the commit message editor (or 100 if auto-wrap is
off)
To reproduce: open the keybindings menu ('?'), press '>' to select the last
line, esc to close it, open it again. The view would appear still scrolled down,
so the selected first line was out of view. Even worse, if you open a different,
shorter menu (e.g. the "View upstream reset options..." menu in the Files
panel), you'd only see the Cancel item, the other ones were scrolled out of
view.
This is a regression introduced with efd4298b5e (#5134).
When marking commits as good or bad during a bisect, a "git rev-list" call would
appear in the Command log. This is confusing, it's an internal detail that is
not interesting for the user to see.
When opening a PR, a "git ls-remote" call would appear in the Command log. This
is confusing, it's an internal detail that is not interesting for the user to
see.
Since the reflog can get very long, this saves some memory but especially some
UI thread lag. In one of my repos I had over 11'000 reflog entries (I guess I
should prune them more regularly...), and rendering them took ~600ms; since this
happens on the UI thread, there was an annoying stall for half a second after
every background fetch, for example.
This fixes a bug in ListContextTrait.FocusLine whereby the view would go blank
when scrolling by page (using ',' or '.') in views that have
renderOnlyVisibleLines set to true but refreshViewportOnChange set to false.
Currently we don't have any such views; the only ones who use
renderOnlyVisibleLines are commits and subcommits, and they also use
refreshViewportOnChange. However, we are going to add one in the next commit,
and eventually it might be a good idea to convert all our list views to that by
default, and get rid of the renderOnlyVisibleLines flag.
Move SetContentLineCount into OverwriteLinesAndClearEverythingElse. Calling it
separately beforehand is not concurrency safe; we need both to happen
when the view's writeMutex is locked.
We have this logic to avoid constantly rerendering the main view when hitting
up-arrow when you are already at the top of the list. That's good, but we do
want to scroll the selection into view if it is outside and you hit up or down,
no matter if it changed.
It is possible to scroll the selection out of view using the mouse wheel; after
doing this, it would sometimes scroll into view by itself again, for example
when a background fetch occurred. In the files panel this would even happen
every 10s with every regular files refresh.
Fix this by adding a scrollIntoView parameter to HandleFocus, which is false by
default, and is only set to true from controllers that change the selection.
We want to do this whenever we switch branches; it wasn't done consistently
though. There are many different ways to switch branches, and only some of these
would reset the selection of all three panels (branches, commits, and reflog).
We move the selection down by the number of commits that were reverted (to keep
the same commits selected). However, this only happens after refreshing, which
has rendered the main view with the wrong commit, so we need to render it again
after moving the selection.
There are many other callers of MoveSelection in LocalCommitsController, but all
of them happen before the refresh. Revert is special because it needs to move
the selection after refreshing, e.g. when reverting the only commit of a branch.
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.
This is not really super important because we are very unlikely to assign a key
such as esc or up/down to a menu item. However, users might do this in a custom
commands menu, and in that case it is important that the builtin keys still
work; or they might remap those builtin commands to other keys, in which case
they might conflict with single-letter keys in normal menus.
The test shows two problems: a <down> keybinding is not removed from a menu item
(the 'y' binding is removed though, which is correct), and keybindings such as
'j' and 'H' don't work. We will fix both of these separately in the following
commits.
Codeberg is a Gitea-based git hosting service that uses the same URL
patterns for pull requests and commits. This adds native support so
users don't need to manually configure it.
The confirmation used to make sense back when the Open MergeTool command was its
own top-level command; however, that command was changed in 703f053a7e to open a
menu instead, and Open MergeTool is now just a submenu entry in that menu, so it
no longer needs a confirmation.
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.
We want to test the order in which the commits are listed in the error message.
For one of the tests the order is already as we want it, but for the other it's
not (we want them to show up in log order). We'll fix this in the next commit.
It is a bit generic, it seems that users sometimes set it for other reasons, and
then they are confused why they don't see anything. Use a more specific name
instead.
We move the code to push the branches context into CheckoutRef, this way it
works consistently no matter where we call it from. Previously, checking out
remote branches or tags would switch to the branches view, but checking out a
commit did not.
Note that it now also takes effect for undoing or redoing a checkout, which may
be a bit questionable; but I still think it makes sense for this, too.
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.
This doesn't really solve a pressing problem, because I guess it's unlikely that
users add spaces at the beginning or end of what they type into a prompt; but it
could happen, and in this case we almost always want to strip it. Just adding
this here for completeness while I was working on this code.
The only exception is the input prompt of custom commands, because who knows
what users want to use that input for in their custom command.
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).
Previously it was used both for the Confirm handler and the Cancel handler, as
well as for the Confirm handler of confirmation popups (not prompts). There was
no other way to do it given how wrappedConfirmationFunction was shared between
all these; but now there is. The logic is really only needed for the Confirm
handler of prompts.
This doesn't fix anything, it just makes things clearer.
And call this new helper function from both wrappedConfirmationFunction and
wrappedPromptConfirmationFunction; this gives us more flexibility to do
different things in each of those.
Introduce the 'SelectedSubmodule' struct and allow using it as a
placeholder value.
Add a corresponding test.
Update the documentation to include it among the listed placeholder
values.
When pressing '.' (next page) or ',' (previous page), the selection
now stays at the bottom or top of the viewport respectively, instead
of being centered which caused items to scroll off. If the selection is not
already on the last/first line of the view, '.'/',' moves it there without
scrolling.
This implements a special case for page navigation as suggested by
the maintainer in issue #5017, keeping the cursor position consistent
with user expectations for page-based navigation.
Fixes#5017
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
This allows for having extra slashes in git urls, for example to avoid
warnings with older bitbake fetcher implementations in yocto. Which
warns about a missing / in git urls
The "Cycle pagers" command wasn't rendered correctly, because it's bound to '|'
by default, but this was taked as a table column delimiter. Fix this by escaping
the pipe character.
A similar thing could happen for the description or tooltip (and did in fact,
for a tooltip in the russion translation), so escape those too just to be sure.
The existing diff parser incorrectly treated subsequent lines beginning
with "---" as filename headers while processing hunks. This caused
corruption when dashed lines appeared within diffs themselves.
Restrict filename detection to only occur between hunks.
When replacing the naked return with a `return result`, the linter starts to
complain about "return copies lock value: sync/atomic.Int32 contains
sync/atomic.noCopy". I suspect this is also a problem when using a naked return,
and the linter just doesn't catch it in that case. Either way, it's better to
use a pointer to ensure that the atomic is not copied.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
This will put whatever git's default merge variant is as the first menu item,
and add a second item which is the opposite (no-ff if the default is ff, and
vice versa).
If users prefer to always have the same option first no matter whether it's
applicable, they can make ff always appear first by setting git's "merge.ff"
config to "true" or "only", or by setting lazygit's "git.merging.args" config to
"--ff" or "--ff-only"; if they want no-ff to appear first, they can do that by
setting git's "merge.ff" config to "false", or by setting lazygit's
"git.merging.args" config to "--no-ff". Which of these they choose depends on
whether they want the config to also apply to other git clients including the
cli, or only to lazygit.
- Squash and FastForwardOnly are mutually exclusive, and instead of asserting
this at runtime, model the API so that they can't be passed together.
- FastForwardOnly is unused, so remove it; however, we are going to need --ff
and --no-ff in the next commit, so add those instead.
- Instead of putting the enum into the MergeOpts struct, replace the struct by
the enum. We can reintroduce the struct when we add more arguments, but for
now it's an unnecessary indirection.
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.
I can't do my usual "add the tests first, with EXPECTED/ACTUAL sections that
document the bug" method here, because the tests would hang without the bug
being fixed.
We need two different tests here: one where a cherry-picked commit simply
becomes empty "by itself", because the change is already present on the
destination branch (this was only a problem with git versions older than 2.45),
and the other where the cherry-pick stops with conflicts, and the user resolves
them such that no changes are left, and then continues the cherry-pick. This
would still fail even with git 2.45 or later. The fix is the same for both cases
though.
The tests show that the selection behavior after skipping an empty cherry-picked
commit is not ideal, but since that's only a minor cosmetic problem we don't
bother fixing it here.
Whenever git returns the error "The previous cherry-pick is now empty", we would
previously continue the rebase; this works for rebase because it behaves the
same as "git rebase --skip" in this case. That's not true for cherry-pick
though; if you continue a cherry-pick where the current commit is empty, it will
return the same error again, causing lazygit to be stuck in an endless loop.
Fix this by skipping instead of continuing; this shouldn't make a difference for
rebase, but works for cherry-pick.
Theoretically we could have a similar problem for revert (if you are trying to
revert a commit that has already been undone through some other means); this
should then be fixed in the same way with this change. However, the change is
not relevant for revert because git returns a different error in this case.
When focusing the main view, going into full screen mode by pressing '+' twice,
and then opening the search prompt ('/') or a menu (e.g. '?' or ':'), the full
screen display would switch to the focused side panel.
Fix this by always excluding popups from the window arrangement logic. No popup
should ever have any influence on how the views beneath it are laid out.
This is an object that is owned by Gui, is accessible through GuiCommon.State(),
and also passed down to GitCommand, where it is mostly needed. Right now it
simply wraps access to the Git.Paging config, which isn't very exciting, but
we'll extend it in the next commit to handle a slice of pagers (and maintain the
currently selected pager index), and doing this refactoring up front allows us
to make that change without having to touch clients.
This is likely to do bad things; for example, if the prompt is the shell command
prompt, then we would run into what looks like a deadlock bug in tcell. In other
cases, the characters in the following lines might be treated as random commands
after the prompt is confirmed.
Fixes#4890
When Git is configured with status.renames=copies, it can produce
status codes starting with "C" (copy) in addition to "R" (rename).
The file loader was only checking for "R" prefixes, causing copy
operations to be parsed incorrectly and breaking file staging.
This fix extends the status parser to handle both "R" and "C"
prefixes, ensuring proper support for Git's copy detection feature.
Fixes file staging issues when using status.renames=copies configuration.
We want to switch to have paragraphs consistently on one line, and auto-wrap
them automatically when generating Config.md.
The changes to Config.md in this commit are temporary.
Our logic to decide if a file needs to be checked out from the previous commit
or deleted because it didn't exist in the previous commit didn't work for
submodules. We were using `git cat-file -e` to ask whether the file existed, but
this returns an error for submodules, so we were always deleting those instead
of reverting them back to their previous state.
Switch to using `git ls-tree -- file` instead, which works for both files and
submodules.
It works for submodules too.
Also, pass file name and file content explicitly; the existing tests don't care
about these, but when writing tests that do, it makes them easier to understand.
Doesn't make a difference currently, since the title is either StatusTitle when
the dashboard is showing, or LogTitle when one of the branch logs is showing.
This is going to change in the next commit, though.
When cycling to the last branch log command, and then editing the config to
remove one or more log commands, lazygit would crash with an out of bounds panic
when returning to it. Fix this by resetting the log index to 0 when it is out of
bounds. (I think resetting to 0 is better than clamping, although it doesn't
matter much.)
I know that uint was chosen to document that it can't be negative (not sure why
the "8" was chosen, though). That's pointless in languages that don't enforce
this though: you can subtract 1 from uint8(0) and you'll get something that
doesn't make sense; that's not any better than getting -1. I'm not a fan of
using unsigned types in general (at least in languages like go or C++), and they
usually just make the code more cumbersome without real benefits.
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.
This makes it possible to pass it to an external diff command that is
used like a pager. An example for this can be seen in the added
documentation in the next commit.
The logic in postRefreshUpdate would only rerender the main view if the context
being updated is the current view. This is not the case when a popup is showing;
but we still want to render the main view in that case, behind the popup. This
happens for example when we refresh the Files scope, we determine that all
conflicts have been resolved and show a popup asking to continue the merge or
rebase, but the postRefreshUpdate of the Files context only happens when the
popup is already showing, so we would still see the conflict markers behind the
popup, which is rather 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.
Previously, the feedback you got when pressing "-" was just a "Checking out..."
status in the bottom line. This was both easy to miss if you are used to looking
for an inline status in the branches panel, and it didn't provide information
about which branch was being checked out, which can be annoying in very large
repos where checking out takes a while, and you only see at the end if you are
now on the right branch.
Improve this by trying to figure out which branch was the previously checked out
one, and then checking it out normally so that you get an inline status next to
it (as if you had pressed space on it). There are cases where this fails, e.g.
when the previously checked out ref was a detached head, in which case we fall
back to the previous behavior.
When you delete a branch, we call this function to determine whether we need to
ask for confirmation of not. We don't want to log this, because it's not a
command that a user would normally use as part of this operation.
In all other menus besides the keybindings menu it makes sense to hide
keybindings that match the confirmMenu binding. This is important to make it
clear which action will be triggered when you press the key.
In the keybindings menu this is different; the main purpose of that menu is not
to allow triggering commands by their key while the menu is open, but to serve
as a reference for what the keybindings are when it is not open. Because of
this, it is more important to show all bindings in this menu, even if they
conflict with the confirmMenu key.
This fixes a regression introduced in b3a3410a1a.
This is needed when remapping the confirmMenu key to, say, "y", and there's a
menu that has an item with a "y" binding. This already worked correctly (confirm
takes precedence, as desired), but it's still confusing to see the item binding.
It seems useful to have the flexibility to remap "enter" in confirmations to
"y", but keep "enter" for menus and suggestions (even though we sometimes use
menus as confirmations, but it's still good to give users the choice).
This one doesn't make a difference in practice because we don't remap the key in
tests, but if we would, then this would no longer work correctly. It's just more
correct this way.
The universal.confirm keybinding is the wrong one to use for this, we want
universal.goInto instead. They are both bound to "enter" by default, but when
remapping confirm to "y" we don't want to use that for entering worktrees.
Rebinding the universal.confirm keybinding currently doesn't make sense, because
the rebound key would also be used for editable prompts, which means you would
only be able to bind it to a ctrl key (not "y", which is desirable for some
people), and also it would allow you to enter a line feed in a branch name.
Fix this by always using enter for editable prompts.
So far, confirmations and prompts were handled by the same view, context, and
controller, with a bunch of conditional code based on whether the view is
editable. This was more or less ok so far, since it does save a little bit of
code duplication; however, now we need separate views, because we don't have
dynamic keybindings, but we want to map "confirm" to different keys in
confirmations (the "universal.confirm" user config) and prompts (hard-coded to
enter, because it doesn't make sense to customize it there).
It also allows us to get rid of the conditional code, which is a nice benefit;
and the code duplication is actually not *that* bad.
To fix the problem described in the previous commit, iterate backwards over the
stashes that we want to delete. This allows us to use their Index field.
The test shows that we are currently auto-forwarding branches even if they are
checked out by another worktree; this is quite bad, because when you switch to
that other worktree you'll see that the files that are touched by the fetched
commits are all modified (which we don't test here).
This is similar to using lazygit's Git.Paging.ExternalDiffCommand config, except
that the command is configured in git. This can be done either with git's
`diff.external` config, or through .gitattributes, so it gives a bit more
flexibility.
Many people don't understand what this means, which is apparent from the amount
of issues that got filed because of this. Let's get rid of it to avoid this
confusion. People will have to configure their pager twice if they want to use
it both on the command line and in lazygit, which I think is not a big deal.
This was needed in an earlier version of the test, when we asserted the file
content in a more complicated way. It should have been removed in caca62b89e.
This frees up ctrl-z for suspend. Hopefully, redo is not such a frequently used
operation that the change annoys people.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
In some cases we set a disabled reason but leave the text empty, so that we
don't get an error toast when the item is invoked. In such a case it looks
awkward if there is a tooltip showing "Disabled: " with no following text.
For many menus, just "Close" is fine, but some menus act more like confirmations
(e.g. the menu that appears when you cherry-pick and get conflicts); in this
case, it's good to make it more obvious that hitting esc cancels the whole
thing.
Dismissing a range selection is handled by the global escape handler for all
list views, but not for the staging and patch building views, so we need to make
the esc description dynamic for these, too.
The main reason for this is that sometimes the escape key is handled by a local
binding, in which case it appears before the '?' binding in the options status
bar, and other times it is handled by the global controller, in which case it
appeared after. Moving it to before the keybindings menu handler makes it appear
before '?' in both cases.
Also, if the window is too narrow to show all keybindings, the ones that don't
fit will be truncated, and in this case it is more important to show the esc
binding because of its context sensitivity.
This also moves the esc entry up a few positions in the keybindings menu, but I
don't think this matters much.
Sometimes there is a local and a global keybinding for the same key; if both are
configured to be shown in the options map, we should only show the local one
because it takes precedence. This happens for example for <esc> in a popup, or
for <esc> in the focused main view.
Note that this is also a problem in the keybindings menu, and we don't solve
that here.
It's maybe not totally obvious, so let's show it.
Esc now appears twice in the status bar, because the global controller also
appends its generic "Cancel". We'll fix this in the next commit.
An example for this can be seen in the next commit.
We make this a setter rather than an added constructor argument so that we don't
have to change all those contexts that don't want to make use of this.
This assertion didn't test anything useful, given that the filtered view already
has two lines. This should have been adapted in 9f0b4d0000 when we added
section headers while filtering.
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".
One reason why git diff can be very slow is when "diff.algorithm = histogram" is
being used. In this case, showing a very long single-file diff can take seconds
to load, and you'll see the "loading..." message in the main view until we got
the first lines of the diff to show. There's nothing really we can do about this
delay; however, when switching to another, shorter file (or commit) while the
"loading..." message is still showing, this switch should be instantaneous. And
it was before 0.54.0, but we broke this in 0.54.0 with 8d7740a5ac (#4782); now
users have to wait for the slow git diff command to output more text before the
switch occurs.
To fix this, don't block waiting for the process to terminate if we just stopped
it.
When showing a confirmation whose text ended with a line feed, we would make the
popup panel one line less tall than it needs to be, so it would show a scroll
bar. One example where this occurred is the very first popup that users ever see
(the "seriously you rock" message for new users); that's a pretty bad first
impression.
This happens because our code to suppress trailing newlines in views doesn't
work for styled text, and the text in confirmations is bold. This code checks if
the last character of the text is a line feed, and in this case it isn't; the
escape sequence for turning bold off comes after it.
We should really fix this by improving that mechanism, but this would require
some tricky logic, so as a quick fix, trim trailing (and leading) linefeeds from
the text that we display in a confirmation, before making it bold.
In 3a9dbf7341 we created a global /tmp/lazygit/ folder that contains the temp
directories of each running instance, to avoid polluting /tmp with multiple
folders. The problem with that approach was that the folder was created with 700
permissions, so if multiple users were using lazygit on the same machine (e.g. a
server), all users except the first one would get fatal errors on startup.
Fix this by creating temp folders containing the user's uid.
When the useHunkModeInStagingView config is on and you enter the staging view
with hunk selection enabled, it is confusing to see "a: Select hunk" in the
options view at the bottom.
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.
Now that we close a task's stdout pipe when we are done with it, it should
terminate by itself at that point, so there's no longer a need to kill it. This
way, called processes get a chance to terminate gracefully rather than being
killed with SIGKILL; in particular, this allows git to clean up its index.lock
file if it created one.
This is no longer as important now that we fixed the stale, left-over temp dirs
caused by daemon mode, but it could still be helpful in case lazygit crashes, or
if you have many instances of lazygit running.
The function that called us has just created a temp dir, and scheduled a defer
to remove it again; by calling os.Exit we short-cut this defer, and don't clean
up the temp dir. There is no reason to exit here, the calling function will
return after having called us.
Previously we would call git merge-base with the upstream branch to determine
where unpushed commits end and pushed commits start, and also git merge-base
with the main branch(es) to see where the merged commits start. This worked ok
in normal cases, but it had two problems:
- when filtering by path or by author, those merge-base commits would usually
not be part of the commit list, so we would miss the point where we should
switch from unpushed to pushed, or from pushed to merged. The consequence was
that in filtering mode, all commit hashes were always yellow.
- when main was merged into a feature branch, we would color all commits from
that merge on down in green, even ones that are only part of the feature branch
but not main.
To fix these problems, we switch our approach to one where we call git rev-list
with the branch in question, with negative refspecs for the upstream branch and
the main branches, respectively; this gives us the complete picture of which
commits are pushed/unpushed/merged, so it also works in the cases described
above.
And funnily, even though intuitively it feels more expensive, it actually
performs better than the merge-base calls (for normal usage scenarios at least),
so the commit-loading part of refresh is faster now in general. We are talking
about differences like 300ms before, 140ms after, in some unscientific
measurements I took (depends a lot on repo sizes, branch length, etc.). An
exception are degenerate cases like feature branches with hundreds of thousands
of commits, which are slower now; but I don't think we need to worry about those
too much.
This makes it easier to use the full ref in the git merge-base call, which
avoids ambiguities when there's a tag with the same name as the current branch.
This fixes a hash coloring bug in the local commits panel when there's a tag
with the same name as the checked out branch; in this case all commit hashes
that should be yellow were painted as red.
GetMergeBase is always called with a full ref, so it shouldn't need the
ignoringWarnings hack (which is about ignoring warnings coming from ambiguous
refs).
Also, separate stdout and stderr, which would also have solved the problem. We
no longer really need it now, but it's still cleaner.
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
This way we don't have to update the text and all translations every time we
bump the version.
Remove the year from the error text, it's cumbersome to update and I don't find
it very important to have in the message.
Also remove the invitation to file an issue; I don't find it very likely that we
are going to relax the minimum git requirement again.
It seems that `git for-each-ref` is a lot slower than `git tag --list` when
there are thousands of tags, so revert back to the previous method, now that we
no longer use the IsAnnotated field.
This reverts commit b12b1040c3.
Storing it in the Tag struct makes loading tags a lot slower when there is a
very large number of tags; so determine it on the fly instead. On my machine,
the additional call takes under 5ms, so it seems we can afford it.
These should have been added when we started rendering this information in
e5b09f34e0; apparently I was too lazy back then. Adding them now to guard
against breaking it in the next commit.
I'm adding these to the CRUD tests, it doesn't seem worth adding separate tests
just for these assertions.
Like message, extraField can get very long (when there are thousands of tags on
a single commit), so move it to the end; this allows us to truncate overly long
lines in the output and still get all the essential fields.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
There's no reason not to allow these.
Technically we could enable a few more, but I chose not to because some might be
surprising or confusing in filtering mode. For example, creating a fixup commit
would work (shift-F), but the newly created commit might not show up if it
doesn't match the filter. Similarly, pressing `f` to fixup a commit into its
parent would work, but that parent commit might not be visible, so users might
expect to be fixing up into the next visible commit.
We show it only if the "showPanelJumps" config is on, although the
focus-main-view command is not technically part of the panel jump keys; but it
looks similar.
I left this out originally because it's not needed for the status "dashboard"
view (except on really tiny screens); however, it *is* useful after pressing `a`
to show the all branches log, and even more so for people who use the
"statusPanelView: allBranchesLog" config. And it doesn't really hurt for the
dashboard view either, so just enable it always rather than making a distinction
which view we are showing.
This is not a behavior change, we already include these in the menu, but that's
because of a bug that we will fix in the next commit.
I find it useful to see these commands, especially for rarely-used custom
commands that you don't want to waste a keybinding on.
When entering staging (or patch building) for an added or deleted file, it
doesn't make sense to use hunk mode, because pressing space would stage/unstage
the entire file, and if the user wanted to do that, they would have pressed
space in the Files panel. So always use line mode for added/deleted files by
default, even if the useHunkModeInStagingView user config is on.
Not because it's terribly important to test here (doesn't hurt though), but
because it will be useful in the next commit for a new test we're adding there.
When shift-selecting a range of commits across a file rename in
filtering-by-path mode, the diff currently shows an added file rather than a
renamed file. Add a test that demonstrates this, we'll fix this in the next
commit.
When filtering for a file path we use the --follow option for "git log", so it
will follow renames of the file, which is great. However, if you then selected
one of the commits before a rename, you didn't see a diff, because we would pass
the original filter path to the "git show" call.
To fix this, call git log with the --name-status option when filtering by path,
so that each commit reports which file paths are touched in this commit;
remember these in the commit object, so that we can pass them to the "git show"
call later.
Be careful not to store too many such paths unnecessarily. When filtering by
folder rather than file, all these paths will necessarily be inside the original
filter path, so detect this and don't store them in this case.
There is some unfortunate code duplication between loading commits and loading
reflog commits, which I am too lazy to clean up right now.
These are very similar in that they call RunAndProcessLines on a git log command
and then process lines to create commits. Extract this into a common helper
function. At the moment this doesn't gain us much, but in the next commit we
will extend this helper function considerably with filter path logic, which
would otherwise have to be duplicated in both places.
It is good practice to use a -- argument even if no further arguments follow.
Doesn't really make a difference for this particular command though, I think.
I don't know why this function argument was added, but I don't like unnecessary
indirections, so I'm removing it as SubCommitsHelper has access to everything it
needs to do it itself.
Recycle reflog commits only for the non-filtered ones.
If we're not filtering, FilteredReflogCommits and ReflogCommits are the same. If
we then enter filtering and get reflog entries again, and pass a
lastReflogCommit, we'd recycle the previous FilteredReflogCommits, which are
unfiltered, so that's no good. Work around this by simply never using the
recycle mechanism when getting the filtered reflog commits.
We could do better by remembering what the last filter path or author was, and
only suppressing the recycling when it changed; but that's more complexity than
I want to add, so hopefully this is good enough.
Using the filtered one is probably not a good idea. It didn't do much harm
because the split of ReflogCommits and FilteredReflogCommits doesn't really work
right now (FilteredReflogCommits is always the same as ReflogCommits, even in
filtering mode), but we'll fix this in the next commit.
I can only guess what happened here: in aa750c0819, this code to manually load
the reflog commits was added, to make sorting branches by recency work when the
reflog is filtered by path. At that time we didn't have separate ReflogCommits
and FilteredReflogCommits models yet. Then, FilteredReflogCommits was introduced
(in 8822c409e2), probably for the very purpose of being able to get rid of this
again; but then it was forgotton to actually get rid of it.
The funny thing is that the introduction of FilteredReflogCommits happened in
the very next commit, 15 minutes later.
The test shows that selecting a commit before the rename shows an empty diff,
and selecting the rename itself shows an added file rather than a rename.
This broke with the introduction of the age in the stashes list in bc330b8ff3
(which was included in 0.41, so 12 minor versions ago).
This makes it work again when filtering by file, but it still doesn't work when
filtering by folder (and never has). We'll fix that next.
This was not a good idea, and it seems it has never been tested: the --name-only
and -z options don't work well together. Specifically, -z seems to simply cancel
--name-only.
This is not enough to make it work, we'll need more fixes in the next commit.
This reverts commit 50044dd5e0.
The test shows that it doesn't work, the list is empty both when filtering by
file and by folder.
I could have added test cases for these to the unit tests in
stash_loader_test.go instead, but I don't find tests that need to mock git's
output very valuable, so I opted for an integration test even though it takes
much longer to run.
Since filtering switches to half-screen mode in the local commits panel, most
people probably didn't notice, but we do also filter those other views. So when
leaving half-screen mode (but not filtering), you could switch to sub-commits or
stashes, and those would show the filtered view only after the next refresh
(e.g. after a background fetch). It's worse when leaving filtering, because this
goes back to normal screen mode, and you would often see an empty stashes panel
after that (until the next background fetch), which is quite confusing.
I also find it questionable to always switch focus to the commits panel when
entering filtering. If it is initiated from subcommits, reflog, or stashes,
maybe we want to stay there. I'm not changing this now since I'm unsure how much
people rely on the current behavior.
When exiting filtering mode while the focus is not in the local commits panel,
the call to HandleFocus would render the selection in the commits panel as if
the panel had the focus.
The call to HandleFocus was introduced recently in 4981419ba9 in an attempt to
rerender the main view correctly, but it should only have been called when local
commits is the focused context. But instead, we can use PostRefreshUpdate, which
handles this distinction.
We forgot to convert the model indices to view indices in searchModelCommits.
This needs to be done for search results to be highlighted correctly in the
"divergence from upstream" view, which adds "--- Remote/Local ---" entries, and
during a rebase, where we have "--- Pending rebase todos ---" and "--- Commits
---" which offset view indices from model indices.
I have seen cases where during a rebase (two nonModelItems) all entries in
viewIndicesByModelIndex beyond the second nonModelItem were off by 4 rather than
2 as I would expect. The only explanation I have for this is that the function
was called concurrently.
Improve this by working on a local variable and only assign to self at the end;
this is not a real fix for the concurrency issue of course, but it makes it much
less likely to be a problem in practice.
This doesn't change behavior, just makes the code a little bit easier to
understand: the outermost condition is "do we show a popup and is the mouse
event for some other view than the focused one". Only if that's true do we need
to define the isCommitMessageView function, and check whether both views belong
to the commit message editor.
Previously a click was detected as a double-click whenever the click was on the
already selected line, regardless of how long ago the last click was (or even
when it wasn't selected by clicking at all). Now that gocui supports proper
double-click detection, we can do better.
The rationale for this is the same as in the previous commit; however, for these
functions we only allow a single controller to set them, because they are event
handlers and it doesn't make sense for multiple controllers to handle them.
Trying to do this would previously have the second one silently overwrite the
first one's.
We don't currently have this in lazygit, but I ran into the situation once
during development, and it can lead to bugs that are hard to diagnose.
Instead of holding a list of functions, we could also have added a panic in case
the function was set already; this would have been good enough for the current
state, and enough to catch mistakes early in the future. However, I decided to
allow multiple controllers to attach these functions, because I can't see a
reason not to.
These are never called on the context, they only exist to satisfy the
HasKeybindings interface. But that's wrong, HasKeybindings has nothing to do
with focus handling or rendering the main view. Remove them from that interface
and add them to IController instead.
Similar to what was done in 457cdce61d, and for the same reason.
However, instead of waiting and fixing them one by one as we see them fail, I
decided to go about it more systematically. To do that, I added calls to
`time.Sleep(1 * time.Second)` in all the Shell.Commit* helper functions; this
ensures that all the commits we make get different committer time stamps, making
all these tests fail. With this I'm pretty confident that we're good now.
When entering filtering we would only call FocusLine, which takes care of
highlighting the selected line in the commits list, but not of re-rendering the
main view. HandleFocus does that.
When exiting filtering, the HandleFocus call was missing entirely.
The tests needed to be reworked a little bit to make this testable.
Now that -committerdate is the default sort order, we could get different
results for the sort order of the branches list depending on whether the commits
on both branches have the same committer time stamp (likely in an integration
test, since git time stamps have second resolution), in which case git will fall
back to alphabetical order, or not (rare, but possible), in which case master
will have the newer commit and will come first. Make this stable by forcing the
sort order to alphabetical.
We might have more tests with this problem, we'll just have to fix them one by
one as we see them fail.
At the same time, we change the defaults for both of them to "date" (they were
"recency" and "alphabetical", respectively, before). This is the reason we need
to touch so many integration tests. For some of them I decided to adapt the test
assertions to the changed sort order; for others, I added a SetupConfig step to
set the order back to "recency" so that I don't have to change what the test
does (e.g. how many SelectNextItem() calls are needed to get to a certain
branch).
When toggling the value in the UI we simply overwrite the value in UserConfig;
this would be bad if there was ever a chance that we want to write the user
config back to disk, but it is very unlikely that we can do that, because
currently we have no way to tell which parts of the config come from the global
config file and which ones come from a repo-local one.
The version choice is a little arbitrary, but see discussion at
https://github.com/jesseduffield/lazygit/pull/4559#issuecomment-2876201680.
The main reason why I'm updating the version now is that versions before 2.27
had a bug with branch sorting, where sorting by -committerdate (which will be
our default soon) would sort branches that point at the same commit in reverse
alphabetical order rather than alphabetical order. While this is only slightly
annoying but not a huge deal for users, it makes maintaining our integration
tests across versions very hard. So I wanted to update to at least 2.27 to get
around this problem, and went with 2.32 after the discussion linked to above.
The choice of which versions to run integration tests on is pretty arbitrary
too, I just picked some at random which are about 5 to 6 minor versions apart.
It's a very common pattern in the code base to have some code that we want to
run either directly, or with a confirmation, depending on some condition. In
most cases this is solved by creating a local helper function that we call
either directly or from within the HandleConfirm of the confirmation; provide a
convenience helper that makes this easier.
These were found by commenting out the initialization of the struct literal in
EnglishTranslationSet.TranslationSet(), and then running
$ punused pkg/i18n/**/*.go
Punused can be installed with `go install github.com/bep/punused@latest`.
The code was copied from StagingController in 0496e3af50, and I did add the new
text in that commit, I just forgot to adapt the code to actually use it.
If the hunk to be selected was partially scrolled offscreen, the view wouldn't
scroll enough to make it completely visible (the last line of the hunk was still
offscreen).
This is only a minimal fix for a pressing problem. The code to handle scrolling
after selection changes has lots of problems, and is also inconsistent between
list views and the patch explorer, but cleaning this up needs more time than I
have right now.
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).
While it's true that the behavior is a little different from the staging panel,
where the staged lines are actually removed from the view and in many cases the
selection stays more or less in the same place, it is still very useful to move
to the next stageable thing in the custom patch building view too.
This improves the experience when selecting a hunk generously with the mouse, by
dragging over it including some context lines above and below. Previously we
would consider the "moving end" of the selection range for whether things need
to be added or removed, but this doesn't make sense if it's a context line. Now
we consider the first actual change line that is included in the range.
It is confusing to get header lines, hunk headers, or context lines rendered as
being included in a custom patch, when including these makes no difference to
the patch.
This is only a visual change; internally, we still record these non-patch lines
as being included in the patch. It doesn't matter though; you can press space on
a header line and nothing happens.
It would probably be cleaner to only record + and - lines in the includedLines
array, but that would be a bit more work, and doesn't seem worth it.
CheckMergeOrRebase calls Refresh already. However, it does an async refresh by
default, so we must turn this into a sync refresh so that moving the selection
down by one works even for the very first commit in history. Also, we must add
an explicit call to FocusLine so that the view selection is in sync with the
model selection; previously this was taken care of by the PostRefreshUpdate call
that happens as part of a refresh.
Keep the same commit selected, by moving the selection down by the number of
cherry-picked commits. We also do this when reverting commits, and it is
possible now that we use a sync waiting status.
We also need to turn the refresh that happens as part of CheckMergeOrRebase into
a sync one, so that the commits list is up to date and the new selection isn't
clamped.
We are about to change the selection behavior when cherry-picking, and it's good
to have tests that document in what way it changes in the next commit.
For the case of creating a new branch by moving commits to it, we were using the
current (old) branch name in the stash name; change this to use the new name
instead.
Unlike moving a patch to the index, applying or reverting a patch didn't
auto-stash, which means that applying a patch when there's a modified (but
unstaged) file in the working tree would error out with the message "error:
file1: does not match index", regardless of whether those modifications conflict
with the patch or not.
To fix this, we *could* add auto-stashing like we do for the "move patch to
index" command. However, in this case we rather simply stage the affected files
(after asking for confirmation). This has a few advantages:
- it only changes the staging state of those files that are contained in the
patch (whereas auto-stashing always changes all files to unstaged)
- it doesn't unnecessarily show a confirmation if none of the modified files are
affected by the patch
- if the patch conflicts with the modified files, the conflicts were "backwards"
("ours" was the patch, "theirs" the modified file); it is more logical if "ours"
is the current state of the file, and "theirs" is the patch.
It's a little unfortunate that the behavior isn't exactly the same as for "move
patch to index", but for that one we do need the auto-stash because of the
rebase that runs behind the scenes.
The tests show that this currently fails with the confusing error message "does
not match index", regardless of whether the patch conflicts with the
modifications or not. We'll improve this in the next commit.
I don't bother adding tests for reverting a patch, as the code is basically the
same as for apply.
This is functionality that works already, we only add the test for more complete
test coverage. However, there's a detail problem, and the test demonstrates
this: we keep the stash even if there was no conflict. We'll fix this next.
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.
This was added after this PR comment:
https://github.com/jesseduffield/lazygit/pull/3276#discussion_r1469077611
> Can we do a refresh after this reset so that the screen shows that the patch
> has been cancelled? That way, if we cancel on the next popup, the screen will
> be in a valid state.
I don't understand what "cancel on the next popup" means; there is no further
popup after this code.
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
The function would return "head/branchname" when there was either a tag or a
remote with the same name.
While fixing this we slightly change the semantic of the function (and of
determineCheckedOutBranchName, which calls it): for a detached head it now
returns an empty string rather than the commit hash. I actually think this is
better.
The icon will appear when there's a tag with the same name as the current branch
(that's what we're testing here), or even when there's a remote with the same
name. I'm not adding a test for this latter case, but this was actually how I
discovered the issue.
This would crash with an index-out-of-range error.
I double-checked that all other callers of PrepareInteractiveRebaseCommand
already call getBaseHashOrRoot, so this was the only one that was broken.
I decided not to add a test for this as the scenario is not a very common one.
Instead, pass the entire Mutexes struct by pointer to controllers.
This is better because Mutexes now has a zero value and doesn't need to be
initialized.
Pretty basic fix, didn't seem to have any complications.
I only added the refs/ prefix to the FullRefName() method
to align with other similar methods, and to make this change
not impact any user facing modals.
Fixes: https://github.com/jesseduffield/lazygit/issues/4634
Also adds a test demonstrating that the stash show behavior is now fixed
Previously we would call pullFiles() from the pick() handler if we were not in a
rebase, assuming that the default keybinding for both is "p". This needn't be
the case of course, if the user has remapped one or the other.
The consequence of this was that swapping the keybindings for "pullFiles" and
"pushFiles" would work in all panels except the Commits panel (unless "pick" was
also remapped in the same way).
Fix this by using the new AllowFurtherDispatching mechanism of DisabledReasons
to pass the keybinding on to the next handler.
If a DisabledReason has its AllowFurtherDispatching flag set, it is returned as
a ErrKeybindingNotHandled error, instead of shown as a toast right away. This
allows gocui to continue to dispatch the keybinding, and we can unwrap the error
at the other end (in our global ErrorHandler) and display it then.
This allows having keybindings for the same key at the local and global levels,
and they will continue to be dispatched even if the first one returns a
DisabledReason. It is opt-in, so we only use it for cases where we know that a
local and a global handler share the same (default) keybinding.
There was no reason to declare a variable for disabledReason, assign it inside
the "if binding.GetDisabledReason != nil" statement, and then check its value
again after that if statement. Move all that code inside the first if statement
to make the control flow easier to understand.
Adaptions are for this gocui commit:
Cleanup: remove Is* error functions
- Use errors.Is instead of quality comparisons. This is better because it
matches wrapped errors as well, which we will need later in this branch.
- Inline the errors.Is calls at the call sites. This is idiomatic go, we don't
need helper functions for this.
See https://go.dev/blog/go1.13-errors for more about this.
When switching between repos, each repo might have a different focused panel; in
this case, the previously focused panel would show the "inactive" highlight. By
default this is only bold text, so it's barely noticeable, but it becomes more
pronounced when setting e.g.
gui:
theme:
inactiveViewSelectedLineBgColor:
- "#666666"
I noticed this especially when entering or leaving submodules; for example,
enter a submodule by pressing enter in the Files panel, then switch to the
Commits panel in the submodule, then press Esc to go back to the parent repo.
This would put the focus back into the Files panel, but keep the inactive
highlight in the Commits panel.
In the unlikely scenario that you have a remote branch on `origin` called
`foo`, and a local tag called `origin/foo`, git changes the behavior of
the previous command such that it produces
```
$ git for-each-ref --sort=refname --format=%(refname:short) refs/remotes
origin/branch1
remotes/origin/foo
```
with `remotes/` prepended. Presumably this is to disambiguate it from
the local tag `origin/foo`. Unfortunately, this breaks the existing
behavior of this function, so the remote branch is never shown.
By changing the command, we now get
```
$ git for-each-ref --sort=refname --format=%(refname) refs/remotes
refs/remotes/origin/branch1
refs/remotes/origin/foo
```
This allows easy parsing based on the `/`, and none of the code outside
this function has to change.
----
We previously were not showing remote HEADs for modern git versions
based on how they were formatted from "%(refname:short)".
We have decided that this is a feature, not a bug, so we are building
that into the code here.
Moving the getter of the suggested branch name to a separate function
allows us to reuse it in situations where we are not calling the regular
create new branch function, such as move commits to a new branch
Signed-off-by: Elias Assaf <elyas51000@gmail.com>
When refreshing the branches list, we have code to keep the same branch selected
even when the refresh changes the sort order; this code remembers the selected
branch before the refresh, and then tries to select it again afterwards (looking
it up by name) if it is still there.
However, we stored the previously selected branch too early, before even
obtaining the branches list; if the user moved the selection between that point
and the end of the refresh, it would jump back. Fix this by remembering the
previous selection only at the last moment, right before assigning the new
branches slice.
We still have a race condition here between the UI code that manages the
selection as the user presses arrow keys, and the background thread doing the
refresh that reads and restores the selection; however, the race was there
before, and we make it neither better nor worse with this PR. It doesn't seem to
be a problem in practice.
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.
We will need a user config in the file tree in the next commit, and passing the
entire common is the easiest way to do that while ensuring hot-reloading when
users change the config while lazygit is running.
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.
The click handler of MainViewController was registered as a global handler, so
it was used when a side panel was focused that doesn't have a
SwitchToFocusedMainViewController attached (e.g. Status, Worktrees, or
Submodules). This handler would then push the main view context, but with the
code that is meant only for toggling between the main view pair contexts, i.e.
with taking over the parentContext from the otherContext, which doesn't have one
at that point. This would later lead to a crash in onClick because the
parentContext was nil.
Fix this by splitting the click handler in two, one for when it already has the
focus, and one for toggling from the other view, and make these focus specific.
Previously we would re-render the focused main view several times during a
refresh, once for every side panel. While this should usually not be noticeable
for users because we are careful to avoid flicker when refreshing the main view,
this would sometimes lead to the main view scrolling up to the top by itself;
see PR description for more details.
This is for the unlikely case that a repo-local config file can't be written
back after migration; in this case we can't log the migration changes to the
console, so include them in the error popup instead.
This might be useful to see in general (users will normally only see it after
they quit lazygit again, but still). But it is especially useful when writing
back the config file fails for some reason, because users can then make these
changes manually if they want.
We do this only at startup, when the GUI hasn't started yet. This is probably
good enough, because it is much less likely that writing back a migrated
repo-local config fails because it is not writeable.
Most migrations happen at startup when loading the global config file, at a time
where the GUI hasn't been initialized yet. We can safely print to the console at
that point. However, it is also possible that repo-local config files need to be
migrated, and this happens when the GUI has already started, at which point we
had better not print anything to stdout; this totally messes up the UI.
In this commit we simply suppress the logging when the GUI is running already.
This is probably good enough, because the logging is mostly useful in the case
that writing back the migrated config file fails, so that users understand
better why lazygit doesn't start up; and this is very unlikely to happen for
repo-local config files, because why would users make them read-only.
It's a bit silly to find out by string comparison whether computeMigratedConfig
did something, when it knows this already and can just return the information.
This doesn't make a huge difference to the production code; the string
comparison isn't very expensive, so this isn't a big deal. However, it makes the
tests clearer; we don't have to bother specifying an expected output string if
the didChange flag is false, and in particular we can get rid of the ugly "This
test intentionally uses non-standard indentation" bit in one of the tests.
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.
This now allows for leaving the status panel and returning back to the
same log command. Previously any return to the status panel would result
in the next command in the list being shown. Now, you need to press `a`,
with a log command being rendered, to rotate to the next
allBranchesLogCmd.
In one case it was actually called *before* making a commit (when switching from
the commit message panel to committing in the editor). And clearing the
preserved message is the only thing it does, so name it after what it does
rather than when it's called.
Previously it would only clear the message if the panel had been opened with
preserveMessage=true; we don't need this check, because callers know how they
opened the panel, and whether they want to clear the message.
This is no change in behavior because OnCommitSuccess only clears the message
when the commit message panel was opened with preserveMessage=true, which it
isn't in the case of creating a tag.
And this is in fact the desired behavior, because we don't want creating a tag
to interfere with preserving commit messages in any way.
The default binding for ConfirmInEditor is <a-enter>, which has two problems:
- some terminal emulators don't support it, including the default terminal on
Mac (Terminal.app)
- on Windows it is bound to toggling full-screen
Ideally we would use <c-enter> instead (and Command-Enter on Mac), but neither
is possible without https://github.com/gdamore/tcell/issues/671, so for the time
being add an alternate keybinding which works everywhere.
Show both bindings in the footer of the commit description panel if they are
both non-null. While we're at it, fix the footer for the case where either or
both of the keybindings are set to <disabled>.
And finally, change "commit" to "submit" in that footer; we use the same panel
also for creating tags, in which case "commit" is not quite right.
It's never called, the binding ListController.HandleGotoBottom wins.
The functionality of loading more commits is implemented by GetOnFocus, and this
way it works not only for '>', but also for other navigation keys like page
down.
BeginInteractiveRebaseForCommit is used for all the patch commands, and for
rewording. It works by setting the commit we want to stop at to 'edit'; this
doesn't work for merge commits. This wasn't a problem for the patch commands so
far, because you typically don't use custom patches with merge commits (although
we don't prevent this; maybe we should?).
However, it was a problem when you tried to reword a merge commit; this
previously failed with an error, as the test added in the previous commit
demonstrated.
Also, we want to add a new patch command that has to stop *before* the selected
commit (pull patch to new commit before the original one), and this wouldn't
work for the first commit in a feature branch, because it would have to set the
last commit before that to 'edit', which isn't possible if that's a merge (which
is likely).
To fix all this, use a 'break' before the selected commit if the commit is a
merge. It is important that we only do it in that case and not always, otherwise
we would break the new regression tests that were added a few commits ago.
I almost broke this during the development of this branch, so add a test to
guard against that. The point here is that the stack remains intact, i.e. the
newly created commit is the last commit of the lower branch, and thus shows the
"*".
I almost broke this during the development of this branch, so add a test to
guard against that. The point here is that the stack remains intact, i.e. the
renamed commit is the head of the lower branch, and thus shows the "*".
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.
Hopefully, graphs will never get wider than 32768 characters. (They would get
kind of hard to navigate if they did...)
This reduces the size of the Pipe struct from 48 to 32 bytes, which makes a
significant difference when there are many millions of instances.
This saves some memory at the cost of a slight performance increase (I suppose
reallocting the slice when adding new Pipes is slightly more expensive now).
Performance of the BenchmarkRenderCommitGraph benchmark is 130μs before, 175μs
after. I'm guessing this is still acceptable.
Change the base type of some of our enums from int to uint8, and reorder fields
for better packing. This reduces the size of models.Commit from 152 to 132 bytes
on my machine.
This doesn't improve overall memory usage significantly, but why not save a
little bit of memory if it's easy.
We need to pass %P instead of %p in the format string of the git log command, so
that the parent hashes have the full length and can be shared with the real
hashes.
This in itself is not an improvement, because hashes are unique (they are shared
between real commits and rebase todos, but there are so few of those that it
doesn't matter). However, it becomes an improvement once we also store parent
hashes in the same pool; but the real motivation for this change is to also
reuse the hash pointers in Pipe objects later in the branch. This will be a big
win because in a merge-heavy git repo there are many more Pipe instances than
commits.
The "// merge commit" comment was plain wrong, this is any commit that has a
parent, merge or not. The "else if" condition was unnecessary, a plain "else"
would have been enough. But the code in the two blocks was almost identical, so
extract the one thing that was different and unify it.
And while we're at it, use IsFirstCommit() instead of counting parents.
This makes it easier to copy diff hunks and paste them into code. We only strip
the prefixes if the copied lines are either all '+' or all '-' (possibly
including context lines), otherwise we keep them. We also keep them when parts
of a hunk header is included in the selection; this is useful for copying a diff
hunk and pasting it into a github comment, for example.
A not-quite-correct edge case is when you select the '--- a/file.txt' line of a
diff header on its own; in this case we copy it as '-- a/file.txt' (same for the
'+++' line). This is probably uncommon enough that it's not worth fixing (it's
not trivial to fix because we don't know that we're in a header).
This is very old; I can only guess that this was added at a time where today's
list column handling wasn't in place yet, so the space was needed to separate
columns. This now causes a gap of two spaces between the rebase todo column and
the author column, which I'm sure wasn't intended. Funny that I never noticed.
This is very similar to what we are doing for staging or discarding hunks in the
Files panel. Git doesn't allow applying patches with a zero context size (unless
you use the --unidiff-zero option, which is discouraged).
Fish shell does not support "&&" and "||" operators like
POSIX-compatible shells. Instead, it uses a different syntax structure
based on begin/end and if/else.
This caused existing lazygit nvim-remote integration templates to break
when fish was the user's default shell.
This commit adds explicit fish shell detection using the FISH_VERSION
environment variable, and provides fish-compatible templates that
correctly handle launching Neovim or sending remote commands via $NVIM.
Fixes behavior where edits would not open in a new Neovim tab or line
navigation would fail when $NVIM was set.
Ensures smoother editing experience for users running fish shell
(supported since Nov 2012 with FISH_VERSION).
The long story: I want to call this function from RefsHelper; however, I can't
make WorkingTreeHelper a field of RefsHelper because RefsHelper is already a
field in WorkingTreeHelper, so that would be a circular dependency.
The shorter story: there's really little reason to have to instantiate a helper
object in order to call a simple function like this. Long term I would like to
get to a state where a lot more of these helper functions are free-standing, and
you pass in the data they need.
While at it, simplify the implementation of AnyStagedFiles and AnyTrackedFiles
to one-liners.
It's the same, really, except that GetCheckedOutRef() does a check if any
branches exist and returns nil if not. Since we are accessing the returned
branch unconditionally without checking for nil, it seems this check is not
needed here. (The functions we are touching here are called from handlers that
are guarded with itemSelected or singleItemSelected, so we know that at least
one branch exists.)
The goal is to get rid of the dependency to refsHelper.
And only while the task is running.
This avoids accumulating lots of blocked goroutines when scrolling a view down
more than 1024 times (the capacity of the readLines channel).
In this commit this is only possible by pressing '0' in a side panel; we'll add
mouse clicking later in the branch.
Also, you can't really do anything in the focused view except press escape to
leave it again. We'll add some more functionality in a following commit.
Previously we would render the diff for a directory to the main/secondary pair,
but a diff for a file to the staging/stagingSecondary pair. (And similar for
commit files: main/secondary for directories, but
patchBuilding/patchBuildingSecondary for files.)
I always found this confusing and couldn't really understand why we are doing
this; but now it gets in my way because I want to attach a controller to
main/secondary so that they can be focused. So change it to always use the main
context pair for everything we render from a side panel.
When rerendering a view at the end of a refresh, we call HandleFocus only if the
view has the focus. This is so that we rerender the main view for the new
selection.
What was missing here is to update the view selection from the list selection if
the view doesn't have the focus, so that the selection is painted properly.
Normally this is not relevant because you don't see the selection if another
side panel has the focus; however, you do see it as an inactive selection when
e.g. a popup is shown, in which case it does matter.
This will become more important when we introduce section headers for commits,
because in that case the view selection needs to change when the working copy
state changes from normal to rebasing or vice versa, even if the list selection
stays the same.
The changed test submodule/reset.go shows how this was wrong before: when
entering the submodule again after resetting, there is a refresh which keeps the
same branch selected as before (master); however, since the branches panel is
not focused, the view didn't notice and kept thinking that the detached head is
selected (which it isn't, you can tell by running the test in sandbox mode and
focusing the branches panel at the end: you'll see that master is selected). So
the change in this commit fixes that.
We do this because
- it's closer to what you would do on the command line
- it simplifies the code a bit
- it will allow us to support cherry-picking merge commits.
Previously we would create new Commit objects to store in the CherryPicking mode
which only contained a name and hash, all other fields were unset. I'm not sure
why we did this; it's easier to just reference the original commits. Later on
this branch we will need this because we need to determine whether a copied
commit was a merge commit (by looking at its Parents field).
We treat the .git/sequencer/todo file as read-only. Technically it seems it
would be possible to treat it as modifiable in the same way as
.git/rebase-merge/git-rebase-todo, effectively turning a cherry-pick or revert
that stops at a conflict into an interactive rebase; however, git itself doesn't
allow this (there is no "git cherry-pick --edit-todo"), so it seems safer not to
rely on it.
Theoretically it would be possible to allow modifying the rebase todos when a
cherry-pick or revert conflicts in the middle of a rebase. However, it would
introduce a bit of complexity to support this, as we would have to be able to
distinguish between rebasing todos and cherry-picking/reverting todos, which we
currently can't; it could also be a bit error-prone as far as edge cases are
concerned. And it's really a pretty uncommon situation, so it doesn't seem worth
it, and we just forbid all modifications to todos whenever we are cherry-picking
or reverting.
