It said "Press tab to toggle focus", which is wrong for people who remapped
their togglePanel key binding to something else. Print the actual key binding
instead.
As discussed in https://github.com/jesseduffield/lazygit/pull/2599, it
makes more sense to have the user specify whether they want verbose
commits from their own git config, rather than lazygit config.
This means that we can remove all the code (including test coverage)
associated with the custom verbose flag, and lazygit will just inherit
the .gitconfig settings automatically.
We have a use-case to rebind 'm' to the merge action in the branches panel. There's three ways to handle this:
1) For all global keybindings, define a per-panel key that invokes it
2) Give a name to all controller actions and allow them to be invoked in custom commands
3) Allow checking for merge conflicts after running a custom command so that users can add their own 'git merge' custom command
that matches the in-built action
Option 1 is hairy, Option 2 though good for users introduces new backwards compatibility issues that I don't want to do
right now, and option 3 is trivially easy to implement so that's what I'm doing.
I've put this under an 'after' key so that we can add more things later. I'm imagining other things like being able to
move the cursor to a newly added item etc.
I considered always running this hook by default but I'd rather not: it's matching on the output text and I'd rather something
like that be explicitly opted-into to avoid cases where we erroneously believe that there are conflicts.
It seems that older git versions would drop empty commits when rebasing. Since
this aspect is not relevant to what we're testing here, fix this by simply
avoiding empty commits in these tests.
The test apply_in_reverse_with_conflict.go fails in git versions 2.30.8 and
earlier. Apparently the output "Applied patch to 'file2' cleanly" was only added
more recently. It's not essential that we check this output.
Save has been deprecated for a while, push is the recommended way to save a
stash. Push has been available since 2.13, so we can use it without problems.
Older versions of git don't support the -b option yet. However, no version of
git complains about the -c option, even when the init.defaultBranch config is
not supported.
For older git versions we won't be able to support any other main branch than
"master", so hard-code that in Init.
This doesn't fix anything for older versions yet; see the next commit for that.
Now that we are running each test 6 times on CI, the risk of flakiness
is higher. I want to fix these tests for good but it'l take time, so
we're just retrying for now
The code in getHydratedRebasingCommits relied on the assumption that the
git-rebase-todo file contains full SHAs. This has only been true from 2.25.2 on,
before that it would contain abbreviated SHAs. Fix this by storing fullCommits
in a slice instead of a map, and using a linear search.
I don't know why we're getting index.lock errors but they're impossile to stop
anyway given that other processes can be calling git commands. So we're retrying
a few times before re-raising. To do this we need to clone the command and the current
implementation for that is best-effort.
I do worry about the maintainability of that but we'll see how it goes.
Also, I thought you'd need to clone the task (if it exists) but now I think not;
as long as you don't call done twice on it you should be fine, and you shouldn't
be done'ing a task as part of running a command: that should happen higher up.
It's not clear what was happening but it seemed like we sometimes weren't
fully writing to our stdout buffer (which is used for the error message)
even though we had returned from cmd.Wait().
Not sure what the cause was but removing an unnecessary goroutine fixed it.
I've simplifiied the code because it was too complex for the current requirements, and this fixed the misc/initial_open
test which was occasionally failing due to a race condition around busy tasks
The global counter approach is easy to understand but it's brittle and depends on implicit behaviour that is not very discoverable.
With a global counter, if any goroutine accidentally decrements the counter twice, we'll think lazygit is idle when it's actually busy.
Likewise if a goroutine accidentally increments the counter twice we'll think lazygit is busy when it's actually idle.
With the new approach we have a map of tasks where each task can either be busy or not. We create a new task and add it to the map
when we spawn a worker goroutine (among other things) and we remove it once the task is done.
The task can also be paused and continued for situations where we switch back and forth between running a program and asking for user
input.
In order for this to work with `git push` (and other commands that require credentials) we need to obtain the task from gocui when
we create the worker goroutine, and then pass it along to the commands package to pause/continue the task as required. This is
MUCH more discoverable than the old approach which just decremented and incremented the global counter from within the commands package,
but it's at the cost of expanding some function signatures (arguably a good thing).
Likewise, whenever you want to call WithWaitingStatus or WithLoaderPanel the callback will now have access to the task for pausing/
continuing. We only need to actually make use of this functionality in a couple of places so it's a high price to pay, but I don't
know if I want to introduce a WithWaitingStatusTask and WithLoaderPanelTask function (open to suggestions).
We had some test flakiness involving the index.lock file which is fixed by this commit.
We shouldn't be accessing newTaskID without the mutex, although I'm surprised that this
actually fixes the issue. Surely we don't have tasks (which typically render to the main
view) which use index.lock?
We had a race condition due to refreshing branches in two different places, one which refreshed reflog commits
beforehand. The race condition meant that upon load we wouldn't see recency values (provided by the reflog commits)
against the branches
I want to see how we go removing all retry logic within a test. Lazygit should be trusted to tell us when it's no longer busy,
and if it that proves false we should fix the issue in the code rather than being lenient in the tests
Turns out we're just running our refresh functions one after the other which isn't ideal but we can fix that separately.
As it stands this wait group isn't doing anything.
I don't know if this is a hack or not: we run a git command and increment the pending action
count to 1 but at some point the command requests a username or password, so we need to prompt
the user to enter that. At that point we don't want to say that there is a pending action,
so we decrement the action count before prompting the user and then re-increment it again afterward.
Given that we panic when the counter goes below zero, it's important that it's not zero
when we run the git command (should be impossible anyway).
I toyed with a different approach using channels and a long-running goroutine that
handles all commands that request credentials but it feels over-engineered compared to this
commit's approach.
The remote branches controller was using its own escape method meaning it didn't go through the flow of cancelling
an active filter. It's now using the same approach as the sub-commits and commit-files contexts: defining a parent
context to return to upon hittin escape.
Given that we now persist search/filter states even after a side context loses focus, we need to make it really
clear to the user that the context is currently being searched/filtered
This is a pickle: initially I wanted it so that a filter would cancel automatically if the current context lost focus.
But there are situations where you want to retain the focus, e.g. when a popup appears, or when you view the commits
of a branch. The issue is that when you view the commits of a branch, the branches context is removed from the context
stack. Even if this were not the case, you could imagine going branches -> sub-commits -> files -> sub-commits, where
in that case branches would definitely be off the stack upon navigating to the files context.
So because I'm too lazy to find a proper solution to this problem, I'm just making it so that filters in side contexts
are retained unless explicitly cancelled.
There's another edge case this commit handles which is that if I'm in the sub-commits context via the branches context
and start a search, then navigate to the reflog context and hit enter to get to the sub-commits context again, I need
to cancel the search before I switch. Likewise with the commit files context.
The first line of the diff pane would show branch heads (e.g.
commit dd9100ccc8b69a8b14b21a84e34854b5acfb871a (mybranch, origin/mybranch)
only when a pager is used. The reason is that the default of the --decorate
option to git show is "auto", which means to show the decoration only when
output goes to a tty. Lazygit uses a pty only when a pager is used, so the
decoration wouldn't show when no pager is used.
Since the branch head annotation is useful and we always want to see it, force
it by explicitly passing --decorate.
This solves three problems:
1. When the local main branch is behind its upstream, the merged status of
commits of a feature branch sitting on origin/main was not correct. This can
easily happen when you rebase a branch onto origin/main instead of main, and
don't bother keeping local main up to date.
2. It works when you don't have the main branch locally at all. This could
happen when you check out a colleague's feature branch that goes off of
"develop", but you don't have "develop" locally yourself because you normally
only work on "main".
3. It also works when you work on a main branch itself, e.g. by committing to it
directly, or by merging a branch locally. These local commits on a main
branch would previously be shown in green instead of red; this broke with
910a61dc46.
For consistency with the previous commit.
Note that this menu entry is used both for unstaged and for staged changes, and
for staged changes it is not quite accurate, as we are not discarding changes in
that case (just unstaging them). Not sure it's worth fixing this; it's still
better than "Delete", anyway.
The title was saying "Unstage lines", which was just wrong. The text said
"Delete lines", which can be seen as a bit misleading; we are only discarding
the changes to the selected lines, not deleting the lines themselves.
For consistency, rename the config variable skipUnstageLineWarning accordingly.
The assert package is already very good at displaying errors, including printing
a diff of expected and actual value, so there's no point in printing the same
information again ourselves.
This test is almost identical to swap_in_rebase_with_conflict.go, except that it
sets the commit that will conflict to "edit".
This test is interesting because there's special code needed to determine
whether an "edit" command conflicted or not, i.e. whether to show the "confl"
entry. In this case we do. We have lots of other tests already that have "edit"
commands that don't conflict, so that's covered already.
When stopping in a rebase because of a conflict, it is nice to see the commit
that git is trying to apply. Create a fake todo entry labelled "conflict" for
this, and show the "<-- YOU ARE HERE ---" string for that one (in red) instead
of for the real current head.
This test is interesting because it already behaves as desired: since git has
rescheduled the "pick" command, we do _not_ want to show a "conflict" entry in
this case, as we would see the same commit twice then.
We don't actually use it to do map lookups; we still iterate over it in the same
way as before. However, using a map makes it easier to patch elements; see the
next commit.
We use CommitFilesController also for the files of commits that we show
elsewhere, e.g. for branch commits, tags, or stashes. It doesn't make sense to
discard changes from those (for stashes it might be possible to implement it
somehow, but that would be a new feature), so we disallow it unless we are in
the local commits panel.
Discarding changes to an entire directory doesn't quite work correctly in all
cases; for example, if the current commit added files to the directory (but the
directory existed before) then those files won't be removed.
It might be possible to fix the command so that these cases always work for
directories, but I don't think it's worth the effort (you can always use a
custom patch for that), so let's display an error for now.
I don't know why we were setting the initial context to CurrentSideContext
and not just CurrentContext in the first place. If there is no current context
in either case it'll default to the files context. So the only issue is if
we anticipated that some random context would be focused and we didn't want to
activate that. But I can't think of any situation where that would happen.
Whenever we perform an action in a test, we should assert on the result before doing the next action.
This prevents issues where the test moves too fast for our code. It would be nice to not have to do this,
but for now that's the situation
A better refactor would be to allow matchers to assert against either a string or a slice of cells, so that I could have
the same ergonomics that I have elsewhere, but this is a start.
The root commit is special in that it has no parents. So we need to add a pipe that's headed for a commit
that doesn't actually exist i.e. the mythical empty tree commit. We're using the actual hash of that
pseudo-commit, but it's not being read anywhere.
The menuFromCommand option is a little complicated, so I'm adding an easy way to just use the command output directly,
where each line becomes a suggestion, as-is.
Now that we support suggestions in the input prompt, there's less of a need for menuFromCommand, but it probably still
serves some purpose.
In future I want to support this filter/valueFormat/labelFormat thing for suggestions too. I would like to think a little more
about the interface though: is using a regex like we currently do really the simplest approach?
We have not been good at consistent casing so far. Now we use 'Sentence case' everywhere. EVERYWHERE.
Also Removing 'Lc' prefix from i18n field names: the 'Lc' stood for lowercase but now that everything
is in 'Sentence case' there's no need for the distinction.
I've got a couple lower case things I've kept: namely, things that show up in parentheses.
I'll be honest, for all I know logging should be global in general: it is
a pain to pass a logger to any struct that needs it. But smart people on the
internet tell me otherwise, and I do like the idea of not having any global
variables lying around.
Nonetheless, I often need to log things when locally debugging and that's a
different kind of logging than the kind you would include in the actual
released binary. For example if I want to log something from gocui, I would
rather not have gocui depend on lazygit's logging setup.
Go really doesn't like us doing anything inheritance-y: it does not support open recursion meaning
it's really hard to re-use code. As such, here we're falling back to conditional logic.
This fixes an issue where our ListContextTrait was calling FocusLine which was intended to be
overridden by ViewportListContextTrait, but the subclassed function wasn't being called. I'm
not actually sure how this went wrong given that it was working fine in the past, but at any rate,
the new code is easy to follow.
Missed a spot a couple PR's ago. We had an integration test which caught this but which was skipped due
to index.lock file issues. The test was also broken for other reasons due to it not having been running
for a while, so I've fixed that up too.
By constructing an arg vector manually, we no longer need to quote arguments
Mandate that args must be passed when building a command
Now you need to provide an args array when building a command.
There are a handful of places where we need to deal with a string,
such as with user-defined custom commands, and for those we now require
that at the callsite they use str.ToArgv to do that. I don't want
to provide a method out of the box for it because I want to discourage its
use.
For some reason we were invoking a command through a shell when amending a
commit, and I don't believe we needed to do that as there was nothing user-
supplied about the command. So I've switched to using a regular command out-
side the shell there
If a given menu item has an associated keybinding of 'enter', hitting enter won't actually execute
that item unless your cursor is on it. This creates confusion, and so we're going to use a strikethrough
style to communicate that the keybinding is reserved for something else.
The reason for this is that now our labels for navigation keybindings are larger so they
take up more realestate. It's not the kind of thing a user needs to be told anyway,
anybody is going to try out hjkl and the arrow keys when a TUI opens up.
We could map from <up> to the single character up unicode rune but given you can rebind this stuff
I'd rather keep it simple
The only exception is when moving a custom patch for an entire commit to an
earlier commit; in this case the source commit becomes empty, but we want to
keep it, mainly for consistency with moving the patch to a later commit, which
behaves the same.
In all other cases where we rebase, it's confusing when empty commits are kept;
the most common example is rebasing a branch onto master, where master already
contains some of the commits of our branch. In this case we simply want to drop
these.
The option doesn't have any affect in these views, so we don't need to toggle it
here. But the problem was the HandleFocus call at the end: this would activate
the wrong view, so we need to avoid it here.
Show an error if the user tries to turn the option on, to let them know that it
doesn't work here.
It's not possible to reliably stage things into a custom patch when "ignore
whitespace" is on, so always treat it as off here (like we do in the staging
panel).
It looks like this is a regression that was introduced in 8edad826ca.
It defaults to {"master", "main"}, but can be set to whatever branch names
are used as base branches, e.g. {"master", "devel", "v1.0-hotfixes"}. It is
used for color-coding the shas in the commit list, i.e. to decide whether
commits are green or yellow.
Our refresh code may try to push a context. It does this in two places:
1) when all merge conflicts are resolved, we push a 'continue merge?' confirmation context
2) when all conflicts of a given file are resolved and we're in the merge conflicts context,
we push the files context.
Sometimes we push the confirmation context and then push the files context over it, so the user
never sees the confirmation context.
This commit fixes the race condition by adding a check to ensure that we're still in the
merge conflicts panel before we try escaping from it
