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 "*".
