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
There's a bug in LineNumberOfLine, but the existing test coverage doesn't catch
it, as the only test case for this was one where oldStart and newStart were the
same for all hunks. Add a test case where newStart is different for one of the
hunks; this demonstrates a bug, where all expected results from index 12 on are
off by one.
If the command used by OSCommand.OpenLink fails, lazygit crashes. With this change, if the OpenLink command fails, lazygit just shows a dialog inviting the user to visit the relevant URL.
Fixes#2882
This allows us to jump back to the parent neovim process when we want to edit a file, rather than opening a new neovim
process within lazygit.
Arguably this should be the default, but I'm not familiar with the various ways people use lazygit with neovim.
This was on oversight on my part: I assumed that the --work-tree arg was
always intended for use with linked worktrees which have a .git file
pointing back to the repo.
I'm honestly confused now: seems like there are three kinds of worktrees:
* the main worktree of a non-bare repo
* a linked worktree (with its own gitdir in the repo's worktrees/ dir)
* a random folder which you specify as a worktree with the --work-tree arg
I'm pretty sure the --work-tree arg is only intended to be used with this
third kind or workree
... and import stefanhaller's tcell fork for real rather than just replacing it
This solves the problem that people trying to
"go install github.com/jesseduffield/lazygit@latest" would get the error
go: github.com/jesseduffield/lazygit@latest (in github.com/jesseduffield/lazygit@v0.40.0):
The go.mod file for the module providing named packages contains one or
more replace directives. It must not contain directives that would cause
it to be interpreted differently than if it were the main module.
The proper fix is to actually have these two functions share code,
or for views to be able to manage their own heights based on their contents.
But I want to get this out for the sake of a Lazygit Anniversary release.
Now that we refresh upon focus, we can scrap this file watching code.
Stefan says few git UIs use file watching, and I understand why: the
reason this code was problematic in the first place is that watching
files is expensive and if you have too many open file handles that
can cause problems.
Importantly: this code that's being removed was _already_ dead.
When opening lazygit with `lazygit log` the worktrees view was appearing in front of the files view.
This is because it had higher precedence than the files view in the ordered view mapping, and
that was because it originally was in the branches window so it was further down the list.
The reason this didn't cause issues on typical startup is that the files context is activated at the
start so it is brought to the front.
I've been thinking about this for a while: I think it looks really cool if nuking your working tree
actually results in a nuke animation.
So I've added an opt-out config for it
This allows to do the equivalent of "git rebase --onto <target> <base>", by
first marking the <base> commit with the new command, and then selecting the
target branch and invoking the usual rebase command there.
We are in the outsideFilterModeBindings section here; all of these handlers are
wrapped in a OutsideFilterMode guard in a loop below. No need to add one
manually here.
It's tricky to get this right for reflog commits wrt what's the current branch
for each one; so just disable it entirely here, it's probably not something
anybody needs here.
We want to mark all local branch heads with a "*" in the local commits panel, to
make it easier to see how branches are stacked onto each other. In order to not
confuse users with "*" markers that they don't understand, do this only for the
case where users actually use stacked branches; those users are likely not going
to be confused by the display. This means we want to filter out a few branch
heads that shouldn't get the marker: the current branch, any main branch, and
any old branch that has been merged to master already.
The model will be used for logic, so the full hash is needed there; a shortened
hash of 8 characters might be too short to be unique in very large repos. If
some view wants to display a shortened hash, it should truncate it at
presentation time.
- check out a non-main branch before we start
- add authors to expected commits so that we can see whether the commits show an
asterisk
- explicitly check what the top line displays after bisecting has started
This shows that the detached head shows an asterisk, which we don't want. We'll
fix that in the next commit.
This test not only tests the correct handling and display of the updateRef
command, but also the visualization of branch heads in the commits panel. Since
we are about to change the behavior here, extend the test so that a master
commit is added (we don't want this to be visualized as a branch head), and then
a stack of two non-main branches. At the end of this branch we only want to
visualize the head commit of the first.
Update-ref commands have an empty sha, and strings.HasPrefix returns true when
called with an empty second argument, so whenever an update-ref command is
present in a rebase, all commits from there on down were drawn with a green sha.
From the go 1.19 release notes:
Command and LookPath no longer allow results from a PATH search to be found relative to the current directory. This removes a common source of security problems but may also break existing programs that depend on using, say, exec.Command("prog") to run a binary named prog (or, on Windows, prog.exe) in the current directory. See the os/exec package documentation for information about how best to update such programs.
We've been sometimes using lo and sometimes using my slices package, and we need to pick one
for consistency. Lo is more extensive and better maintained so we're going with that.
My slices package was a superset of go's own slices package so in some places I've just used
the official one (the methods were just wrappers anyway).
I've also moved the remaining methods into the utils package.
In the presentation layer, when showing branches, we'll show worktrees against branches if they're
associated. But there was a race condition: if the worktree model was refreshed after the branches model,
it wouldn't be used in the presentation layer when it came time to render the branches.
A better solution would be to have some way of signalling that a particular context needs to be refreshed
and after all the models are done being refreshed, we then refresh the contexts. This will prevent
double-renders
Afero is a package that lets you mock out a filesystem with an in-memory filesystem.
It allows us to easily create the files required for a given test without worrying about
a cleanup step or different tests tripping on eachother when run in parallel.
Later on I'll standardise on using afero over the vanilla os package
When switching worktrees (which we can now do via the branch view) we re-layout the windows and their views.
We had the worktree view ahead of the file view based on the Flatten() method in context.go, because it used
to be associated with the branches panel.
We want to be using forward slashes everywhere internally, so if we get a path from windows
we should immediately convert it to use forward slashes.
I'm leaving out the recent repos list because that would require a migration
There are quite a few paths you might want to get e.g. the repo's path, the worktree's path,
the repo's git dir path, the worktree's git dir path. I want these all obtained once and
then used when needed rather than having to have IO whenever we need them. This is not so
much about reducing time spent on IO as it is about not having to care about errors every time
we want a path.
This fixes pkg/integration/tests/worktree/rebase.go which was failing on old git versions due to a difference in
order of branches that don't have recency values
We now always re-use the state of the repo if we're returning to it, and we always reset the windows to their default tabs.
We reset to default tabs because it's easy to implement. If people want to:
* have tab states be retained when switching
* have tab states specific to the current repo retained when switching back
Then we'll need to revisit this
Older versions of git don't support the -z flag in `git worktree list`.
So we're using newlines.
Also, we're not raising an error upon error because that triggers another refresh,
which gets us into an infinite loop
For marking as good or bad, the current commit is pretty much always the one you
want to mark, not the selected. It's different for skipping; sometimes you know
already that a certain commit doesn't compile, for example, so you might
navigate there and mark it as skipped. So in the case that the current commit is
not the selected one, we now offer two separate menu entries for skipping, one
for the current commit and one for the selected.
This can be useful if you want to find the commit that fixed a bug (you'd use
"broken/fixed" instead of "good/bad" in this case), or if you want to find the
commit that brought a big performance improvement (use "slow/fast"). It's pretty
mind-bending to have to use "good/bad" in these cases, and swap their meanings
in your head.
Thankfully, lazygit already had support for using custom terms during the bisect
(for the case that a bisect was started on the command-line, I suppose), so all
that's needed is adding a way to specify them in lazygit.
Now that we run code concurrently in our loaders, we need to handle that in our tests.
We could enforce a deterministic ordering by mocking waitgroup or something like that,
but I think it's fine to let our tests handle some randomness given that prod itself
will have that randomness.
I've removed the patch test file because it was clunky, not providing much value, and
it would have been hard to refactor to the new pattern
Previously our synchronous refreshes took far longer because nothing
was happening concurrently. We now run refresh functions concurrently
and use a wait group to ensure they're all done before returning
We're:
* using concurrency with wait groups
* avoiding regex
* processing lines of input as they come rather than storing everything in one string
* avoiding an inner loop by creating a mapping of remote names to branches
The speedup is most noticeable on first load, when we haven't yet fetched out main branches.
I saw a speedup from 105ms to 60ms. On subsequent loads the gain is more modest;
54ms to 40ms
Notably, the reflog view is taking ages here because it's got a
few thousand lines to write to the view.
In future we should only populate the view's viewport.
This reverts commit 90613056ce, or the part that removed
a goroutine at least.
Reverting because this has caused an infinite wait for push/pull on windows.
We'll need to find out why that happens separately
On german/french/spanish keyboards, typing [ requires modifier
keys like AltGr, so the `mod==0` condition is wrong.
Fixes#2573
ch != 0 is useless because IsPrint is implemented this way:
if uint32(r) <= MaxLatin1 {
return properties[uint8(r)]&128 != 0
}
with properties[0] set to 1 (so, bit 7 not set)
-> 0 is not printable.
Previously we used a single-line prompt for a tag annotation. Now we're using the commit message
prompt.
I've had to update other uses of that prompt to allow the summary and description labels to
be passed in
Previously, we would only show the authors based on local commits, but sometimes you want to set a commit author
to that of a commit on another branch. Now, so long as you've viewed the branch's commits, the author will appear
as a suggestion.
Previously we applied a right-align on the first column of _all_ menus, even though we really
only intended for it to be on the first column of the keybindings menu (that you get from pressing
'?')
The true issue was that we were focusing the line in the view before it gets resized in the layout function.
This meant if the view was squashed in accordion mode, the view wouldn't know how to set the cursor/origin to
focus the line.
Now we've got a queue of 'after layout' functions i.e. functions to call at the end of the layout function,
right before views are drawn.
The only caveat is that we can't have an infinite buffer so we're arbitrarily capping it at 1000 and dropping
functions if we exceed that limit. But that really should never happen.
This happens consistently for my when I close my MacBook's lid. It seems that
MacOS locks the user's keychain in this case, and since I have my keychain
provide the pass phrases for my ssh keys, fetching fails because it tries to
prompt me for a pass phrase.
This all worked correctly already, we have the FailOnCredentialRequest()
mechanism specifically for this situation, so all is great. The only problem was
that it was trying to pause the ongoing task while prompting the user for input;
but the task is nil for a background fetch (and should be).
It said "Press tab to toggle focus", which is wrong for people who remapped
their togglePanel key binding to something else. Print the actual key binding
instead.
As discussed in https://github.com/jesseduffield/lazygit/pull/2599, it
makes more sense to have the user specify whether they want verbose
commits from their own git config, rather than lazygit config.
This means that we can remove all the code (including test coverage)
associated with the custom verbose flag, and lazygit will just inherit
the .gitconfig settings automatically.
We have a use-case to rebind 'm' to the merge action in the branches panel. There's three ways to handle this:
1) For all global keybindings, define a per-panel key that invokes it
2) Give a name to all controller actions and allow them to be invoked in custom commands
3) Allow checking for merge conflicts after running a custom command so that users can add their own 'git merge' custom command
that matches the in-built action
Option 1 is hairy, Option 2 though good for users introduces new backwards compatibility issues that I don't want to do
right now, and option 3 is trivially easy to implement so that's what I'm doing.
I've put this under an 'after' key so that we can add more things later. I'm imagining other things like being able to
move the cursor to a newly added item etc.
I considered always running this hook by default but I'd rather not: it's matching on the output text and I'd rather something
like that be explicitly opted-into to avoid cases where we erroneously believe that there are conflicts.
It seems that older git versions would drop empty commits when rebasing. Since
this aspect is not relevant to what we're testing here, fix this by simply
avoiding empty commits in these tests.
The test apply_in_reverse_with_conflict.go fails in git versions 2.30.8 and
earlier. Apparently the output "Applied patch to 'file2' cleanly" was only added
more recently. It's not essential that we check this output.
Save has been deprecated for a while, push is the recommended way to save a
stash. Push has been available since 2.13, so we can use it without problems.
Older versions of git don't support the -b option yet. However, no version of
git complains about the -c option, even when the init.defaultBranch config is
not supported.
For older git versions we won't be able to support any other main branch than
"master", so hard-code that in Init.
This doesn't fix anything for older versions yet; see the next commit for that.
Now that we are running each test 6 times on CI, the risk of flakiness
is higher. I want to fix these tests for good but it'l take time, so
we're just retrying for now
The code in getHydratedRebasingCommits relied on the assumption that the
git-rebase-todo file contains full SHAs. This has only been true from 2.25.2 on,
before that it would contain abbreviated SHAs. Fix this by storing fullCommits
in a slice instead of a map, and using a linear search.
I don't know why we're getting index.lock errors but they're impossile to stop
anyway given that other processes can be calling git commands. So we're retrying
a few times before re-raising. To do this we need to clone the command and the current
implementation for that is best-effort.
I do worry about the maintainability of that but we'll see how it goes.
Also, I thought you'd need to clone the task (if it exists) but now I think not;
as long as you don't call done twice on it you should be fine, and you shouldn't
be done'ing a task as part of running a command: that should happen higher up.
It's not clear what was happening but it seemed like we sometimes weren't
fully writing to our stdout buffer (which is used for the error message)
even though we had returned from cmd.Wait().
Not sure what the cause was but removing an unnecessary goroutine fixed it.
I've simplifiied the code because it was too complex for the current requirements, and this fixed the misc/initial_open
test which was occasionally failing due to a race condition around busy tasks
The global counter approach is easy to understand but it's brittle and depends on implicit behaviour that is not very discoverable.
With a global counter, if any goroutine accidentally decrements the counter twice, we'll think lazygit is idle when it's actually busy.
Likewise if a goroutine accidentally increments the counter twice we'll think lazygit is busy when it's actually idle.
With the new approach we have a map of tasks where each task can either be busy or not. We create a new task and add it to the map
when we spawn a worker goroutine (among other things) and we remove it once the task is done.
The task can also be paused and continued for situations where we switch back and forth between running a program and asking for user
input.
In order for this to work with `git push` (and other commands that require credentials) we need to obtain the task from gocui when
we create the worker goroutine, and then pass it along to the commands package to pause/continue the task as required. This is
MUCH more discoverable than the old approach which just decremented and incremented the global counter from within the commands package,
but it's at the cost of expanding some function signatures (arguably a good thing).
Likewise, whenever you want to call WithWaitingStatus or WithLoaderPanel the callback will now have access to the task for pausing/
continuing. We only need to actually make use of this functionality in a couple of places so it's a high price to pay, but I don't
know if I want to introduce a WithWaitingStatusTask and WithLoaderPanelTask function (open to suggestions).
We had some test flakiness involving the index.lock file which is fixed by this commit.
We shouldn't be accessing newTaskID without the mutex, although I'm surprised that this
actually fixes the issue. Surely we don't have tasks (which typically render to the main
view) which use index.lock?
We had a race condition due to refreshing branches in two different places, one which refreshed reflog commits
beforehand. The race condition meant that upon load we wouldn't see recency values (provided by the reflog commits)
against the branches
I want to see how we go removing all retry logic within a test. Lazygit should be trusted to tell us when it's no longer busy,
and if it that proves false we should fix the issue in the code rather than being lenient in the tests
Turns out we're just running our refresh functions one after the other which isn't ideal but we can fix that separately.
As it stands this wait group isn't doing anything.
I don't know if this is a hack or not: we run a git command and increment the pending action
count to 1 but at some point the command requests a username or password, so we need to prompt
the user to enter that. At that point we don't want to say that there is a pending action,
so we decrement the action count before prompting the user and then re-increment it again afterward.
Given that we panic when the counter goes below zero, it's important that it's not zero
when we run the git command (should be impossible anyway).
I toyed with a different approach using channels and a long-running goroutine that
handles all commands that request credentials but it feels over-engineered compared to this
commit's approach.
The remote branches controller was using its own escape method meaning it didn't go through the flow of cancelling
an active filter. It's now using the same approach as the sub-commits and commit-files contexts: defining a parent
context to return to upon hittin escape.
Given that we now persist search/filter states even after a side context loses focus, we need to make it really
clear to the user that the context is currently being searched/filtered
This is a pickle: initially I wanted it so that a filter would cancel automatically if the current context lost focus.
But there are situations where you want to retain the focus, e.g. when a popup appears, or when you view the commits
of a branch. The issue is that when you view the commits of a branch, the branches context is removed from the context
stack. Even if this were not the case, you could imagine going branches -> sub-commits -> files -> sub-commits, where
in that case branches would definitely be off the stack upon navigating to the files context.
So because I'm too lazy to find a proper solution to this problem, I'm just making it so that filters in side contexts
are retained unless explicitly cancelled.
There's another edge case this commit handles which is that if I'm in the sub-commits context via the branches context
and start a search, then navigate to the reflog context and hit enter to get to the sub-commits context again, I need
to cancel the search before I switch. Likewise with the commit files context.
The first line of the diff pane would show branch heads (e.g.
commit dd9100ccc8b69a8b14b21a84e34854b5acfb871a (mybranch, origin/mybranch)
only when a pager is used. The reason is that the default of the --decorate
option to git show is "auto", which means to show the decoration only when
output goes to a tty. Lazygit uses a pty only when a pager is used, so the
decoration wouldn't show when no pager is used.
Since the branch head annotation is useful and we always want to see it, force
it by explicitly passing --decorate.
This solves three problems:
1. When the local main branch is behind its upstream, the merged status of
commits of a feature branch sitting on origin/main was not correct. This can
easily happen when you rebase a branch onto origin/main instead of main, and
don't bother keeping local main up to date.
2. It works when you don't have the main branch locally at all. This could
happen when you check out a colleague's feature branch that goes off of
"develop", but you don't have "develop" locally yourself because you normally
only work on "main".
3. It also works when you work on a main branch itself, e.g. by committing to it
directly, or by merging a branch locally. These local commits on a main
branch would previously be shown in green instead of red; this broke with
910a61dc46.
For consistency with the previous commit.
Note that this menu entry is used both for unstaged and for staged changes, and
for staged changes it is not quite accurate, as we are not discarding changes in
that case (just unstaging them). Not sure it's worth fixing this; it's still
better than "Delete", anyway.
The title was saying "Unstage lines", which was just wrong. The text said
"Delete lines", which can be seen as a bit misleading; we are only discarding
the changes to the selected lines, not deleting the lines themselves.
For consistency, rename the config variable skipUnstageLineWarning accordingly.
The assert package is already very good at displaying errors, including printing
a diff of expected and actual value, so there's no point in printing the same
information again ourselves.
This test is almost identical to swap_in_rebase_with_conflict.go, except that it
sets the commit that will conflict to "edit".
This test is interesting because there's special code needed to determine
whether an "edit" command conflicted or not, i.e. whether to show the "confl"
entry. In this case we do. We have lots of other tests already that have "edit"
commands that don't conflict, so that's covered already.
When stopping in a rebase because of a conflict, it is nice to see the commit
that git is trying to apply. Create a fake todo entry labelled "conflict" for
this, and show the "<-- YOU ARE HERE ---" string for that one (in red) instead
of for the real current head.
This test is interesting because it already behaves as desired: since git has
rescheduled the "pick" command, we do _not_ want to show a "conflict" entry in
this case, as we would see the same commit twice then.
We don't actually use it to do map lookups; we still iterate over it in the same
way as before. However, using a map makes it easier to patch elements; see the
next commit.
We use CommitFilesController also for the files of commits that we show
elsewhere, e.g. for branch commits, tags, or stashes. It doesn't make sense to
discard changes from those (for stashes it might be possible to implement it
somehow, but that would be a new feature), so we disallow it unless we are in
the local commits panel.
Discarding changes to an entire directory doesn't quite work correctly in all
cases; for example, if the current commit added files to the directory (but the
directory existed before) then those files won't be removed.
It might be possible to fix the command so that these cases always work for
directories, but I don't think it's worth the effort (you can always use a
custom patch for that), so let's display an error for now.
I don't know why we were setting the initial context to CurrentSideContext
and not just CurrentContext in the first place. If there is no current context
in either case it'll default to the files context. So the only issue is if
we anticipated that some random context would be focused and we didn't want to
activate that. But I can't think of any situation where that would happen.
Whenever we perform an action in a test, we should assert on the result before doing the next action.
This prevents issues where the test moves too fast for our code. It would be nice to not have to do this,
but for now that's the situation
A better refactor would be to allow matchers to assert against either a string or a slice of cells, so that I could have
the same ergonomics that I have elsewhere, but this is a start.
The root commit is special in that it has no parents. So we need to add a pipe that's headed for a commit
that doesn't actually exist i.e. the mythical empty tree commit. We're using the actual hash of that
pseudo-commit, but it's not being read anywhere.
