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.
What happens here is that when stopping on an "edit" todo entry, we rely on the
assumption that if the .git/rebase-merge/amend file exists, the command was
successful, and if it doesn't, there was a conflict. The problem is that when
you stop on an edit command, and then run a multi-commit cherry-pick or rebase,
this will delete the amend file. You may or may not consider this a bug in git;
to work around it, we also check the existence of the rebase-merge/message file,
which will be deleted as well by the cherry-pick or revert.
This problem can't happen inside of lazygit itself right now, but this will
change in the future. It will only happen when you stopped in an interactive
rebase on an "edit" entry, and then you perform a revert or cherry-pick
consisting of more than one commit; in this situation lazygit will show a
conflict although there is none.
This is not possible with lazygit yet, as we don't support range-select for
reverting, and we don't use `git cherry-pick` for cherry-picking. Both will
change in the future, so it's good to fix this bug.
MergeRebasingCommits already merges the rebasing commits into the commits slice
that is passed in, so it doesn't make sense to append the result to commits
again. It isn't a problem, but only because commits is always empty.
It is useful to see if the conflicted commit was a "pick" or an "edit". What's
more, we're about to add support for showing cherry-picks and reverts, and
seeing that a conflicted commit was a revert is important because its diff is
backwards compared to the diff of the conflicting files in the Files panel.
This is equivalent in the current state of the code, but it will no longer be
after the next commit, because we will introduce a new status value
StatusConflicted. And in a later PR we might add yet another value
StatusCherryPicking to distinguish rebase todos from cherry-pick todos; using
commit.IsTODO is a safer way to check whether a commit is any of these.
When you are in the middle of a rebase, and you cherry-pick a commit which
conflicts, it helps to be clear on whether you are prompted to continue the
cherry-pick or the rebase.
It looks like enums.go was supposed to be file that collects a bunch of enums,
but actually there's only one in there, and since it has methods, it deserves to
be in a file of its own, named after the type.
The situation is that you perform a rebase, and one of the commits becomes empty
during the rebase, in a way that couldn't be predicted before the rebase
started. To explain: git rebase has some logic where it immediately discards
commits if it can tell that they already exist in the branch being rebased onto;
it does this by looking at the patch ids of the commits to work out which
commits already exist upstream. But for those commits that become empty even
though there isn't a corresponding commit upstream, git stops with an error, and
lazygit detects this (in CheckMergeOrRebaseWithRefreshOptions) and automatically
continues the rebase.
This all works fine; I'm adding this test because I almost broke this during
development of this branch, so I'm adding it to guard against regressions.
- Remove REBASE_MODE_NORMAL. It is not the "normal" mode anyway, rather a legacy
mode; we have removed support for it in eb0f7e3d02, so there's no point in
representing it in the enum.
- Remove distinction between REBASE_MODE_REBASING and REBASE_MODE_INTERACTIVE;
these are the same now.
- Rename StatusCommands.IsInInteractiveRebase to IsInRebase.
- Remove StatusCommands.RebaseMode; use StatusCommands.IsInRebase instead.
All test cases set it to enums.REBASE_MODE_NONE, so we can simplify things a
little bit by hard-coding that. This makes the changes in the following commits
a little easier.
When debugging an integration test that involves some behind-the-scenes
rebasing, the daemon lazygit would also wait for a debugger to attach. This is
very confusing because the test seems to hang; once you figured out what's going
on, it's inconvenient because you need to attach a debugger to the daemon every
time you debug the test.
Now, it would sometimes be useful to be able to debug the daemon itself (whether
inside an integration test, or during normal usage), and I have often wished to
be able to do that. We might introduce an additional env var (and command-line
option?) to enable this; but that's out of scope here.
For #, !, [, and ], the problem is that it doesn't ignore the file because the
special characters need to be quoted. For *, the problem is that it ignores too
much (it also hides the abc_def file because the * is treated as a glob).
It is shown either when committing with `w`, or when typing the skipHooks prefix
if there is one. This should hopefully make it clearer when the hooks are run
and when they are not.
We removed prefilling the skipHook prefix in b102646b20 with the intention of
making it clearer that using the prefix in normal commits and typing `w` to skip
hooks are now two independent features.
It turns out that some people liked it with prefilling the prefix and perceive
it as a regression, so put it back in.
But only if we don't have a preserved message; this is an important use case,
when you try to make a normal commit, the hook fails, and then you want to make
the same commit with skipping the hook, but with the same message that you
already typed.
This makes it possible to use date and time in initial values like this:
```yaml
initialValue: 'ruudk/{{ runCommand "date +\"%Y/%-m\"" }}/'
```
I want to use this to configure my BranchPrefix like this:
```yaml
git:
branchPrefix: 'ruudk/{{ runCommand "date +\"%Y/%-m\"" }}/'
```
For the less common conflict types DD, AU, UA, DU, and UD, we would previously
only show "* Unmerged path" in the main view, which isn't helpful. Also, for
some of these we would split the main view and show this text both in the
unstaged changes and staged changes views, which is a bit embarrassing.
Improve this by offering more explanation about what's going on, and what the
most likely way to resolve the situation is for each case.
We keep the same commit selected (even though its index changed because of the
added update-ref todo), which is nice; however, the main view shows the diff of
the wrong commit, which is very confusing. I'm suprised that this hasn't been
noticed yet.
The reason why this happens is that we first do the refresh, which includes
re-rendering the main view diff (with the same commit index as before, so the
wrong one), and then we restore the correct commit index but don't render the
main view again.
I can only guess here: maybe they were added to more clearly document the public
interface of the classes? If so, I don't think that works. Developers who are
not familiar with the convention will just add a new public method to the class
without updating the interface.
Apparently this was an attempt at working around go's lack of default arguments,
but it's very unidiomatic and a bit confusing. Make it a normal parameter
instead, so all clients have to pass it explicitly.
There is a section at the end with deprecated settings, and a comment saying
"The following configs are all deprecated". The clipboard-related settings were
accidentally added to that section; they are not deprecated, so move them up to
before that section.
The current rules for discarding submodule changes is that no other changed item
must be also selected. There are some bugs with the current implementation when
submodules are in folders.
For example, selecting and discarding a folder with only a nested submodule
change will currently do nothing. The submodule changes should be discarded. The
folder only contains submodule changes so it should be no different than
pressing discard on the submodule entry itself.
Also, I noticed range selecting both the folder and the submodule and then
pressing discard would be incorrectly disallowed.
Renaming a file inside the same directory shows it with its full path in the
tree view, which isn't what we want. We'll fix this in the next commit.
Also adding a few other test cases for moving files; they show that the display
of moved files in tree view isn't ideal. For example, moving file1 from top
level into dir shows it as "R file1 → file1", which isn't distinguishable from
renaming file1 inside dir. I suppose what we would like to have here is
"R ../file1 → file1" or something, but I'll leave that for the future; here I
only want to fix the regression that was introduced with the root item PR.
In 8b8343b8a9 we made a change to run newPtyTask from AfterLayout; this is
needed so that the PTY gets the new, updated view size. However, this created a
race condition for integration tests that select a line in a list view and then
expect the main view to have certain content; sometimes that content gets
rendered too late.
I'm surprised that this didn't cause more tests to fail; right now I only know
of one test that occasionally fails because of this, which is stash/rename.go.
Fix this by moving the AfterLayout to inside newPtyTask, and do it only when we
are actually using a PTY (we don't when no pager is configured, which is the
case for integration tests).
The diff is best viewed with "ignore whitespace" turned on.
The test shows a misbehavior: even though the diff shows "-one" and "+three",
meaning that "three" is the state we want to check out, we get "one". The reason
is that the checkout file command doesn't pay attention to range selections, it
only looks at the "moving end" of the range. Had we created the range by
selecting "two" and then pressed shift-up to "three", we would have gotten the
expected result.
The actual usage is case insensitive, so this doesn't actually matter.
But if fills my heart with joy.
The test is case sensitive, but the actual response to `git config
commit.gpgSign` is equivalent to `git config commit.gppsign`
It's not an ideal solution because there's no indication of what kind of objects
you can add to those maps or arrays, but at least they show up at all (with a
comment containing a link to more information), and that's already an
improvement.
Add verify flag
Add and update integration tests
Rename verify to forceSkipHooks
Adapt CommitSkipHooks integration test to actually use a hook
Remove forceSkipHooks param from OnConfirm et al
Simplify tests
Assert the entire lines using Equals instead of Contains. This makes the tests a
bit easier to read, and it makes it much easier to decide how they need to be
changed when we change the layout (like we do in the last commit of this
branch).
It is true that this requires changing all these tests for any future UI
changes, but I think this is a good price to pay; those adaptions are trivial
and can be done without thinking.
I'm pretty sure that the check for the main view was meant to be done with the
commit selected in the commits panel, not with the first file of the commit
files view selected, so it was pressing enter too early. It's pure coincidence
that the test worked.
This is useful for copying the entire content of the selected file as it was at
the selected commit.
We only add it to the commit files panel; it is not needed in the files panel,
because there you can simply press "e" to edit the file.
This allows changing git config values while lazygit is running (e.g. in a
different terminal tab, or even in lazygit's ":" shell prompt), and have them
take effect immediately, while still getting some benefit from caching them
while lazygit is in the foreground.
Before we changed the commitPrefix config to a list in #4261, we had this bug
where the defaults section in Config.md would erroneously list the default for
commitPrefix as
git:
commitPrefix:
pattern: ""
replace: ""
This was not correct, commitPrefix was a pointer, and the default for that was
nil, which is not the same.
Now, some people copied and pasted the entire defaults section into their config
files, setting the commitPrefix to an empty pattern (which is not the same as
not setting it at all). And this caused the branch name to be filled in to the
subject field for every commit; see for example
https://github.com/jesseduffield/lazygit/discussions/3632.
New users copying the defaults section into their config file in the current
version no longer have this problem because now that commitPrefix is a list, it
is no longer included in the defaults section. However, the migration that we
added in #4261 would happily carry over those empty strings into a list entry,
so people migrating from an older version still have the broken config in their
config files.
To work around the issue, ignore commit prefix settings whose pattern is an
empty string. I can't imagine a valid reason why people would actually want to
set the pattern to an empty string, so I assume this only comes from the broken
defaults problem described above.
Some operating systems 'open' implementations do not like
when some special characters are unencoded, so they will
double-enconde the branch name, which we already encode.
This particularly matters since branch names with / are common
Rewording a commit at the beginning of a long branch can take very long;
without this change, the commit message panel would stay visible with a blinking
cursor during that time, which is very confusing.
This has the slight downside that it will say "Rebasing" in the lower right
corner until the operation is done; but we already have this problem when doing
custom patch operations, or dropping changes from a commit, so it's not new, and
we can think about how to fix all these another time.
WithGpgHandling already does an async refresh when done, so there's no need to
do one here for the case of amending the head commit. On top of that,
WithGpgHandling uses WithWaitingStatus and works in the background, so the
Refresh here would come too early anyway.
All this does is clear the preserved commit message; however, we open the commit
message panel with PreserveMessage: false when rewording, so this is not
necessary.
For some reason the //nolint:golint,unused comment no longer seems to work after
I updated gopls (I think).
But I also don't understand why we mark unused stuff with linter comments
instead of just getting rid of it.
Previously we only filtered them out from the example config section in
Config.md, but they still appeared in the schema. This is not ideal, because the
schema descriptions can appear in editors on mouse hover or in auto-completions.
So filter them out earlier, so that they don't appear in the schema either.
Some people have post-checkout hooks that take a lot of time, which makes
discarding changes slow. You can argue that a post-checkout hook should only run
when you switch branches, so it doesnt't have to run when checking out single
files or directories. You can also argue that lazygit might have implemented
discarding changes by taking the current patch and applying it in reverse, which
wouldn't have run a post-checkout hook either.
So disable them for all cases where we use git checkout with a path; this
includes checking out a file from the commit files view.
This seems to be what most people use when indenting yaml files, and it seems to
make more sense than the default of 4.
We also use it the example config in Config.md.
In ff4ae4a544 we changed the order of the calls to render before selecting the
branch. This was done only to save an extra call to ReApplyFilter, which is done
by refreshView; I claimed that the order of refreshView vs. SetSelectedLineIdx
doesn't matter here. I guess I was wrong about that, it makes the integration
test custom_commands/suggestions_preset.go flaky. To fix this, put the
refreshView call back to where it was (after the SetSelectedLineIdx call), and
instead insert an extra call to ReApplyFilter where necessary to fix the bug
that ff4ae4a544 was trying to fix.
This is very similar to the same menu in the Files panel, except that it works
on whatever diff is currently shown in the main view, including range diffs
either in diffing mode (shift-W), or from a range selection of commits.
Change our fake clipboard command to not append a linefeed; that's closer to
what the production code does.
This allows us to use Equals instead of Contains for checking the clipboard
contents.
Finally, use FileSystem().FileContent() to assert the clipboard contents,
instead of selecting the clipboard file and then checking the diff view.
This may lead to unrelated processes being killed on Windows (https://github.com/jesseduffield/lazygit/issues/3008). Imagine:
1. lazygit is started and runs git diff in process X which completes immediately and exits.
2. lazygit is left in the background for several hours by which process X pid is reused by an unrelated process.
3. lazygit is focused back on and runs another git diff. It first runs this stop logic which will kill process X and its children.
Previously the schema only allowed a single value; however, it is now possible
to specify multiple values separated by comma, and you would get very ugly red
error squiggles in VS Code when you did that.
The only solution that I can see is to get rid of the "enum" specification, and
mention the valid values only in the description. Add examples too so that you
get at least auto-completion.
When pasting a multi-line commit message into the subject field of the commit
editor, we would interpret the first newline as the confirmation for closing the
editor, and then all remaining characters as whatever command they are bound to,
resulting in executing all sorts of arbitrary commands.
Now we recognize this being a paste, and interpret the first newline as moving
to the description.
Also, prevent tabs in the pasted content from switching to the respective other
panel; simply insert four spaces instead, which should be good enough for the
leading indentation in pasted code snippets, for example.
Jesse's comment from https://github.com/jesseduffield/lazygit/issues/4237:
We recently added a new option to check out a commit's branch from within the
commits, reflog, and sub-commits panels:
https://github.com/user-attachments/assets/0a5cf3f2-6803-4709-ae5a-e4addc061012
After using it for some time, I find it annoying that the default option has
changed. I rarely find myself wanting to check out a branch from the commits
panel, and it's rarer still to want to check out a branch from the reflog and
sub-commits panel. Although there may be use cases for this, it is jarring that
something you can always do (checkout the commit) is harder to do than something
that you can sometimes do (checkout the branch).
We've also had a user complain (see
https://github.com/jesseduffield/lazygit/pull/4117) about their muscle-memory
being broken by the recent change, and I have also fallen victim to this. I
don't think that the new branch checkout option is sufficiently useful to
dislodge the existing keybinding, so let's swap them.
The code that tries to reselect the same branch again uses GetItems, which in
case of filtering is the filtered list. After replacing the branches slice with
a new one, the filtered list is no longer up to date, so we must reapply the
filter before working with it. It so happens that refreshView does that, so
simply call that before setting the selection again; I don't think the order
matters in this case. Otherwise we'd have to insert another call to
ReApplyFilter before the call to GetItems, which we can avoid this way.
Note that this doesn't actually make anything work better in the case of
deleting a branch, since we can't reselect the deleted branch anyway of course.
But it avoids a possible crash if the branch that was deleted was the last one
in the unfiltered list.
This includes the "only conflicting" status that the user can't switch to
themselves. We display it anyway to give a hint that files are being filtered,
and to let them know that they can turn the filter off if they want to.
I renamed the "Reset filter" item to "No filter" to make it look more like a
state than an action, so that it fits the radio button concept better.
When there are conflicts and we set the filter to show only conflicting files,
then none of the radio buttons light up, which is slightly strange. I guess it's
ok though.
We don't need to maintain additional state to allow this; all we need to do is
take over the filter only when the number of conflicting files goes from zero to
non-zero, rather than every time it is non-zero.
The only problem is that we don't allow users to go back to showing only
conflicted files, but that's just because we don't have that as an entry in the
menu. And I don't think it's a problem.
This handles the situation where the user's own config says to not show
untracked files, as is often the case with bare repos managing a user's
dotfiles.
Git diff and patch doesn't work reliably with a context size of 0, so disable it
in this case (and discarding changes as well). Magit does the same, see
https://github.com/magit/magit/issues/4222.
Staging entire files by pressing space in the Files panel is still possible, of
course.
This was recently introduced, but it was done the wrong way.
WithWaitingStatusSync should only be called from the main thread, and it is
meant to be used for updating the bottom line while the UI is blocked. It is a
bad idea to call this from a background thread, and it results in ugly flicker
(occasionally).
Use the newly extracted WithWaitingStatusImpl instead, this is the same as
WithWaitingStatus (which is exactly what we need) but without the implicit
OnWorker, which we don't want because we are on a background thread already.
This shows a status as if the user had typed 'f' manually in the files panel.
I want this particularly for the first fetch after startup. There are often
situations where I need to wait for this first background fetch to be done
before I can do what I want (e.g. rebase my branch onto its base branch, or
check out a branch that my coworker has told me they just pushed), but currently
it's hard to tell when that is.
For every subsequent background fetch after the first one it is less important,
but it hopefully doesn't hurt, and it might be nice to have some visual
indication that background activity is happening.
As far as I can tell, this is not needed. The call to Refresh at the end of
backgroundFetch takes care of redrawing after refreshing.
The call was added in 2fc1498517, that's a long time ago, and we had multiple
big refactorings since then. Maybe it was needed back then but no longer is
today.
It has fields .To and .From (the hashes of the last and the first selected
commits, respectively), and it is useful for creating git commands that act on a
range of commits.
When pushing a branch that didn't have an upstream yet, we use the command line
git push --set-upstream origin HEAD:branch-name
The HEAD: part of this is too unspecific; when checking out a different branch
while the push is still running, then git will set the upstream branch on the
newly checked out branch, not the branch that was being pushed. This might be
considered a bug in git; you might expect that it resolves HEAD at the beginning
of the operation, and uses the result at the end.
But we can easily work around this by explicitly supplying the real branch name
instead of HEAD.
For editable views it is important to actually show the blank line so that we
can put the cursor there for typing.
This fixes problems with adding blank lines at the end of longer commit
messages.
We used to automatically color branches starting with "feature/", "bugfix/", or
"hotfix/". For those who don't want this, it's a bit non-obvious to turn off,
but it's actually pretty easy to configure manually for those who want this, so
we just remove this default coloring.
We use an interactive shell so that users can use their custom shell aliases in
lazygit's shell prompt, which is convenient; however, this only really works for
shells like bash or zsh. We know it doesn't work for fish or nushell (because
these use different names for the $? variable); so use an interactive shell only
if the user's shell is either bash or zsh.
There are two ways to jump to the editor on a specific line: pressing `e` in the
staging or patch building panels, or clicking on a hyperlink in a delta diff. In
both cases, this works perfectly in the unstaged changes view, but in other
views (either staged changes, or an older commit) it can often jump to the wrong
line; this happens when there are further changes to the file being viewed in
later commits or in unstaged changes.
This commit fixes this so that you end up on the right line in these cases.
When pressing `e` on line 5 in a diff of an older commit, we expect it to take
us to line 5 in that file. But we end up on line 2, because the file had further
changes both in newer commits, and in the unstaged changes of the working copy.
This updates gocui to include https://github.com/jesseduffield/gocui/pull/68 and
https://github.com/jesseduffield/gocui/pull/69, which changes views to not have
an extra blank line at the end when content ending in a newline character is
written to them. This makes text views more consistent with list views, which
don't have a blank line after the last list entry either.
The easiest way to do that is to rename the local branch after pushing.
This shows various levels of brokenness for the reset and rebase to upstream
commands: both menu entries display the wrong upstream branch name in the menu
(the local one rather than the remote one); executing the rebase command works
correctly though, the rebase command uses the right branch name. Resetting
fails, though.
We'll fix this in the next commit.
This makes it possible to "silently" disable a keybinding. The effect is the
same as putting the check in the handler and returning nil from there, except
that doing it this way also hides it from the bottom line if DisplayOnScreen is
true.
It should have been half-screen from the get-go. I think I just used
full-screen to make demos look nicer. Now that we have a CLI arg for the
screen mode we can make use of that in the demos.
Introduce a new "screen-mode" command line argument that allows a user
to specify which screen mode (normal, half or full) Lazygit should use
when it runs.
This argument will take precedence over a default Window Size specified
in user config.
When the user checks out a commit which has a local branch ref attached
to it, they can select between checking out the branch or checking out
the commit as detached head.
In 5a3049485c we changed the execution of shell commands to use an interactive
shell (-i), because this allows users to use aliases or shell functions, which
is a nice convenience.
Since then, however, many users have reported problems with lazygit not coming
back to the foreground after executing a shell command. Some users report that
appending "; exit" to the end of the command line solves this. I don't really
understand what the cause of this problem was, or why appending "; exit" solves
it, but if it helps, let's do it.
Sometimes we populate the commit message panel with a pre-created commit
message. The two cases where this happens is:
- you type `w` to commit, in which case we put the skipHookPrefix in the subject
- you have a commitPrefix pattern, in which case we match it against the branch
name and populate the subject with the replacement string if it matches
In either case, if you have a preserved commit message, we use that.
Now, when you use either of these and then cancel, we preserve that initial,
unchanged message and reuse it the next time you commit. This has two problems:
it strips spaces, which is a problem for the commitPrefix patterns, which often
end with a space. And also, when you change your config to experiment with
commitPrefix patterns, the change seemingly doesn't take effect, which can be
very confusing.
To fix both of these problems, only preserve the commit message when it is not
identical to the initial message.
This makes it so that when the staging view is resized, we keep the same patch
line selected (as opposed to the same view line, which may correspond to a
different patch line after resizing). It doesn't seem like a terribly important
feature for resizing the window, but it is essential when initially entering the
staging view: we select the first line of the first hunk in this case, but we do
that before layout runs. At layout time the view is then split into
unstaged/staged changes, and if this split is horizontal, the view gets narrower
and may be wrapped in a different way. With this commit we ensure that the first
line of the first hunk is still selected after that.
So far, lines in the view corresponded 1:1 to lines in the patch. Once we turn
on wrapping for the staging view (which we don't do yet), this is no longer
true, so we need to convert from view lines to patch lines or vice versa all
over the place.
This doesn't improve the code much in the current state, but we'll add some more
code to this helper function in the next commit, which makes it worth it.
We haven't needed this before since we were only using the function for text in
confirmations and menus, which is unlikely to contain tabs. We are going to use
it for patches in the staging view though, which often do.
to make it more generally usable by clients other than ConfirmationHelper, which
we will do later in this branch. Rename it to WrapViewLinesToWidth while we're
at it.
Add tests; in particular, add a sanity check that we wrap lines the same way as
gocui does. The tests that are added here are the same ones as in gocui for its
lineWrap function, but we'll extend them a bit in later commits in this branch.
This is also what we do in the staging controller, and it makes it so that when
you exit the patch building view and then enter it again (for another file, or
the same one) we select the first hunk again.
The test demonstrates that the behavior is undesirable right now: we move the
commit only past the update-ref todo of branch1, which means the order of
commits stays the same and only the branch head icon moves up by one. However,
we move the selection down by one, so the wrong commit is selected now. This is
especially bad if you type a bunch of ctrl-j quickly in a row, because now you
are moving the wrong commit.
There are two possible ways to fix this:
1) keep the moving behavior the same, but don't change the selection
2) change the behavior so that we move the commit not only past the update-ref,
but also past the next real commit.
You could argue that 1) is the more desirable fix, as it gives you more control
over where exactly the moved commit goes; however, it is much trickier to
implement, so we go with 2) for now (and that's what the commented-out
"EXPECTED" section documents here). If users need more fine-grained control,
they can always enter an interactive rebase first.
For non-merge commits we change "pick" to "drop" when we delete them. We do this
so that we can use the same code for dropping a commit no matter whether we are
in an interactive rebase or not. (If we aren't, we could just as well delete the
pick line from the todo list instead of setting it to "drop", but if we are, it
is better to keep the line around so that the user can change it back to "pick"
if they change their mind.)
However, merge commits can't be changed to "drop", so we have to delete them
from the todo file. We add a new daemon instruction that does this.
We still don't allow deleting a merge commit from within an interactive rebase.
The reason is that we don't show the "label" and "reset" todos in lazygit, so
deleting a merge commit would leave the commits from the branch that is being
merged in the list as "pick" commits, with no indication that they are going to
be dropped because they are on a different branch, and the merge commit that
would have brought them in is gone. This could be very confusing.
One of the comments we are deleting here said:
// Comparing just the hash is not enough; we need to compare both the
// action and the hash, as the hash could appear multiple times (e.g. in a
// pick and later in a merge).
I don't remember what I was thinking when I wrote this code, but it's nonsense
of course. Maybe I was thinking that the hash that appears in a "merge" todo
would be the hash of the commit that is being merged in (which would then
actually appear in an earlier pick), but it isn't, it's the hash of the merge
commit itself (so that the rebase can reuse its commit message). Which means
that hashes are unique, no need to compare the action.
So far it didn't have to handle the case where one hash is empty and the other
isn't, but in the next commit we need that, so let's handle that case correctly.
There's enough logic in the function now that it's worth covering it with tests.
Not much of a change in behavior, because moving merge commits was already not
possible. However, it failed with a cryptic error message ("Todo fa1afe1 not
found in git-rebase-todo"), so disable it properly instead.
When enabled, it adds "+n -m" after each file in the Files panel to show how
many lines were added and deleted, as with `git diff --numstat` on the command
line.
Opening links containing ampersands inside lazygit (a pull-request
creation page in BitBucket Server, for instance) returns the following
Powershell error:
> The ampersand (&) character is not allowed. The & operator is reserved
> for future use; wrap an ampersand in double quotation marks ("&") to
> pass it as part of a string.
We fix it by enclosing the URL in single quotes.
The OS command to open file in explorer in WSL doesn't currently work as
expected; it always opens the file explorer at the default opening
location. This is because the {{filename}} variable returns the path in
WSL format, and not in the format expected by Windows.
We use wslpath, a utility shipped with WSL, to make the path conversion.
It is needed by both call sites of this function. This has the added benefit
that the argument doesn't unnecessarily show up in the status view when diffing
mode is on.
In 67b8ef449c we changed the "edit" command to insert a "break" after the
selected commit, rather than setting the selected todo to "edit". The reason for
doing this was that it now works for merge commits too.
Back then, I claimed "In most cases the behavior is exactly the same as before."
Unfortunately that's not true, there are two reasons why the previous behavior
was better (both are demonstrated by tests earlier in this branch):
- when editing the last commit of a branch in the middle of a stack of branches,
we are now missing the update-ref todo after it, which means that amending the
commit breaks the stack
- it breaks auto-amending (see the added test earlier in this branch for an
explanation)
For these reasons, we are going back to the previous approach of setting the
selected commit to "edit" whenever possible, i.e. unless it's a merge commit.
The only scenario where this could still be a problem is when you have a stack
of branches, and the last commit of one of the branches in the stack is a merge
commit, and you try to edit that. In my experience with stacked branches this is
very unlikely, in almost all cases my stacked branches are linear.
We will need this because under some conditions we are going to use this
function to edit a range of commits, and we can't set merge commits to "edit".
This corresponds to the code in startInteractiveRebaseWithEdit which has similar
logic.
It is a bit unfortunate that we will have these two different ways of setting
todos to edit: startInteractiveRebaseWithEdit does it after stopping in the
rebase, in the Then function of its refresh, but InteractiveRebase does it in
the daemon with a ChangeTodoActionsInstruction. It still makes sense though,
given how InteractiveRebase works.
This not only affects "edit", but also "drop", "fixup", and "squash".
Previously, when trying to use these for a range selection that includes a merge
commit, they would fail with the cryptic error message "Some todos not found in
git-rebase-todo"; now they simply exclude the merge commit. I'm not sure if one
is better or worse than the other, and we should probably simply disable the
commands when a merge commit is selected, but that's out of scope in this PR.
This is very similar to edit_range_select_outside_rebase.go, except that it
selects commits right after, and including, a merge commit.
This test already works correctly. The reason we add it is that we are going to
have two different implementations of the `e` command depending on whether the
last selected commit is a merge commit, and we want to make sure they both work
with a range selection.
Auto-amending is a little-known feature of git that is very convenient once you
know it: whenever you stop at a commit marked with `edit` in an interactive
rebase, you can make changes and stage them, and when you continue the rebase
they automatically get amended to the commit you had stopped at. This is so
convenient because making changes to a commit is one of the main reasons why you
edit a commit.
Unfortunately this currently doesn't work in lazygit because we don't actually
use `edit` to stop at the first commit (instead, we add a `break` todo after it,
which doesn't have the auto-amend functionality).
We'll improve this later in this branch.
We allow deleting remote branches (or local and remote branches) only if *all*
selected branches have one.
We show the a warning about force-deleting as soon as at least one of the
selected branches is not fully merged.
The added test only tests a few of the most interesting cases; I didn't try to
cover the whole space of possible combinations, that would have been too much.
Since we want to select multiselections, this will make it easier to pass a
slice of remote branches. It does require that for the case of the local
branches panel we need to synthesize a RemoteBranch object from the selected
local branch, but that's not hard.
Mouse wheel scrolling of the custom patch view worked *unless* a file (as
opposed to a directory) is selected in the commit files view. The reason was an
obvious typo in the AttachControllers call.
This code had a lot of logic that (fortunately) didn't work because it was
buggy:
- it was supposed to wait for the auto-fetch delay before fetching for the first
time in case we start with a repo that we had open in a previous session (i.e.
that appears in the recent repos list). This code actually ran always, not
just for known repos, because the IsNewRepo flag is only set later, after this
function runs. Fortunately, the code didn't work, because time.After starts a
timer but doesn't wait for it (to do that, it would have to be
`<-time.After`).
- if the first fetch fails with error 128, it was supposed to show an error
message and not start the background fetch loop. Fortunately, this didn't work
because 1) it was guarded by isNew which is always false here, and 2) because
git's error message in this case is actually "exit code: 128", not "exit
status 128" (maybe this has changed in git at some point).
I find both of these undesirable. Whenever I open a repo I want an auto-fetch to
be triggered immediately to get my branch information up to date as quickly as
possible. And if the initial fetch fails (e.g. because one of my remotes is
offline or doesn't exist any more), then that's no reason not to start the
auto-fetch loop anyway.
So let's simplify the code to do what it did before, but with much fewer lines
of code.
Original commit message of the gocui change:
This fixes View.Size, Width and Height to be the correct (outer) size of a view
including its frame, and InnerSize/InnerWidth/InnerHeight to be the usable
client area exluding the frame. Previously, Size was actually the InnerSize (and
a lot of client code used it as such, so these need to be changed to InnerSize).
InnerSize, on the other hand, was *one* less than Size (not two, as you would
have expected), and in many cases this was made up for at call sites by adding 1
(e.g. in calcRealScrollbarStartEnd, parseInput, and many other places in the
lazygit code).
There are still some weird things left that I didn't address here:
- a view's lower-right coordinates (x1/y1) are one less than you would expect.
For example, a view with a 2x2 client area like this:
╭──╮
│ab│
│cd│
╰──╯
in the top-left corner of the screen (x0 and y0 both zero) has x1/xy at 3, not
4 as would be more natural.
- a view without a frame has its coordinates extended by 1 on all sides; to
illustrate, the same 2x2 view as before but without a frame, sitting in the
top-left corder of the screen, has coordinates x0=-1, y0=-1, x1=2, y1=2. This
is highly confusing and unexpected.
I left these as they are because they would be even more of a breaking change,
and also because they don't have quite as much of an impact on general app code.
When creating a PR against a selected branch (via O = "create pull request
options"), the user will first be asked to select a remote (if there is more
than one). After that, the suggestion area is populated with all remote branches
at that origin - instead of all local ones. After all, creating a PR against a
branch that doesn't exist on the remote won't work.
Please note that for the "PR is not filed against 'origin' remote" use case
(e.g. when contributing via a fork that is 'origin' to a GitHub project that is
'upstream'), the opened URL will not be correct. This is not a regression and
will be fixed in an upcoming PR.
Fixes#1826.
As far as I understand, this was needed back when the staging context was still
responsible for rendering its highlight (as opposed to the gocui view, as it is
today). It was necessary to call it with isFocused=false so that it removed the
highlight. The isFocused bool is no longer used today (and we'll remove it in
the next commit), so there's no need to render the view here any more.
This fixes flickering when leaving the staging view by pressing escape. The
reason is that in this case the patch state was already set to nil by the time
we get here, so we would render an empty view for a brief moment.
On top of that, it fixes unwanted scrolling to the top when leaving the staging
view. The reason for this is that we have code in layout that scrolls views up
if needed, e.g. because the window got taller or the view content got shorter
(added in #3839). This kicked in because we emptied the view, and scrolled it
all the way to the top, which we don't want.
This way it won't scroll to the top; we want this when entering the staging
panel or the patch building panel by clicking into the view, and also when
returning from these views by pressing escape. Note that there's a bug in this
latter case: the focused panel still scrolls to the top when hitting escape, we
will fix this in the next commit.
Change it in the same way for NewRenderStringWithScrollTask, just for
consistency, although it's not really necessary there. We use this function only
for focusing the merge conflict view, and in that case we already have an empty
task key before and after, so it doesn't change anything there.
When clicking in a single-file diff view to enter staging (or custom patch
editing, when coming from the commit files panel), and then pressing shift-down
or shift-up to select a range, it would move the selected line rather than
creating a range. Only on the next press would it start to select a range from
there.
This is very similar to the fix we made for pressing escape in 0e4d266a52.
When clicking in the main view to enter staging, and then pressing shift-down to
select a range, it moves the selection rather than selecting a two-line range.
We'll fix this in the next commit.
So far we only had tests that called Click() only once. If you have a test that
calls Click twice (we'll add one in the next commit), you'll notice that the
second click is interpreted as a drag because the mouse button wasn't released
in between. Fix this by sending a "mouse-up" event after the click.
After pasting commits once, we hide the cherry-picking status (as if it had been
reset), and no longer paint the copied commits with blue hashes; however, we
still allow pasting them again. This can be useful e.g. to backport a bugfix to
multiple major version release branches.
The string literal "\uf0868" does *not* create a single rune with the code point
f0868, as was intended; instead, it creates two runes, one with the code point
f086, followed by the character '8'.
Currently we try to delete a branch normally, and if git returns an error and
its output contains the text "branch -D", then we prompt the user to force
delete, and try again using -D. Besides just being ugly, this has the
disadvantage that git's logic to decide whether a branch is merged is not very
good; it only considers a branch merged if it is either reachable from the
current head, or from its own upstream. In many cases I want to delete a branch
that has been merged to master, but I don't have master checked out, so the
current branch is really irrelevant, and it should rather (or in addition) check
whether the branch is reachable from one of the main branches. The problem is
that git doesn't know what those are.
But lazygit does, so make the check on our side, prompt the user if necessary,
and always use -D. This is both cleaner, and works better.
See this mailing list discussion for more:
https://lore.kernel.org/git/bf6308ce-3914-4b85-a04b-4a9716bac538@haller-berlin.de/
It's maybe not very common, but it's totally possible for a remote branch to
have a different name than the local branch. This test shows that we don't
support this properly when deleting the remote branch.
This might seem controversial; in many cases the client code gets longer,
because it needs an extra line for an explicit `return nil`. I still prefer
this, because it makes it clearer which calls can return errors.
I can only guess, but I think this was a typo (or a copy-paste-o) when this code
was written. It was introduced in 55af07a1bb, and I think the defer was kept by
accident; if it had been on purpose, then the statement would have been put
right after the Lock call.
This solves several problems that arise from opening a menu while the prompt is
open. We might try to solve these in a different way, e.g. by dismissing the
search prompt before opening a menu, but restricting what you can do while the
prompt is open seems like the more robust fix.
To achieve this, we
- call resetKeyBindings both when opening and when closing the search/filter
prompt
- change the keybindings to only contain the ones for the search prompt when
that context is active.
So far, lazygit has always auto-staged files as soon as the conflict markers
disappeared from them, which means that we could rely on any file that still had
a status of "UU" to still contain conflict markers.
We are going to make the auto-staging optional in the next commit, and in that
case the user will want to manually stage "UU" files; so we must now check
whether the file contains conflict markers, and disallow the staging in that
case.
We make the name of the GetSelectedRefRangeForDiffFiles very specific on purpose
to make it clear that this is only for switching to diff files, so the
implementations can make assumptions about that (unlike GetSelectedRef, which is
used for different purposes and needs to stay more generic).
Right now it doesn't do very much yet, but it's still worth it even in this
state, I'd say. The function is going to become a bit longer in the next commit,
though.
In other views that show lists of commits (reflog and stash) it doesn't make
sense to show a range diff of selected entries because they don't form a linear
sequence, so we keep the previous behavior of showing the diff for the free end
of the selection range in those view.
The same applies to the commits view if the selection range includes rebasing
todos; these can have an arbitrary order, and a range diff doesn't make sense
for those.
Hopefully this will help alleviate the problem that diffing mode is sticky, and
you often forget that it's still on.
Make it magenta like the mode text in the information view.
This fixes two problems:
- Set the title ref to the commit description (like
SwitchToDiffFilesController.enter does), rather than just the hash
- SetTitleRef only sets the title of the DynamicTitleBuilder, but doesn't set it
on the view; this only happens in ContextMgr.Activate, so you'd have to switch
to a different side panel and then back to see the new title.
The three nested `if` statements may looks strange, and you might wonder why we
don't have single one with &&. The answer is that later in this branch we will
add an `else` block to the middle one.
In the next commit it will be reused for checking whether we need to reset the
patch; and extracting it makes it easier to extend it for non-sticky diff ranges
later in the branch.
viewFiles is only called from enter; it doesn't make much sense to fill in a
SwitchToCommitFilesContextOpts struct to pass it to viewFiles for this one call.
Simply inline viewFiles into enter and get rid of all that.
I found this indirection confusing when reading the code. It looks like
SwitchToDiffFilesController is instantiated with different such contexts, but
it's always Contexts.CommitFiles, so just use that directly.
Note that this doesn't only affect the diff views, which are the ones where we
want this, but also the status view, where the plan was to keep them underlined
always for better discoverability. We could make this configurable dynamically
(by adding another flag to ViewUpdateOpts), but actually I think it's fine this
way.
Any newly loaded custom command coming from the per-repo config file should add
to the global ones (or override an existing one in the global one), rather than
replace all global ones.
We can achieve this by simply prepending the newly loaded commands to the
existing ones. We don't have to take care of removing duplicate key assignments;
it is already possible to add two custom commands with the same key to the
global config file, the first one wins.
If a `t.FileSystem().FileContent("file.txt", Equals("bla"))` assertion fails
because the file doesn't exist, the error would say
Expected path 'file.txt' to not exist, but it does
which is very confusing.
We fixed one specific scenario where this happened ealier in this branch, but in
case there are more that we don't know about yet, at least make sure we don't
crash.
I don't know what this condition is supposed to guard against, or whether we
really need it (it was added in 06ca71e955, and the commit message of that
commit only says "fix bug"). But if we do need it, then it seems that `>=` is
more correct than `>`.
It is already possible to search a filtered list by searching first, and then
enabling a filter, so I found it inconsistent to not allow searching when you
are already filtering. One reason for not allowing this might be that the search
status (on the left) hides the filter status (on the right), but if we think
that's enough reason to not allow both at the same time, then we should cancel a
search when we enter filtering.
There are many situations where this can arise. Some examples are:
- the terminal window is small, and you are showing a view that shows more
content than fits into the view port, and the view is scrolled all the way
down; now you resize the terminal window to a taller size. Previously, the
scroll position of the view would stay the same, so it would add blank space
at the bottom; now it will scroll to fill that blank space with content
- expandFocusedSidePanel is on, you go to the bottom of a list view, now switch
to a different panel, then scroll that (now unfocused) panel all the way down
with the scroll wheel; now you focus that panel again. It becomes larger
because of the accordion behavior, but would show blank space at the bottom.
And probably others that I can't remember right now. I only remember that I
always found it confusing to look at a view that had blank space at the bottom
even though it had more content to scroll into view.
It might seem cleaner to integrate this into the text style system, so that you
could say `ts := ts.Url("some link")` and then `ts.Sprint("my text")`. However,
this would require adding a new field to TextStyle, which I didn't want to do.
It is just unexpected and confusing when it isn't.
There's something weird about the cursor position when scrolling it out of view;
it will be shown clamped to the visible area of the view (as if it had moved in
the opposite direction than the scroll direction), but then when you type
something again, or just move the cursor with the arrow keys, the view will jump
back to where the cursor really was. This looks confusing, and it might be
reason enough not to allow scrolling the view at all.
It doesn't seem to be used by anything, it looks like we only need to implement
the method so that the IController interface is satisfied. But still, it doesn't
hurt to be correct here, and might avoid confusion in the future when somebody
does try to use the method.
Since onNewRepo calls resetKeybindings, which reinitializes the keybindings for
custom commands, all we have to do for this is store a pointer to a config
instead of storing the customCommands, so we get the up-to-date ones every time.
It still doesn't update the view immediately when reloading the config, only the
next time the status panel is focused. But at least it does it then; writing the
code to update the panel when reloading the config is probably not justifiable.
This allows having per-repo config files with different languages per repo. Now
granted, this is not an important use case that we need to support; however, the
goal is to eventually make all configs hot-reloadable (as opposed to loading
them only once at startup), so this is one step in that direction.
For now we only support .git/lazygit.yml; in the future we would also like to
support ./.lazygit.yml, but that one will need a trust prompt as it could be
versioned, which adds quite a bit of complexity, so we leave that for later.
We do, however, support config files in parent directories (all the way up to
the root directory). This makes it possible to add a config file that applies to
multiple repos at once. Useful if you want to set different options for all your
work repos vs. all your open-source repos, for instance.
Currently, userConfig is only read once at startup and then never changes. Later
in this branch, we will add the possibility to reload the user config; this can
happen either when switching repositories, or when the user has edited the
config file. In both cases, reloading happens on the main thread, but the user
config could be accessed concurrently from background threads, so we need to
make this safe.
At the moment, the user config is only read once at startup, so there's no point
in writing it back to disk. However, later in this branch we will add code that
reloads the user config when switching repos, which does happen quite a bit in
integration tests; this would undo the changes that a test made in its
SetupConfig function, so write those changes to disk to prevent that from
happening.
This makes it more explicit how to deal with the different types of config
files: a user-supplied config file (via the LG_CONFIG_FILE env var) is required
to exist, whereas the default config file will be created if it is missing.
We will later extend this with repo-specific config files, which will be skipped
if missing.
Split it so createAllViews instanciates the views, and sets those properties
that are independent of the user config, and configureViewProperties which sets
those things that do depend on the user config. For now we call the second right
after the first, but later we'll call configureViewProperties after reloading
the user config.
It was added in 043cb2ea44, and the commit message was "reload config whenever
returning to gui". I don't understand what this means; Run() is called exactly
once after startup, so it would just reload the config again for no reason.
We will add a real way of reloading the config whenever it has changed later in
this branch.
We are going to make a few changes to the fields in this branch, and we can make
them with more peace of mind when we can be sure they are not accessed from
outside this package.
In practice, using path seems to work too, since Windows seems to be capable of
dealing with a path like C:/x/y instead of C:\x\y; but it's cleaner to do this
properly.
When clicking in a single-file diff view to enter staging (or custom patch
editing, when coming from the commit files panel), you needed to press escape
twice to exit, where the first press would seemingly do nothing.
The reason for this was that after clicking in the diff we end up in non-sticky
range select mode, but only with a single line selected, which is basically
indistinguishable from line select mode.
SelectedCommit is context-dependent and points to SelectedLocalCommit,
SelectedReflogCommit, or SelectedSubCommit depending on which panel is active.
If none of these panels is active, it returns the selected local commit, which
is probably the most useful default (e.g. when defining custom commands for the
Files panel).
The comments that I'm deleting here explain why we need the bool; however, in
our case that's a theoretical issue. It would only arise if we ever were to pass
a nil context to SetParentContext, which we never do.
The folder custom_commands contained tests for both custom commands (the ones
you configure in config.yml) and shell commands (the ones you execute at the ":"
prompt). I always found this confusing, so separate these into two different
folders.
Also, use the user's shell (from the SHELL env variable) instead of bash. Both
of these together allow users to use their shell aliases or shell functions in
the interactive command prompt.
This change reduces the number of calls during application startup to
one, calling GetRepoPaths() earlier than previously and plumbing the
repoPaths struct around to achieve this end.
- Introduced a new optional user config command, allBranchesLogCmds
- When pressing 'a' in the Status view, cycle between non-empty, non-identical log commands
- There will always be at least one command to run, since allBranhesLogCmd has a default
- Update documentation & write an integration test
- Update translation string
The current behaviour when creating a new branch off of a remote branch
is to always track the branch it was created from.
For example, if a branch 'my_branch' is created off of the remote branch
'fix_crash_13', then 'my_branch' will be tracking the remote
'fix_crash_13' branch.
It is common practice to have both the local and remote branches named
the same when the local is tracking the remote one. Therefore, it is
reasonable to expect that 'my_branch' should not track the remote
'fix_crash_13' branch.
The new behaviour when creating a new branch off of a remote branch is
to track the branch it was created from only if the branch names match.
If the branch names DO NOT match then the newly created branch will not
track the remote branch it was created from.
For example, if a user creates a new branch 'fix_crash_13' off of the
remote branch 'fix_crash_13', then the local 'fix_crash_13' branch will
track the remote 'fix_crash_13' branch.
However, if the user creates a new branch called 'other_branch_name' off
of the remote branch 'fix_crash_13', then the local 'other_branch_name'
branch will NOT track the remote 'fix_crash_13' branch.
No content changes yet, because nobody has edited anything in Crowdin so far.
However, this changes a few `\u003` to `<` (pretty sure that was an artefact of
how we manually generated the json files in #3649), and it removes all the
translations that are identical to the English version, which I guess is a good
thing (but doesn't make a difference in practice).
I was looking for empty strings in en.json, that's how I found this one. It
resulted in an empty log entry when adding a co-author to an existing commit.
It seems that the embed.FS always uses foreward slashes, even on
Windows.
This not only affected generating the cheatsheets, but also loading a
translation file in production.
The default shortcut to open git difftool (ctrl+t) is not available on
the "Local Branches" window. It is available when selecting a commit
from a local branch, a remote branch, or a tag from the "Local Branches"
window.
This is inconsistent since branches or tags are also commits, the
shortcut should also work on them directly.
This commit remedies this inconsistency by allowing the use of the
shortcut directly on a branch or a tag. The shortcut works both in the
"standard" mode and the "diffing" mode.
This reverts commit 3af545daf7, reversing
changes made to 629b7ba1b8.
We changed our mind about this and want to provide different options for
achieving the same thing, but with more flexibility.
The highlight is normally turned off in HandleFocusLost, but that's not called
when using ReplaceContext (and changing this would be a lot of work, it seems),
so turn it off manually here for now.
Probably not the most import feature in the world, but when resizing the
terminal window while multiple popup panels were open at the same time, we would
only resize the topmost one.
The main reason for changing this is because it makes the next commit easier to
implement.
This is how we do it for confirmation with suggestions too, so be consistent. It
will make things easier later in this branch if we only have one context per
"panel" on the stack, even if the panel consists of two views.
Concretely this means:
- only push the message context onto the stack when opening the panel (this
requires making the description view visible manually; we do the same for
suggestions)
- when switching between message and description, use ReplaceContext rather than
PushContext
We forgot to handle the "suggestions" and "commitDescription" view names.
Instead of listing all the names of views that can appear in popups though,
let's use the context kind for this, which feels more robust.
This is a change in behavior: previously, clicking outside of the search or
filter prompt would close the prompt, now it no longer does (because search has
a persistent popup kind, but it wasn't listed in the list of view names before).
We write a hacky, one-off script to do that. We need this script only once, so
we don't bother polishing it much. We'll re-purpose it later in the branch to
convert the English translation set to JSON; that is an operation that we need
to do regularly in the future.
An inactive selection is one where the view is part of the context stack, but
not the active view. For example, the files view when you enter the staging
panel, or any view when you open a panel.
Remove the old mechanism of clearing the highlight in Layout.
This fixes a problem with a wrong highlight showing up in the staging panel when
entering a file with only staged changes.
Reproduction recipe:
1. stage all changes in a file by pressing space on it in the files panel
2. enter the staged changes panel by pressing enter
3. unstage one of the changes
This makes the unstaged changes panel visible, but keeps the focus in the staged
changes panel. However, the highlight in the unstaged changes view becomes
visible, as if it were focused.
To explain why this happens, you need to know how the selection highlighting of
a view is turned on or off. It is turned on when it gains the focus, i.e. when
ActivateFocus is called on it, which in turn happens when PushContext is called.
It is turned off in Layout when gocui sees that the current view is no longer
the same as last time, in which case it calls onViewFocusLost on the previous
current view.
This mechanism only works reliably when there is at most one PushContext call
per event handler. If there is more than one, then the first one gets its
highlight turned on, then the second one, but since gocui has never seen the
first one as the active view in Layout, it doesn't get the highlight turned off
again even though it should.
And this happens in the above scenario. When pressing enter on a file with only
staged changes, we first push the staging context (in
FilesController.EnterFile), and then later we push the stagingSecondary context
when we realize we only have staged changes. This leaves the highlight of the
staging context on.
runewidth.StringWidth is an expensive call, even if the input string is pure
ASCII. Improve this by providing a wrapper that short-circuits the call to len
if the input is ASCII.
Benchmark results show that for non-ASCII strings it makes no noticable
difference, but for ASCII strings it provides a more than 200x speedup.
BenchmarkStringWidthAsciiOriginal-10 718135 1637 ns/op
BenchmarkStringWidthAsciiOptimized-10 159197538 7.545 ns/op
BenchmarkStringWidthNonAsciiOriginal-10 486290 2391 ns/op
BenchmarkStringWidthNonAsciiOptimized-10 502286 2383 ns/op
In d5b4f7bb3e and 58a83b0862 we introduced a combined mechanism for rerendering
views when either their width changes (needed for the branches view which
truncates long branch names), or the screen mode (needed for those views that
display more information in half or full screen mode, e.g. the commits view).
This was a bad idea, because it unnecessarily rerenders too many views when just
their width changes, which causes a noticable lag. This is a problem, for
example, when selecting a file in the files panel that has only unstaged
changes, and then going to one that has both staged and unstaged changes; this
splits the main view, causing the side panels to become a bit narrower, and
rerendering all those views took almost 500ms on my machine. Another similar
example is entering or leaving staging mode.
Fix this by being more specific about which views need rerendering under what
conditions; this improves the time it takes to rerender in the above scenarios
from 450-500s down to about 20ms.
This reintroduces the code that was removed in 58a83b0862, but in a slightly
different way.
The rendering of remote branches is in no way dependent on the width of the view
(or the screen mode). Unlike in the local branches view, we don't truncate long
branch names here (because there's no more information after them).
This is an error introduced in d5b4f7bb3e.
For checkboxes it probably doesn't really make sense to use them yet, because
we'd have to find a way how you can toggle them without closing the dialog; but
we already provide rendering for them to lay the ground.
But radio buttons can be used already, because for those it is ok to close the
dialog when choosing a different option (as long as there is only one grounp of
radio buttons in the panel, that is).
Strike it through if not applicable. This will hopefully help with confusion
about the meaning of "all" in the "Discard all changes" entry; some people
misunderstand this to mean all changes in the working copy. Seeing the "Discard
unstaged changes" item next to it hopefully makes it clearer that "all" is meant
in contrast to that.
Several custom patch commands on parts of an added file would fail with the
confusing error message "error: new file XXX depends on old contents". These
were dropping the custom patch from the original commit, moving the patch to a
new commit, moving it to a later commit, or moving it to the index.
We fix this by converting the patch header from an added file to a diff against
an empty file. We do this not just for the purpose of applying the patch, but
also for rendering it and copying it to the clip board. I'm not sure it matters
much in these cases, but it does feel more correct for a filtered patch to be
presented this way.
We're going to add another argument in the next commit, and that's getting a bit
much, especially when most of the arguments are bool and you only see true and
false at the call sites without knowing what they mean.
This currently works (albeit with a bit of manual work, as the user needs to
resolve conflicts), and we add this test just to make sure that we don't break
it with the following change.
This currently works, we add it as a regression test to make sure we don't break
it. It is an interesting test because it turns the deletion of the file in the
moved-from commit into a modification.
This is important when using a pager that draws a horizontal line across the
entire width of the view; when changing from a file or directory that has only
unstaged (or only staged) changes to one that has both, the main view is split
in half, but the PTY task would be run on the view in its old state, so the
horizonal line would be too long and wrap around.
All PTYs were created with the size of the main view, on the assumption that
main and secondary always have the same size. That's not true though; in
horizontal split mode, the width of the two views can differ by one because of
rounding, and when using a pager that draws a horizontal line across the width
of the view, this is visible and looks very ugly.
When switching to a repo that was open before, the context tree is reused, so
before adding keybinding functions to those contexts again, we need to clear the
old ones.
When refreshViewportOnChange is true, we would refresh the viewport once at the
end of FocusLine, and then we would check at the end of AfterLayout if the
origin has changed, and refresh again if so. That's unnecessarily complicated,
let's just unconditionally refresh at the end of AfterLayout only.
We want to add an additional method to ISearchableContext later in this branch,
and this will make sure that we don't forget to implement it in any concrete
context.
Searching in the "Divergence from upstream" view would select the wrong lines.
The OnSearchSelect function gets passed a view index, and uses it to select a
model line. In most views these are the same, but not in the divergence view
(because of the Remote/Local section headers).
ListContextTrait.OnSearchSelect was introduced in 138be04e65, but it was never
called. I can only guess that a planned refactoring wasn't finished here.
Unfortunately it isn't possible to delete them. This would often be useful, but
our todo rewriting mechanisms rely on being able to find todos by some
identifier (hash for pick, ref for update-ref), and exec todos don't have a
unique identifier.
If exactly one candidate from inside the current branch is found, we return that
one even if there are also hunks belonging to master commits; we disregard those
in this case.
Copy the slice into a variable and use that throughout the whole operation; this
makes us a little more robust against the model refreshing concurrently.
It has two modified hunks, one for a master commit and one for a branch commit.
Currently we get an error mentioning those two commits, but we would like to
silently select the branch commit.
Put it into the individual menu items instead.
Again, this is necessary because we are going to add another entry to the menu
that is independent of the selected branch.
Instead, disable the individual entries in the menu.
This is necessary because we are going to add another entry to the menu that is
independent of the selected branch.
Previously the entire status was colored in a single color, so the API made
sense. This is going to change in the next commit, so now we must include the
color in the string returned from BranchStatus(), which means that callers who
need to do hit detection or measure the length need to decolorize it.
While we're at it, switch the order of ↑3↓7 to ↓7↑3. For some reason that I
can't really explain I find it more logical this way. The software out there is
pretty undecided about it, it seems: VS Code puts ↓7 first, and so does the
shell prompt that comes with git; git status and git branch -v put "ahead" first
though. Shrug.
It is a valid case for a branch to share no history with any of the main
branches, in which case git merge-base returns an error (and an empty string).
Since we can't distinguish this from one of the main branches having been
deleted, we shouldn't invalidate the cache in that case.
Use Equals instead of Contains for asserting the status view content. This
solves the problem that we might assert Contains("↓2 repo"), but what it really
shows is "↑1↓2 repo", and the test still succeeds. At best this is confusing.
Also, this way we don't have to use the awkward DoesNotContain to check that it
really doesn't show a checkmark.
To do this, we need to fix two whitespace problems:
- there was always a space at the end for no reason. Simply remove it. It was
added in efb51eee96, but from looking at that diff it seems it was added
accidentally.
- there was a space at the beginning if the branch status was empty. This is
actually a cosmetic problem, for branches without a status the text was
indented by once space. Change this so that the space is added conditionally.
It's a bit awkward that we have to use Decolorise here, but this will go away
again later in this branch.
We aren't using them, yet, except for deciding whether to show the warning about
hunks with only added lines.
Add a bit of test coverage for parseDiff while we're at it.
This broke with 81b497d186 (#3387). In that PR I claimed that we never want to
ask for force-pushing if the server rejected the update, on the assumption that
this can only happen because the remote tracking branch is not up to date, and
users should just fetch in this case. However, I didn't realize it's even
possible to have a branch whose upstream branch is not stored locally; in this
case we can't tell ahead of time whether a force push is going to be necessary,
so we _have_ to rely on the server response to find out. But we only want to do
that in this specific case, so this is not quite an exact revert of 81b497d186.
This broke with #3528.
If the remote branch is not stored locally, we only see question marks in the
branch status. In this case we can't tell whether we need to force-push, so it's
best to assume that we don't, and see if the server rejects the push, and react
to that by asking to force push. This second part is also broken right now,
we'll fix this in the next commit.
When branches are sorted by recency we have this logic that first loads the
branches so that they can be rendered quickly; in parallel, it starts loading
the reflog in the background, and when that's done, it loads the branches again
so that they get their recency values. This means that branches are loaded twice
at startup.
We don't need this logic when branches are not sorted by recency, so we can
simply load branches and reflog in parallel like everything else.
This shouldn't change any user observable behavior, it just avoids doing
unnecessary work at startup.
For tooltips that are just one or two characters longer than the available
width, the last word would be cut off. On my screen this happened for the
tooltip for the fixup command.
It can optionally be used to set the title of the panel that shows the output of
a command (when showOutput is true). If left unset, the command string is used
as the title.
To determine whether we need to ask for force pushing, we need to query the push
branch rather than the upstream branch, in case they are not the same.
In a triangular workflow the branch that you're pulling from is not the same as
the one that you are pushing to. For example, some people find it useful to set
the upstream branch to origin/master so that pulling effectively rebases onto
master, and set the push.default git config to "current" so that "feature"
pushes to origin/feature.
Another example is a fork-based workflow where "feature" has upstream set to
upstream/main, and the repo has remote.pushDefault set to "origin", so pushing
on "feature" pushes to origin/feature.
This commit adds new fields to models.Branch that store the ahead/behind
information against the push branch; for the "normal" workflow where you pull
and push from/to the upstream branch, AheadForPush/BehindForPush will be the
same as AheadForPull/BehindForPull.
Our code doesn't realize that we need to prompt the user to force push, when the
branch is up-to-date with its upstream but not with the branch that we're
pushing to.
It is unexpected that a function called PushBranch also sets the upstream
branch; also, we want to add a PushBranch function in the next commit that
doesn't.
This guards against accidentally renaming a model field and thereby breaking
user's custom commands. With this change we'll get a build failure when we do
that.
In go 1.22, loop variables are redeclared with each iteration of the
loop, rather than simple updated on each iteration. This means that we
no longer need to manually redeclare variables when they're closed over
by a function.
For custom commands it is useful to select an earlier command and have it copied
to the prompt for further editing. This can be done by hitting 'e' now.
For other types of suggestion panels we don't enable this behavior, as you can't
create arbitrary new items there that don't already exist as a suggestion.
In the custom commands panel you can now tab to the suggestions and hit 'd' to
delete items from there. Useful if you mistyped a command and don't want it to
appear in your history any more.
- If _not_ inside a neovim session, treat as
a normal nvim session and suspend lazygit.
- If inside a neovim session:
- Do not try to suspend lazygit.
- Send `q` keystroke to neovim session to quit lazygit.
- Send filename/line/etc. to neovim session.
We are going to truncate overly long lines returned from git log, and the most
likely field that is going to make the line too long is the subject; so we must
put it last, otherwise we'd end up with not enough fields to split when it's too
long.
It might not be obvious from the diff what's happening to the mock command
output in the test: it didn't have the divergence field (">") at all, which was
kind of a bug. It didn't matter for these tests though, because we are not
testing the divergence here, and our production code happens to be resilient
against it missing. But now we must add the ">" field before the subject.
Scanners can return errors (e.g. ErrTooLong), and if we don't handle it, the
cmd.Wait() call below will block forever because nobody drains the command's
output.
This happens for CommitLoader.GetCommits when there's a commit whose subject
line is longer than approx. 65500 characters; in that case, lazygit would lock
up completely. With this fix it remains usable, but the commit list is truncated
before the bad commit, which is not good enough. We'll improve that in the
remaining commits of this branch.
Deadlock reporting broke in e1ceb6892a; since then, it was *off* when running
debug builds normally, but *on* when debugging an integration test. Both of
which are exactly opposite of what we want.
Change `func displayCommit()` so all the individual strings are built first,
then the whole thing `cols` is put together. Before, most strings were built
prior to constructing `cols`, but a few were built inside the `cols`
construction.
The rebase.updateRefs feature of git is very useful to rebase a stack of
branches and keep everything nicely stacked; however, it is usually in the way
when you make a copy of a branch and want to rebase it "away" from the original
branch in some way or other. For example, the original branch might sit on main,
and you want to rebase the copy onto devel to see if things still compile there.
Or you want to do some heavy history rewriting experiments on the copy, but keep
the original branch in case the experiments fail. Or you want to split a branch
in two because it contains two unrelated sets of changes; so you make a copy,
and drop half of the commits from the copy, then check out the original branch
and drop the other half of the commits from it.
In all these cases, git's updateRefs feature insists on moving the original
branch along with the copy in the first rebase that you make on the copy. I
think this is a bug in git, it should create update-ref todos only for branches
that point into the middle of your branch (because only then do they form a
stack), not when they point at the head (because then it's a copy). I had a long
discussion about this on the git mailing list [1], but people either don't agree
or don't care enough.
So we fix this on our side: whenever we start a rebase for whatever reason, be
it interactive, non-interactive, or behind-the-scenes, we drop any update-ref
todos that are at the very top of the todo list, which fixes all the
above-mentioned scenarios nicely.
I will admit that there's one scenario where git's behavior is the desired one,
and the fix in this PR makes it worse: when you create a new branch off of an
existing one, with the intention of creating a stack of branches, but before you
make the first commit on the new branch you realize some problem with the first
branch (e.g. a commit that needs to be reworded or dropped). It this case you do
want both branches to be affected by the change. In my experience this scenario
is much rarer than the other ones that I described above, and it's also much
easier to recover from: just check out the other branch again and hard-reset it
to the rebased one.
[1]
https://public-inbox.org/git/354f9fed-567f-42c8-9da9-148a5e223022@haller-berlin.de/
Sometimes it takes a while to get PRs accepted upstream, and this blocks our
progress. Since I'm pretty much the only one making changes there anyway, it
makes sense to point to my fork directly.
It is now only used as the error handler that is passed to gocui.Gui on
construction; it's not a client-facing API any more. Also, it doesn't have to
handle gocui.ErrQuit, as gocui takes care of that.
This lets us get rid of a few more calls to Error(), and it simplifies things
for clients of OnWorker: they can simply return an error from their callback
like we do everywhere else.
In Gui.onWorker we only make the minimum possible change to get things to
compile after the API-breaking change of the gocui update; we'll make this
cleaner later in this branch.
We upgraded our minimum Go version to 1.21 in commit
57ac9c2189. We can now replace our
`utils.Min` and `utils.Max` functions with the built-in `min` and `max`.
Reference: https://go.dev/ref/spec#Min_and_max
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
Unfortunately the migration code requires yaml v3, but our yaml fork is based on
v2, so we need to import both in app_config.go in this commit, which is ugly. We
can clean this up in the next commit.
Make it recognize URLs wrapped in angle brackets, and followed by punktuation.
We don't need this for the status panel, but we will need it for confirmation
panels.
Changing globals in the init() function of a test file is a bad idea, as it
affects all other tests that run after it. Do it explicitly in each test
function that needs it, and take care of restoring the previous value
afterwards.
For people who have the log.showsignature git config set to true, trying to
reword a signed commit would put the signature verification into the subject
field and the commit subject into the description field of the commit message
panel. Amending commits, adding co-authors to a commit, and copying a commit
message to the clipboard would all be broken in a similar way.
Using the "Add to .git/info/exclude" in a worktree results in an error message,
as the test shows. The same would happen in a submodule, but I'm not adding an
extra test for that, as the circumstances are the same.
When exiting filtering mode, we currently keep the selection index the same in
the commits panel. This doesn't make sense at all, since the index in the
filtered view has no relation to the index in the unfiltered view.
I often use filtering mode (either by path or by author) to find a given commit
faster than I would otherwise be able to. When exiting filtering mode, it's
useful to keep the same commit selected, so that I can look at the surrounding
commits, see which branch it was a part of, etc. So reselect the commit again
after exiting filtering mode.
Sometimes this is not possible, most likely when the commit is so long ago that
it's outside of the initial 300 range. In that case, at least select the commit
again that was selected before I entered filtering; this is still better than
arbitrarily keeping the same selection index.
It would crash when some keybindings are set to null, and the filter string is
such that only those keybindings remain visible.
The reason for the crash is that when inserting non-model items (menu section
headers in this case) you specify a column to align them to. This works on the
assumption that the number of columns is always the same. It can cope with the
case that columns are removed because they are empty for all items; but it can't
cope with the case that the getDisplayStrings function returns a lower number of
columns.
And this is what happened here: MenuViewModel.GetDisplayStrings would omit the
keybinding column when none of the entries have a keybinding. This logic is
unnecessary, the generic list rendering mechanism takes care of this, so
removing this logic fixes the crash.
We do have to make sure though that the column is really empty when there's no
keybinding, so change the logic to use FgCyan only when there's a keybinding.
It is a bad idea to read a git-rebase-todo file, remove some update-ref todos,
and write it back out behind git's back. This will cause git to actually remove
the branches referenced by those update-ref todos when the rebase is continued.
The reason is that git remembers the refs affected by update-ref todos at the
beginning of the rebase, and remembers information about them in the file
.git/rebase-merge/update-refs. Then, whenever the user performs a "git rebase
--edit-todo" command, it updates that file based on whether update-ref todos
were added or removed by that edit. If we rewrite the git-rebase-todo file
behind git's back, this updating doesn't happen.
Fix this by not updating the git-rebase-todo file directly in this case, but
performing a "git rebase --edit-todo" command where we set ourselves as the
editor and change the file in there. This makes git update the bookkeeping
information properly.
Ideally we would use this method for all cases where we change the
git-rebase-todo file (e.g. moving todos up/down, or changing the type of a
todo); this would be cleaner because we wouldn't mess with git's private
implementation details. I tried this, but unfortunately it isn't fast enough.
Right now, moving a todo up or down takes between 1 and 2ms on my machine;
changing it to do a "git rebase --edit-todo" slows it down to over 100ms, which
is unacceptable.
In the test we simply removed the update-ref todo but didn't make any other
changes to the todos. This should really have kept everything the way it was,
including the other branch head. The fact that the star was gone was really
because of the bug that we are going to fix later in the branch.
Change the test so that it also makes a change before the update-ref todo; this
way we test that the star is gone because we deleted the update-ref, not because
of the bug.
To guard against the bug, we add another assertion for the branches view to test
that both branches are still there. This currently fails.
Running WSL without a container would be treated as native linux, causing problems at it would then attempt to use `xdg-open`.
This was caused by `isContainer()` always returning true due to some dubious conditionals. These have been removed.
The env-var check seems to not be used by lazygit, nor any common containers, and therefore appears to only exist to manually tell lazygit to behave as if it were inside of a container.
This functionality has been kept, but the env-var has been changed to be all uppercaps as to comply with the POSIX standard.
Fixes#2757
Bug introduced in 4d78d76
I often copy hashes in the commits panel in order to paste them into Github
comments (or other places), and I can't stand it when they have the full length.
I picked a default of 12 for this; I find this to be a good middle ground
between being reliable in large repos (12 still works in the linux kernel repo
today, but it might not be enough in really huge repos) and not being too ugly
(many smaller repos can probably get away with less).
We deliberately don't change this for the "Copy to clipboard" menu, since this
gives users a way to copy the unabbreviated sha if they need this occasionally.
To support this, we turn the confirmation prompt of the "Create fixup commit"
command into a menu; creating a fixup commit is the first entry, so that
"shift-F, enter" behaves the same as before. But there are additional entries
for creating "amend!" commits, either with or without file changes. These make
it easy to reword commit messages of existing commits.
We pass all of them to a single editor command, hoping that the editor will be
able to handle multiple files (VS Code and vim do).
We ignore directories that happen to be in the selection range; this makes it
easier to edit multiple files in different folders in tree view. We show an
error if only directories are selected, though.
We've seen a lot of issues recently where people complain that lazygit doesn't
behave as documented, but that was only because they were running the latest
release but were looking at the documentation of master. Make the documentation
links in the status panel point to the release that they are using in the hope
that this will help a little bit with this problem.
Some operations don't support switching to the editor from the commit message
panel; an example is the commit message panel that appears when moving a custom
patch into a new commit. Disable the "open in editor" menu entry in this case,
instead of silently doing nothing.
Previously we would hide it if no onSwitchToEditor function was set; that was
from a time when <c-o> was bound directly to the switch-to-editor command. Now
it is bound to showing a menu, and that menu is always available even if no
onSwitchToEditor function is set. (We rather need to disable the switch to
editor item _within_ that menu, see next commit.)
For die-hard fuzzy-searching fans it's probably in the way, so taking it out
makes fuzzy filtering work better. For substring filtering it always retains the
sort order anyway.
By default we now search for substrings; you can search for multiple substrings
by separating them with spaces. Add a config option gui.filterMode that can be
set to 'fuzzy' to switch back to the previous behavior.
It sorts them already, so it's unnecessary. In the next commit we use this same
code for substring searching too, and in that case we don't want to sort because
sorting is by Score, but we don't even fill in the score for substring
searching.
Lazygit has two ways to decide whether it needs to ask the user to force-push:
1. if it knows ahead of time that the push will fail because the branch has
diverged, by looking at the incoming/outgoing information that it shows as ↑3↓7.
2. by examining the error that comes back when the push has failed.
The second situation should happen only rarely, because lazygit fetches every
minute by default, so the ↑3↓7 information is usually up to date. It might not
be if the user turned off auto-fetch (or increased the auto-fetch interval).
However, in this case it's almost always harmful to prompt the user to
force-push, because we know that the reason for diverging is that something was
pushed to the remote, and we would wipe it out by force-pushing. In such a
situation, the more likely user action is to pull the remote changes and then
push normally again.
So just remove the second prompt, and replace it by a better error message when
we detect that updates were rejected remotely.
A little bit of history archeology reveals that the second prompt was added at a
time where we didn't have the first one yet, so at that time it made sense to
have it; but when the first prompt was added, we should have removed the second.
So far, the only situation where we called SetSelectionRangeAndMode was one
where the range could only get larger (in startInteractiveRebaseWithEdit, in
which case update-ref todos can be inserted by the rebase). However, in the last
commit we introduced a new call site where the range can get smaller, including
being reduced to a single item. Since this is indistinguishable from a single
selection, set the mode to none in this case; without this, hitting escape would
seemingly do nothing because it collapses the empty range selection.
... and when recalling a commit message from an old commit by pressing up-arrow.
This is necessary because committing turns our soft line breaks into real ones,
but when rewording we want to turn them back into soft ones again, so that it's
possible to insert words at the beginning of a paragraph and have everything
rewrap nicely.
This is only a best effort; the algorithm only removes those hard line breaks
that can be removed without changing the way the message looks. This works well
when the previous commit message was wrapped at the same width, which for most
users should be the most common case; but if it wasn't, the result is not great.
Specifically, if the old wrap width was smaller, some hard line breaks just
won't be removed; if it was wider though, you'll get an unpleasant comb effect
with alternating long and short lines. In such a case it's best to switch to the
editor and use whatever wrapping features you have there (e.g. alt-Q).
I have seen some commit messages that contain CRLF instead of just LF; I'm not
sure if these were created by a broken git client, but they exist, so we need to
deal with them. Editing them when rewording a commit sort of works, but is a
little strange; the \r characters are invisble, so you need an extra arrow key
press to skip over them.
In the next commit we are going to add more logic related to line breaks, and it
is getting confused by the \r, so it is becoming more important to fix this. The
easiest fix is to normalize the line endings right after loading.
This should arguably have been done in b133318b40 already; it's becoming more
important now because we're going to extend the common code with more logic in
the next commit.
We have such a test already (squash_fixups_above_first_commit.go), but it can't
be used for what we want to check here, because it uses the first commit, and we
can't move down from there. So create a new one that basically does the same
thing, but for a commit in the middle. The focus of this new test is to check
how the selection behaves; as you can see, there is a problem both when creating
a fixup and when squashing fixups. We'll address these separately in the next
commits.
It starts a rebase on the bottom-most commit of the range, and sets all the
selected commits to "edit" (skipping merge commits, because they can't be
edited).
In most real-world scenarios, name and path are usually the same. They don't
have to be though, and it's important to make sure we use the right one when
passing arguments to git commands, so change the tests to have different name
and path.
When doing a non-interactive rebase using a version of git earlier than 2.26, or
by explicitly calling `git -c rebase.backend=apply rebase`, lazygit can display
the pending todos by parsing the numbered patch files in `.git/rebase-apply/`.
Unfortunately, support for this has been broken for more than three years
because of the change in 682db77401 (the string literal "normal" should have
been changed to REBASE_MODE_NORMAL instead of REBASE_MODE_MERGING).
It's not an important bug since you can only get into this situation by doing a
rebase outside of lazygit, and then only with a pretty old git version or by
using very uncommon git options. So instead of fixing the bug, just remove the
code.
This makes it possible again to pass multiple arguments, for example
"--ff-only --autostash". This won't work correctly if you want to use
an argument that contains a space, but it's very unlikely that people
will want to do that, so I think this is good enough.
The additional branch head icon is more confusing than useful in this situation.
The update-ref entries show very clearly where the branch heads will go when
continuing the rebase; the information where the branch heads used to be before
the rebase is not really needed here, and just makes the display more confusing.
I'm not adding more tests here because the changes to the existing tests
demonstrate the change clearly enough.
The helix binary seems to be called "helix" on some distributions (e.g. Arch),
but "hx" on others (e.g. Fedora). Provide presets for both, so that
auto-detection from $EDITOR works.
A common workflow for me is to create a fixup commit from only some of my
current changes; to do that, I enter a file, stage a few hunks, and then want to
invoke ctrl-f to find the base commit for these changes. Currently I need to esc
back to the files panel in order to do that; it's more convenient to be able to
do this right from the staging panel.
This commit introduces a new feature to the commit view, allowing users
to filter commits based on the author's name or email address. Similar
to the existing path filtering functionality, accessible through <c-s>,
this feature allows users to filter the commit history by the currently
selected commit's author if the commit view is focused, or by typing in
the author's name or email address.
This feature adds an entry to the filtering menu, to provide users with
a familiar and intuitive experience
Calling "git reset" on the command line (without further arguments) defaults to
--mixed, which is reason enough to make it the default for us, too.
But I also find myself using --mixed more often than --soft. The main use case
for me is that I made a bunch of WIP commits, and want to turn them into real
commits when I'm done hacking. I select the last commit before the WIP commits
and reset to it, leaving all changes of all those commits in the working
directory. Since I want to start staging things from there, I prefer those
modifications to be unstaged at that point, which is what --mixed does.
For some lists it is useful to keep the same sort order when filtering (rather
than sorting by best match like we usually do). Add an optional function to
FilteredList to make this possible, and use it whenever we show lists of things
sorted by date (branches, stashes, reflog entries), as well as menu items
because this allows us to keep the section headers in the keybindings menu,
which is useful for understanding what you are looking at when filtering.
After #3283 we need to read more lines initially so that the scrollbar goes to
its minimal height of 1 for long diffs. Without this, it would start with a
height of 2 and then become smaller after you scroll down half the window
height.
This fixes two problems:
- each time the custom commands panel was opened, the history of commands would
be shown in reversed order compared to last time. (The reason is that
lo.Reverse modifies the slice in place rather than just returning a new,
reversed slice.)
- when executing a previous command again (either by typing it in again, or by
picking it from the history), it should move to the beginning of the history,
but didn't.
We fix this by storing the history in reversed order (as the user sees it in
the panel), this makes the logic simpler. We just have to prepend rather
than append newly added commands now.
While this is theoretically a breaking change, it's not worth bothering because
the order was wrong for existing users in 50% of the cases anyway.
Scenario:
- show the files of a commit, escape out of it again
- start an interactive rebase of a stack of branches, with the rebase.updateRefs
git config set to true
- select one of the update-ref todos
- trigger a refresh (either manually or by bringing lazygit's terminal window to
the front)
This results in an error message "fatal: ambiguous argument '': unknown revision
or path not in the working tree."
Fix this by putting another band-aid on the check for the commit files refresh.
This is the easiest way to fix the problem, but I don't think it's the best one.
We shouldn't be refreshing the commit files context at all if it isn't visible,
because it's pointless; there's no way to switch to it again except by calling
viewFiles again with a specific ref. But I'm too lazy too figure out how to do
that right now.
After discarding file changes from the commit, the was still referencing
these indexes as being part of the range select. The consequence was
needing to hit escape twice to exit commit files in some situations.
Canceling the range select after discarding changes fixes that.
I'm combining the delete single file case from `discard_old_file_change`
with the content of `discard_range_select` and calling that
`discard_old_file_changes`. Hopefully that cleans things up a little
bit.
This also adds a check that the custom patch is getting reset properly.
The waiting status shouldn't happen until after the user has responded
to the popup.
Since we're not giving a standalone prompt about clearing the patch, all
of the business in `discard` doesn't need to be in a function any more
It can be tedious after each cherry-pick opearation to clear the
selection by pressing escape in order for lazygit to stop displaying
info about copied commits. Also, it seems to be a rare case to
cherry-pick commits to more than one destination.
The simplest solution to address this issue is to clear the selection
upon paste.
The only exception is a merge conflict. Initially, I wanted to clear
selected commits in this scenario too. During a discussion we found out
that it may be convenient to have the copied commits still around.
Aborting the rebase and pasting the commits in the middle of a branch
can be a valid use case.
To do that, change the "Apply fixup commits" command to show a menu with the two
choices "in current branch" and "above the selected commit"; we make "in current
branch" the default, as it's the more useful one most of the time, even though
it is a breaking change for those who are used to "shift-S enter" meaning
"squash above selected".
Before this commit, we had pkg/integration/tests/submodule/add.go
failing with a panic. I'm pretty sure the issue is this: we're now
calling quite a few GetDisabledReason calls on each layout() call,
and if a background thread happens to update a model slice while
we're doing this, we can end up with a selection index that's now
out of bounds because it hasn't been clamped to match the new list
length.
Specifically, here we had the selected index being -1 (the list starts
empty and somehow the value is -1 in this case) and then the list
gets a new submodule so the length is now 1, but the list cursor
doesn't know about this so remains on the old value. Then we confirm
the length is greater than zero and try to get the selected submodule
and get an out of bounds error.
This commit fixes the issue by clamping the selected index whenever
we get the length of the list so that it stays in-sync. This is not
a perfect solution because the length can change at any time, but
it seems to reliably fix the test, and using mutexes didn't seem to
make a difference.
Note that we're swapping the order of IFileTree and IListCursor in
the file tree view model to ensure that the list cursor's Len()
method is called (which performs the clamping).
Also, comment from the PR:
This 'trait' pattern we're using is convenient but can lead to awkward
situations. In this case we have both the list view model and the
(embedded) list cursor with a Len() method. The list cursor Len()
method just calls the list view model Len() method. But I wanted
to make it that the list view model now calls ClampSelection() on the
list cursor whenever it obtains the length. This will cause an
infinite loop because ClampSelection() internally calls Len()
(which calls the list view model's Len() method which in turn
calls ClampSelection() again, etc).
The only reason we were passing the list view model into the list
cursor was to supply the length method, so now we're just doing
that directly, and letting the list view model delegate the Len()
call to the list cursor, which now itself calls ClampSelection.
As part of making lazygit more discoverable, there are certain keys which you almost certainly
need to press when you're in a given mode e.g. 'v' to paste commits when cherry-picking. This
commit prominently shows these keybinding suggestions alongside the others in the option view.
I'm using the same colours for these keybindings as is associated with the mode elsewhere e.g.
yellow for rebasing and cyan for cherry-picking. The cherry-picking one is a bit weird because
we also use cyan text to show loaders and app status at the bottom left so it may be confusing,
but I haven't personally found it awkward from having tested it out myself.
Previously we would render these options whenever a new context was activated, but now that we
need to re-render options whenever a mode changes, I'm instead rendering them on each screen
re-render (i.e. in the layout function). Given how cheap it is to render this text, I think
it's fine performance-wise.
This adds a bunch of tooltips to keybindings and updates some keybinding descriptions (i.e. labels).
It's in preparation for displaying more keybindings on-screen (in the bottom right of the screen),
and so due in part to laziness it shortens some descriptions so that we don't need to manage both
a short and long description (for on-screen vs in-menu). Nonetheless I've added a ShortDescription
field for when we do want to have both a short and long description.
You'll notice that some keybindings I deemed unworthy of the options view have longer descriptions,
because I could get away with it.
We're going to be adding tooltips to the cheatsheet to better explain what each actions
does. As such, we're switching to a table format rather than a list.
I'm also changing how the keys are represented, using a markdown approach rather than
an html approach
This is useful if you want to move a range of commits, so you select them, and
then realize it's better to do it in an interactive rebase. Pressing 'i'
preserves the range now.
This helps work around bugs in editors that may get confused about relative
paths (like nvim-remote, see https://github.com/neovim/neovim/issues/18519), and
shouldn't have any negative effect on others.
As part of this, you must now press enter on a merge conflict file
to focus the merge view; you can no longer press space and if you do
it will raise an error.
This changes GetRepoPaths() to pull information from `git rev-parse`
instead of effectively reimplementing git's logic for pathfinding. This
change fixes issues with bare repos, esp. versioned homedir use cases,
by aligning lazygit's path handling to what git itself does.
This change also enables lazygit to run from arbitrary subdirectories of
a repository, including correct handling of symlinks, including "deep"
symlinks into a repo, worktree, a repo's submodules, etc.
Integration tests are now resilient against unintended side effects from
the host's environment variables. Of necessity, $PATH and $TERM are the
only env vars allowed through now.
I prefer this because I almost never use sticky range selections. Also, this
fixes the issue that just clicking a line in a diff (without dragging) already
creates a range selection. It still does, technically, but it's no longer a
problem because a non-sticky one-line range selection behaves the same as a
non-range selection.
Previously if we marked a line with IsSelected() we would check if it was selected, but
we would not check if other lines were unexpectedly selected. Now, if you use IsSelected(),
we ensure that _only_ the lines you marked as such are the selected lines.
Something dumb that we're currently doing is expecting list items
to define an ID method which returns a string. We use that when copying
items to clipboard with ctrl+o and when getting a ref name for diffing.
This commit gets us a little deeper into that hole by explicitly requiring
list items to implement that method so that we can easily use the new
helper functions in list_controller_trait.go.
In future we need to just remove the whole ID thing entirely but I'm too
lazy to do that right now.
Fixes https://github.com/jesseduffield/lazygit/issues/3077
Show unstaged file names in default colour
Previously, we had the following rules:
* file names were in red when unstaged or partially staged
* directory names were in red if unstaged, yellow if partially staged,
and
green if fully staged
Red text on a black background can be hard to read, so instead I'm
changing it
so that unstaged files have their names in the default text colour.
I'm also making it so that partially staged files are in yellow, just
like how
partially staged directories are yellow (same deal with the commit files
view
when adding to a custom patch).
So the new rules are:
* unstaged files/directories use the default colour
* partially staged files/directories are in yellow
* fully staged files/directories are in green
I've also done a refactor on the code clean up some dead code from when
the file tree
outline was drawn with box characters, and I've made it so that the
indentation in
each line is handled inside the function that draws the line rather than
in the recursive
parent function. This makes it easier to experiment with things like
showing the file
status characters on the left edge of the view (admittedly after
experimenting with it,
I decided I didn't like it). Apologies for having a refactor and a
functional change
in the one commit but by the time I was done, I couldn't be bothered
going back and
retroactively splitting it into two halves.
Previously, we had the following rules:
* file names were in red when unstaged or partially staged
* directory names were in red if unstaged, yellow if partially staged, and
green if fully staged
Red text on a black background can be hard to read, so instead I'm changing it
so that unstaged files have their names in the default text colour.
I'm also making it so that partially staged files are in yellow, just like how
partially staged directories are yellow (same deal with the commit files view
when adding to a custom patch).
So the new rules are:
* unstaged files/directories use the default colour
* partially staged files/directories are in yellow
* fully staged files/directories are in green
I've also done a refactor on the code clean up some dead code from when the file tree
outline was drawn with box characters, and I've made it so that the indentation in
each line is handled inside the function that draws the line rather than in the recursive
parent function. This makes it easier to experiment with things like showing the file
status characters on the left edge of the view (admittedly after experimenting with it,
I decided I didn't like it). Apologies for having a refactor and a functional change
in the one commit but by the time I was done, I couldn't be bothered going back and
retroactively splitting it into two halves.
This wasn't necessary before, because the only available branch sorting option
was by recency, so the sort order couldn't change except by checking out
branches. Now, you can sort by committer date, so the branch order can change by
fetching; in this case it's important to keep the same branch selected. One
important use case is to rebase the checked-out branch onto master; you select
master, press "f" to fetch it (this can now change its position in the list),
and then press "r" to rebase. To make this work smoothly it's important to keep
master selected after pressing "f".
Often if a test fails and there's an unaknowledged toast message, that message will
explain why the test failed. Given that we don't display toast messages in
integration tests when they run (for reasons I can't recall right now), we need to
log it as part of the error message.
We don't need it there so no need to enable it.
I'm leaving the disabled reason checks there, even though they're now redundant,
because they're only one-liners and they communicate intent.
We want to show an error when the user tries to invoke an action that expects only
a single item to be selected.
We're using the GetDisabledReason field to enforce this (as well as DisabledReason
on menu items).
I've created a ListControllerTrait to store some shared convenience functions for this.
This requires us to change the 'v' keybinding for paste to something else,
now that 'v' is used globally for toggling range select. So I'm using
'shift+v' and I'm likewise changing 'c' to 'shift+c' for copying, so
that they're consistent.
We will need to clearly communicate this change in keybindings.
The only time we should call SetSelectedLineIdx is when we are happy for a
select range to be retained which means things like moving the selected line
index to top top/bottom or up/down a page as the user navigates.
But in every other case we should now call SetSelection because that will
set the selected index and cancel the range which is almost always what we
want.
This is the highest priority of the escape actions because it's the thing you're
most likely to want to do upon hitting escape if you have a range selected.
Applying this to the staging/patch-building views is tricky: if we want this logic
for when a range of lines is selected, we'll also need to apply it when a hunk
is selected too. I still think it's worth it though: I've often accidentally
escaped from the staging view when trying to cancel a range selection.
We're not fully standardising here: different contexts can store their range state however
they like. What we are standardising on is that now the view is always responsible for
highlighting the selected lines, meaning the context/controller needs to tell the view
where the range start is.
Two convenient benefits from this change:
1) we no longer need bespoke code in integration tests for asserting on selected lines because
we can just ask the view
2) line selection in staging/patch-building/merge-conflicts views now look the same as in
list views i.e. the highlight applies to the whole line (including trailing space)
I also noticed a bug with merge conflicts not rendering the selection on focus though I suspect
it wasn't a bug with any real consequences when the view wasn't displaying the selection.
I'm going to scrap the selectedRangeBgColor config and just let it use the single line
background color. Hopefully nobody cares, but there's really no need for an extra config.
This adds range select ability in two ways:
1) Sticky: like what we already have with the staging view i.e. press v then use arrow keys
2) Non-sticky: where you just use shift+up/down to expand the range
The state machine works like this:
(no range, press 'v') -> sticky range
(no range, press arrow) -> no range
(no range, press shift+arrow) -> nonsticky range
(sticky range, press 'v') -> no range
(sticky range, press arrow) -> sticky range
(sticky range, press shift+arrow) -> nonsticky range
(nonsticky range, press 'v') -> no range
(nonsticky range, press arrow) -> no range
(nonsticky range, press shift+arrow) -> nonsticky range
Previously we included all navigation keybindings from all views in the keybindings menu, meaning
if you pressed enter on 'next page' in the commits view, you'd end up triggering the action
in the sub-commits view.
Because we obtain disabled reasons after every action, we need to keep the code for doing so
super fast. As such, we should not be hitting the filesystem to get rebase state, instead
we should just get the cached state.
I feel like we should actually be using the cached state everywhere like we do with all
our other models if only for the sake of consistency.
A common issue I have is that I want to move a commit from the top of my branch
all the way down to the first commit on the branch. To do that, I need to navigate
down to the first commit on my branch, press 'e' to start an interactive rebase,
then navigate back up to the top of the branch, then move my commit back down to
the base. This is annoying.
Similarly annoying is moving the commit one-by-one without explicitly starting
an interactive rebase, because then each individual step is its own rebase which
takes a while in aggregate.
This PR allows you to press 'i' from the commits view to start an interactive
rebase from an 'appropriate' base. By appropriate, we mean that we want to start
from the HEAD and stop when we reach the first merge commit or commit on the main
branch. This may end up including more commits than you need, but it doesn't make
a difference.
For some bizarre reason `pkg/integration/tests/filter_by_path/cli_arg.go` is failing as of 8c716184 like so:
```
test_lazygit
Usage:
test_lazygit [git-arg]
Positional Variables:
git-arg Panel to focus upon opening lazygit. Accepted values (based on git terminology): status, branch, log, stash. Ignored if --filter arg is passed.
Flags:
-h --help Displays help with available flag, subcommand, and positional value parameters.
-p --path Path of git repo. (equivalent to --work-tree=<path> --git-dir=<path>/.git/)
-f --filter Path to filter on in `git log -- <path>`. When in filter mode, the commits, reflog, and stash are filtered based on the given path, and some operations are restricted
-v --version Print the current version
-d --debug Run in debug mode with logging (see --logs flag below). Use the LOG_LEVEL env var to set the log level (debug/info/warn/error) (default: false)
-l --logs Tail lazygit logs (intended to be used when `lazygit --debug` is called in a separate terminal tab)
-c --config Print the default config
-cd --print-config-dir Print the config directory
-ucd --use-config-dir override default config directory with provided directory
-w --work-tree equivalent of the --work-tree git argument
-g --git-dir equivalent of the --git-dir git argument
-ucf --use-config-file Comma separated list to custom config file(s)
Unknown arguments supplied: filterFile
```
where the CLI args are:
```
([]string) (len=5 cap=5) {
(string) (len=25) "/tmp/lazygit/test_lazygit",
(string) (len=6) "-debug",
(string) (len=108) "--use-config-dir=/Users/jesseduffieldduffield/repos/lazygit/test/_results/filter_by_path/cli_arg/used_config",
(string) (len=2) "-f",
(string) (len=10) "filterFile"
}
```
This appears to be a bug in flaggy itself. I've updated to the latest version but it still breaks. Bizarrely it works fine on CI and
only fails locally. Running lazygit locally with `lg -f pkg/gui/controllers/helpers/refresh_helper.go` it works fine. So I don't
know what's going on there. At any rate, I'm just going to get the test passing by passing `-f=filterFile` as a single argument.
We need to fetch our list of tests both outside of our test binary and within. We need
to get the list from within so that we can run the code that drives the test and runs
assertions. To get the list of tests we need to know where the root of the lazygit repo
is, given that the tests live in files under that root.
So far, we've used this GetLazyRootDirectory() function for that, but it assumes that
we're not in a test directory (it just looks for the first .git dir it can find). Because
we didn't want to properly fix this before, we've been setting the working directory of
the test command to the lazygit root, and using the --path CLI arg to override it when
the test itself ran. This was a terrible hack.
Now, we're passing the lazygit root directory as an env var to the integration test, so
that we can set the working directory to the actual path of the test repo; removing the
need to use the --path arg.
Git has a bug [1] whereby running multiple fetch commands at the same time
causes all of them to append their information to the .git/FETCH_HEAD file,
causing the next git pull that wants to use the information to become confused,
and show an error like "Cannot rebase onto multiple branches". This error would
occur when pressing "f" and "p" in quick succession in the files panel, but also
when pressing "p" while a background fetch happens to be running. One likely
situation for this is pressing "p" right after startup.
Since lazygit never uses the information written to .git/FETCH_HEAD, it's best
to avoid writing to it, which fixes the scenarios described above.
However, it doesn't fix the problem of repeatedly pressing "f" quickly on the
checked-out branch; since we call "git pull" in that case, the above fix doesn't
help there. We'll address this separately in another PR.
[1] See https://public-inbox.org/git/xmqqy1daffk8.fsf@gitster.g/ for more
information.
The algorithm works by blaming the deleted lines, so if a hunk contains only
added lines, we can only hope that it also belongs in the same commit. Warn the
user about this.
Note: the warning might be overly agressive, we'll have to see if this is
annoying. The reason is that it depends on the diff context size whether added
lines go into their own hunk or are grouped together with other added or deleted
lines into one hunk. However, our algorithm uses a diff context size of 0,
because that makes it easiest to parse the diff; this results in hunks having
only added lines more often than what the user sees. For example, moving a line
of code down by two lines will likely result in a single hunk for the user, but
in two hunks for our algorithm. On the other hand, being this strict makes the
warning consistent. We could consider using the user's diff context size in the
algorithm, but then it would depend on the current context size whether the
warning appears, which could be confusing. Plus, it would make the algorithm
quite a bit more complicated.
There are two possible fixes for this bug, and they differ in behavior when
rewording a commit. The one I chose here always splits at the first line feed,
which means that for an improperly formatted commit message such as this one:
This is a very long multi-line subject,
which you shouldn't really use in git.
And this is the body (we call it "description" in lazygit).
we split after the first line instead of after the first paragraph. This is
arguably not what the original author meant, but splitting after the first
paragraph doesn't really work well in lazygit, because we would try to put both
lines into the one-line subject field of the message panel, and you'd only see
the second and not even know that there are more.
The other potential fix would have been to join subject and description with two
line feeds instead of one in JoinCommitMessageAndDescription; this would have
fixed our bug in the same way, but would result in splitting the above message
after the second line instead of the first. I think that's worse, so I decided
for the first fix.
While we're at it, simplify the code a little bit; strings.Cut is documented to
return (s, "") when the separator is not found, so there's no need to do this on
our side.
We do have to trim spaces on the description now, to support the regular reword
case where subject and body are separated by a blank line.
SplitCommitMessageAndDescription splits at the first '\n\n' that it finds (if
there is one), which in this case is between the two paragraphs of the
description. This is wrong.
Use git log instead of git rev-list, this way we don't get a line "commit <sha>"
at the beginning that we then have to discard again.
The test TestGetCommitMsg is becoming a bit pointless now, since it just
compares that input and output are identical.
Without this it's not reliably possible to ask whether a given view is visible
by asking
windowHelper.TopViewInWindow(context.GetWindowName()) == context.GetView()
because there could be transient, invisible contexts after it in the Z order.
I guess it's a bit of a coincidence that this has never been a problem so far.
The output of the GetWindowDimensions function is hard to understand just by looking at it,
so I've added a helper function in the tests to render the window layout as text, so that
in order to create a new test you just come up with some args and paste the output as the
expected output.
This has the same downsides that any snapshot-based testing has: it's more brittle than
targeted assertions. But it is much easier to make sense of these snapshots than it is
to make sense of more fine-grained assertions, and I like the fact that these tests can
serve as documentation.
We are also removing the single-character padding on the left/right edges of the bottom
line because it's unnecessary
Unfortunately we need to create views for each spacer: it's not enough to just
layout the existing views with padding inbetween because gocui only renders
views meaning if there is no view in a given position, that position will just
render whatever was there previously (at least that's what I recall from talking
this through with Stefan: I could be way off).
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
It sounds like at some point we only showed a slash as the search prompt, but I
dug a bit through the history and couldn't find a state of the code where that
was the case. (shrug)
This PR captures the code coverage from our unit and integration tests. At the
moment it simply pushes the result to Codacy, a platform that assists with
improving code health. Right now the focus is just getting visibility but I want
to experiment with alerts on PRs when a PR causes a drop in code coverage.
To be clear: I'm not a dogmatist about this: I have no aspirations to get to
100% code coverage, and I don't consider lines-of-code-covered to be a perfect
metric, but it is a pretty good heuristic for how extensive your tests are.
The good news is that our coverage is actually pretty good which was a surprise
to me!
As a conflict of interest statement: I'm in Codacy's 'Pioneers' program which
provides funding and mentorship, and part of the arrangement is to use Codacy's
tooling on lazygit. This is something I'd have been happy to explore even
without being part of the program, and just like with any other static analysis
tool, we can tweak it to fit our use case and values.
## How we're capturing code coverage
This deserves its own section. Basically when you build the lazygit binary you
can specify that you want the binary to capture coverage information when it
runs. Then, if you run the binary with a GOCOVERDIR env var, it will write
coverage information to that directory before exiting.
It's a similar story with unit tests except with those you just specify the
directory inline via `-test.gocoverdir`.
We run both unit tests and integration tests separately in CI, _and_ we run them
parallel with different OS's and git versions. So I've got each step uploading
the coverage files as an artefact, and then in a separate step we combine all
the artefacts together and generate a combined coverage file, which we then
upload to codacy (but in future we can do other things with it like warn in a PR
if code coverage decreases too much).
Another caveat is that when running integration tests, not only do we want to
obtain code coverage from code executed by the test binary, we also want to
obtain code coverage from code executed by the test runner. Otherwise, for each
integration test you add, the setup code (which is run by the test runner, not
the test binary) will be considered un-covered and for a large setup step it may
appear that your PR _decreases_ coverage on net. Go doesn't easily let you
exclude directories from coverage reports so it's better to just track the
coverage from both the runner and the binary.
The binary expects a GOCOVERDIR env var but the test runner expects a
test.gocoverdir positional arg and if you pass the positional arg it will
internally overwrite GOCOVERDIR to some random temp directory and if you then
pass that to the test binary, it doesn't seem to actually write to it by the
time the test finishes. So to get around that we're using LAZYGIT_GOCOVERDIR and
then within the test runner we're mapping that to GOCOVERDIR before running the
test binary. So they both end up writing to the same directory. Coverage data
files are named to avoid conflicts, including something unique to the process,
so we don't need to worry about name collisions between the test runner and the
test binary's coverage files. We then merge the files together purely for the
sake of having fewer artefacts to upload.
## Misc
Initially I was able to have all the instances of '/tmp/code_coverage' confined
to the ci.yml which was good because it was all in one place but now it's spread
across ci.yml and scripts/run_integration_tests.sh and I don't feel great about
that but can't think of a way to make it cleaner.
I believe there's a use case for running scripts/run_integration_tests.sh
outside of CI (so that you can run tests against older git versions locally) so
I've made it that unless you pass the LAZYGIT_GOCOVERDIR env var to that script,
it skips all the code coverage stuff.
On a separate note: it seems that Go's coverage report is based on percentage of
statements executed, whereas codacy cares more about lines of code executed, so
codacy reports a higher percentage (e.g. 82%) than Go's own coverage report
(74%).
Situations where a view's width changes:
- changing screen modes
- enter staging or patch building
- resizing the terminal window
For the first of these we currently have special code to force a rerender, since
some views render different content depending on whether they are in full-screen
mode. We'll be able to remove that code now, since this new generic mechanism
takes care of that too.
But we will need this more general mechanism for cases where views truncate
their content to the view width; we'll add one example for that later in this
branch.
refreshWorktrees re-renders the branches view, because the branches view shows
worktrees against branches. This means that when both BRANCHES and WORKTREES are
requested to be refreshed, the branches view would be rendered twice in short
succession. This causes an ugly visual glitch when force-pushing a branch,
because when pushing is done, we would see the ↑4↓9 status come back from under
the Pushing status for a brief moment, to be replaced with a green checkmark a
moment later.
Fix this by including the worktree refresh in the branches refresh when both are
requested. This means that the two are no longer running in parallel for an
async refresh, but hopefully that's not so bad.
When pulling/pushing/fast-forwarding a branch, show this state in the branches
list for that branch for as long as the operation takes, to make it easier to
see when it's done (without having to stare at the status bar in the lower
left).
This will hopefully help with making these operations feel more predictable, now
that we no longer show a loader panel for them.
Very similar to WithWaitingStatus, except that the status is shown in a view
next to the affected item, rather than in the status bar.
Not used by anything yet; again, committing separately to get smaller commits.
From the Go specification [1]:
"3. If the map is nil, the number of iterations is 0."
`len` returns 0 if the map is nil [2]. Therefore, checking `len(v) > 0`
before a loop is unnecessary.
[1]: https://go.dev/ref/spec#For_range
[2]: https://pkg.go.dev/builtin#len
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
This is not a complete fix, but it's good enough to fix the spurious test
failures of submodule/reset.go. We have some vague hope to fix this in a more
sustainable way by somehow improving our concurrency model fundamentally, but
that's a more long-term undertaking, and it's annoying that this test fails so
often, so let's fix it in this way for now.
A new gui config flag 'portraitMode':<string> is added to influence when
LazyGit stacks its UI components on top of one another.
The accepted values are 'auto', 'always', 'never'.
'auto': enter portrait mode when terminal becomes narrow enough
'always': always use portrait mode unconditional of the terminal
dimensions
'never': never use portraid mode
Signed-off-by: Louis DeLosSantos <louis.delos@gmail.com>
We're going to make this user config struct a more authoritative source of truth.
Some of these fields weren't actually explained anywhere so I've added explanations.
In places where a lot of explanation is required I've linked to existing explanations in other docs.
This has several benefits:
- it's less code
- we're using the same mechanism to generate all our auto-generated files, so if
someone wants to add a new one, it's clear which pattern to follow
- we can re-generate all generated files with a single command
("go generate ./...", or "make generate")
- we only need a single check on CI to check that all files are up to date (see
previous commit)
For the "cli" and "tui" modes of the test runner there's a "-race" parameter to
turn it on; for running tests on CI with go test, you turn it on by setting the
environment variable LAZYGIT_RACE_DETECTOR to a non-empty value.
Previously there was no way to render a view's search status without also moving the cursor
to the current search match. This caused issues where we wanted to display the status
after leaving the view and coming back, or when beginning a new search from within the
view.
This commit separates the two use cases so we only move the cursor when we're actually
selecting the next search match
Now that we no longer show it in a loader panel, but in the app status view,
it's awkwardly long (the loading animation is much further to the right than for
other waiting status texts). Hopefully seeing just "Fast-forwarding <branch>" is
enough to be able to tell what's happening.
We do this for two reasons:
- when popping up a credentials prompt, it looks distracting if the waiting
status keeps spinning while the user is typing the password
- the task that updates the waiting status periodically would keep the program
busy, so integration tests would wait forever for the program to become idle
again
This can be useful when you know that a cherry-picked commit would conflict at
the tip of your branch, but doesn't at the beginning of the branch (or
somewhere in the middle). In that case you want to be able to edit the commit
before where you want to insert the cherry-picked commits, and then paste to
insert them into the todo list at that point.
This is useful to disable items that are not applicable right now because of
some condition (e.g. the "delete branch" menu item when the currently
checked-out branch is selected).
When a DisabledReason is set on a menu item, we
- show it in a tooltip (below the regular tooltip of the item, if it has one)
- strike through the item's key, if it has one
- show an error message with the DisabledReason if the user tries to invoke the
command
This prevents commands like "go test ./..." from looking into it, and it
prevents VS Code's Problems panel from showing errors about the go files in that
folder.
Add co-author to commits
Add addCoAuthor command for commits
- Implement the `addCoAuthor` command to add co-authors to commits.
- Utilize suggestions helpers to populate author names from the suggestions list.
- Added command to gui at `LocalCommitsController`.
This commit introduces the `addCoAuthor` command, which allows users to easily add co-authors to their commits. The co-author names are populated from the suggestions list, minimizing the chances of user input errors. The co-authors are added using the Co-authored-by metadata format recognized by GitHub and GitLab.
This should already have been done when adding the "View divergence from
upstream" command, but now we're going to add yet another item to the menu that
is unrelated to setting or unsetting the upstream.
So far this hasn't been necessary because all defaults were zero values. We're
about to add the first non-zero value though, and it's important that it is
initialized correctly for users who have a state.yml that doesn't have it yet.
This fixes two minor problems with the prompts:
1. When pressing shift-A in the local commits view, it would first prompt
whether to stage all files, and then it would prompt whether to amend the
commit at all. This doesn't make sense, it needs to be the other way round.
2. When pressing shift-A on the head commit in an interactive rebase, we would
ask whether they want to amend the last commit, like when pressing shift-A in
the files view. While this is technically correct, the fact that we're
amending the head commit in this case is just an implementation detail, and
from the user's point of view it's better to use the same prompt as we do for
any other commit.
To fix these, we remove the confirmation panel from AmendHelper.AmendHead() and
instead add it at the two call sites, so that we have more control over this.
This encapsulates the logic to make sure we have something to commit; which is
to
- auto-stage all files if no files are staged and the SkipNoStagedFilesWarning
config is on
- otherwise, prompt the user whether they want to stage all files
- error out if we don't have any files at all
Of these, the first one was only done when committing with the built-in commit
message panel; there's no reason why it shouldn't also be done when committing
with the editor, or when amending, and now it is.
It implemented this because it wants to do custom truncation of the ref name;
however, we can achieve the same thing by passing the truncated ref name to our
DynamicTitleBuilder, which was previously unused.
Since the slice stores pointers to objects, and we're only modifying the objects
but not the slice itself, there's no need to return it and assign it back. This
will allow us to call the function for subslices of commits.
Also, move the condition that checks for an empty string inside the function;
we're going to call it from more than one place, so this makes it easier.
We will pass these positions back to clients for rendering non-model items, and
it's important that clients can consistently rely on them no matter which
columns were removed.
We'll make some changes to how the display strings are rendered, so it helps to
have this code only once. This also fixes the problem that contexts using
refreshViewportOnChange weren't able to use column alignments so far. We didn't
need this yet, but it's just nice if everything works. :)
When editing a commit, the index of the current commit can change; for example,
when merge commits are involved, or when working with stacked branches where
"update-ref" commands may be added above the selected commit.
Reselect the current commit after pressing "e"; this requires doing the refresh
blocking on the main thread. (Another option that I considered was to use a
SYNC refresh, and then select the new line with an OnUIThread inside the Then
function. This also works, but results in a very visible lag.)
Only do this when an onSwitchToEditor function is actually provided. For the
"Move patch into new commit" command we don't, because it isn't totally
straightforward in that case.
This is useful for when you begin to type the message in lazygit's commit panel,
and then realize that you'd rather use your editor's more powerful editing
capabilities. Pressing <c-o> will take you right there.
When navigating in the commits view to a line that is out of view (e.g. by
pressing , or . to scroll by page, or < or > to scroll to the top or bottom),
the commit graph was not correctly highlighted. Fix this by rerendering the
viewport in this case.
It determines the yellow/red status by getting the merge-base between the
current branch and its upstream; while we're rebasing, the current branch is
HEAD, so it tried to get the merge-base between HEAD and HEAD{u}, which doesn't
work. Fix this by passing the name of the checked-out branch separately.
Previously, when pressing right-arrow when the cursor is already in the last
hunk, it would jump back to the beginning of that hunk. This can be confusing if
the hunk is long, maybe the start of the hunk is already scrolled off the top of
the window, and then pressing right-arrow actually scrolls *backwards*, which is
counter-intuitive. It's better to do nothing in this case.
Same for left-arrow when the cursor is already in the first hunk, although here
the problem is not so severe (unless diff context was increased by a huge
amount, and the start of the first hunk is scrolled off the bottom of the
window).
Previously, the current line was only moved as much as necessary so that it's in
view again. This had the problem that when jumping downwards from hunk to hunk
with the right-arrow key, only the first line of the new hunk was shown at the
bottom of the window. I prefer to put the selected line in the middle of the
view in this case, so that I can see more of the newly selected hunk.
This has the consequence that when scrolling through the view line by line using
down-arrow, the view jumps by half a screen whenever I reach the bottom. I can
see how some users might be opposed to this change, but I happen to like it too,
because it allows me to see more context of what's ahead.
This also fixes a bug where after the rebase each commit in the commits view had a tick against it because we hadn't
refreshed the view since the base commit was no longer marked
