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%).
