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).
- **PR Description**
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.
Also, we change the visualization of what's included in the patch to
mark only the + and - lines of the patch; for all other lines it doesn't
make a difference whether they are included.
And finally, we make it so that only + and - lines are considered when
pressing space; previously it would also look at selected context lines,
which doesn't make much sense. This improves the experience for mouse
users who like to generously select hunks by dragging across them,
including some context lines above and below.
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.
- **PR Description**
For cherry-picking, this used to work in earlier versions, but it broke
in #4443. For reverting, it was never supported.
Also, we add some minor improvements while we're at it, such as slightly
better names for the auto-stashes that are created for the various
operations (so that, if an auto-stash pop fails and the stash is kept
around, you can tell more easily what it was for). Also, we now adjust
the selection of the commits list after cherry-picking, so that the same
commit stays selected.
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.
It seems to be excluded already when you run the lint.sh script, but in VS Code
when setting Lint on Save to package it would still lint a file in gocui when
you save it, which is annoying. (Remove the other paths that were there before;
they seem to be unused, and they were added by the auto-migration.)
Unfortunately, gopls will still lint gocui files with its builtin staticcheck
linter, and I couldn't find a way to turn this off. This might be a reason to
turn off staticcheck in gopls (not sure yet).
- **PR Description**
Applying or reverting a custom patch when one of the files contained in
the patch had unstaged modifications in the working tree would fail with
the confusing error message "does not match index", regardless of
whether those modifications conflicted with the patch or not. You would
expect this to just work if there are no conflicts, or to get the usual
conflict markers if there are. It was possible to work around this by
manually staging those files, but few people knew about this. Fix this
by staging the files (after asking for confirmation).
Also, fix a minor problem where an auto-stash for the "move patch to
index" command was forgotten to be dropped.
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.
- **PR Description**
In the Stash panel, show information about the selected stash above the
diff of the stash in the main view. This is useful for stash entries
with very long names that are not fully visible in the Stash panel.
Fixes#4662.
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.
- **PR Description**
This will format the `z` keyboard shortcut reference in this text in the
same way as all other keyboard shortcuts in the README.md file.
- Update the golangci-lint configuration to use the latest version
(2.2.1)
- Enable a few more linter checks
- Improve the VS Code configuration so that we don't get spurious linter
warnings that wouldn't also show up on CI
- Provide a way to run the exact same golangci-lint version locally that
we use on CI. This is useful in case the globally installed version is a
newer one which emits more warnings.
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
... and configure it to run the same checks as golangci-lint.
It may seem redundant to have two linter checks enabled; however, there are
reasons to have both. golangci-lint is what we are running on CI, and it has a
lot more linters than gopls, so we want to run it locally too to get the same
coverage. The linter that is built into gopls is only staticcheck, so that's
redundant; but it has the advantage that the warnings appear a bit faster
(golangci-lint runs only on save, and with a bit of delay), and in particular it
provides quick fixes for some of the checks, which is quite convenient.
- **PR Description**
In the Tags panel, show information about the selected tag above the
commit log. This is mostly useful for annotated tags.
Fixes#4659.
- **PR Description**
Fix the problem that if the `rebase.updateRefs` git config is on, a
branch icon would appear in the commits list for the head commit as soon
as a remote or tag exists whose name is the same as the name of the
current branch.
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.
- **PR Description**
When GPG signing is enabled but overrideGpg is false, attempting to
reword a non-latest commit would show the "Feature not vailable for
users using GPG" error only after the user had already entered the
commit message.
Now the error is displayed immediately when attempting to start the
reword operation, before opening the editor.
Closes#4611
- **PR Description**
Moving a custom patch from the very first commit of the history to a
later commit 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.
Fixes#4624.
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.
- **PR Description**
Here's a minor cleanup: instantiate mutexes by value so that they don't
have to be initialized explicitly. It is always preferable for structs
to have a valid zero value, and this is one small step in that
direction.
I read this recommendation in the [Uber style
guide](https://github.com/uber-go/guide/blob/master/style.md#zero-value-mutexes-are-valid),
and it makes a lot of sense to me. I find most of the rest of this style
guide to be a very good read, too.
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.
- **PR Description**
Pretty basic fix, didn't seem to have any complications. I basically
just grepped for `stash{` and all of the references seemed like they
could benefit from the addition. 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. I added one integration
test for the super obvious failure behavior. I didn't feel that it was
worth it to create duplicate integration tests for the other behaviors
of drop, apply, pop, etc. I did manually test them though. If
interested, I could add the creation of the `stash` branch to all the
existing tests of stash behavior, just to prove they continue to work
under those conditions. I just didn't do that on the first pass cause I
could see how that could take away from the core behavior the tests are
trying to demonstrate.
Fixes: https://github.com/jesseduffield/lazygit/issues/4634
- **PR Description**
Improve the dispatching of key bindings so that remapping "pullFiles" to
a different key works correctly in the Commits panel.
Fixes#4614.
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.
- **PR Description**
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.
```yml
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.
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.
- **PR Description**
Allows the reset menu to have a different name that is displayed, and a
fully qualified name that git will unambiguously know what it refers
about. We could totally squash this back down to 1 input, and display to
the user the _precise_ full ref name that we are resetting to, but I
think the context they are in (branches tab versus tag tab), means that
we don't need to do that, and can continue to just show the branch name
and the tag name to the end users.
Fixes https://github.com/jesseduffield/lazygit/issues/4569
- **PR Description**
When pressing N to move new commits to a new branch we get greeted with
an empty prompt, this PR makes it so we fill the empty prompt with a
suggestion taken from branchPrefix, similar to the good old create a new
branch.
- **PR Description**
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.
Fixes#4116.
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.
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.
- **PR Description**
In #4346 we added a `/` root item in the Files and CommitFiles panels
whenever there is more than one top-level item. We made it
unconditional, but I promised to add a config as soon as users ask for
being able to disable it. For a while I was able to convince users who
asked for it that it is useful and they don't want to turn it off, but
now there's a [stronger
request](https://github.com/jesseduffield/lazygit/discussions/4590#discussioncomment-13254924)
from someone who refuses to upgrade to the current version, and we don't
want that.
So, add a config option `gui.showRootItemInFileTree` that is true by
default.
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.
- **PR Description**
The status view is not supposed to be focusable right now. (This might
change soon, but for now it isn't.) Pressing '0' on it does nothing.
However, clicking on it would still focus it, because the click handler
in MainViewController assumed that when the clicked view doesn't have
the focus, then its "other" view must have, and we just want to toggle
the focus between the two (like when pressing tab). It didn't take the
possibility into account that the current side panel isn't focusable at
all; if it was, then its SwitchToFocusedMainViewController would have
handled the click.
To fix this, check if the "other" view has the focus before handling the
click, and do nothing otherwise.
This also fixes clicking in the main views of the Worktrees or
Submodules tabs, or any other tabs whose main views are not focusable.
Fixes#4566.
Stefan Haller
