For consistency with the previous commit.
Note that this menu entry is used both for unstaged and for staged changes, and
for staged changes it is not quite accurate, as we are not discarding changes in
that case (just unstaging them). Not sure it's worth fixing this; it's still
better than "Delete", anyway.
The title was saying "Unstage lines", which was just wrong. The text said
"Delete lines", which can be seen as a bit misleading; we are only discarding
the changes to the selected lines, not deleting the lines themselves.
For consistency, rename the config variable skipUnstageLineWarning accordingly.
The assert package is already very good at displaying errors, including printing
a diff of expected and actual value, so there's no point in printing the same
information again ourselves.
This test is almost identical to swap_in_rebase_with_conflict.go, except that it
sets the commit that will conflict to "edit".
This test is interesting because there's special code needed to determine
whether an "edit" command conflicted or not, i.e. whether to show the "confl"
entry. In this case we do. We have lots of other tests already that have "edit"
commands that don't conflict, so that's covered already.
When stopping in a rebase because of a conflict, it is nice to see the commit
that git is trying to apply. Create a fake todo entry labelled "conflict" for
this, and show the "<-- YOU ARE HERE ---" string for that one (in red) instead
of for the real current head.
This test is interesting because it already behaves as desired: since git has
rescheduled the "pick" command, we do _not_ want to show a "conflict" entry in
this case, as we would see the same commit twice then.
We don't actually use it to do map lookups; we still iterate over it in the same
way as before. However, using a map makes it easier to patch elements; see the
next commit.
We use CommitFilesController also for the files of commits that we show
elsewhere, e.g. for branch commits, tags, or stashes. It doesn't make sense to
discard changes from those (for stashes it might be possible to implement it
somehow, but that would be a new feature), so we disallow it unless we are in
the local commits panel.
Discarding changes to an entire directory doesn't quite work correctly in all
cases; for example, if the current commit added files to the directory (but the
directory existed before) then those files won't be removed.
It might be possible to fix the command so that these cases always work for
directories, but I don't think it's worth the effort (you can always use a
custom patch for that), so let's display an error for now.
I don't know why we were setting the initial context to CurrentSideContext
and not just CurrentContext in the first place. If there is no current context
in either case it'll default to the files context. So the only issue is if
we anticipated that some random context would be focused and we didn't want to
activate that. But I can't think of any situation where that would happen.
Whenever we perform an action in a test, we should assert on the result before doing the next action.
This prevents issues where the test moves too fast for our code. It would be nice to not have to do this,
but for now that's the situation
A better refactor would be to allow matchers to assert against either a string or a slice of cells, so that I could have
the same ergonomics that I have elsewhere, but this is a start.
The root commit is special in that it has no parents. So we need to add a pipe that's headed for a commit
that doesn't actually exist i.e. the mythical empty tree commit. We're using the actual hash of that
pseudo-commit, but it's not being read anywhere.
The menuFromCommand option is a little complicated, so I'm adding an easy way to just use the command output directly,
where each line becomes a suggestion, as-is.
Now that we support suggestions in the input prompt, there's less of a need for menuFromCommand, but it probably still
serves some purpose.
In future I want to support this filter/valueFormat/labelFormat thing for suggestions too. I would like to think a little more
about the interface though: is using a regex like we currently do really the simplest approach?
We have not been good at consistent casing so far. Now we use 'Sentence case' everywhere. EVERYWHERE.
Also Removing 'Lc' prefix from i18n field names: the 'Lc' stood for lowercase but now that everything
is in 'Sentence case' there's no need for the distinction.
I've got a couple lower case things I've kept: namely, things that show up in parentheses.
I'll be honest, for all I know logging should be global in general: it is
a pain to pass a logger to any struct that needs it. But smart people on the
internet tell me otherwise, and I do like the idea of not having any global
variables lying around.
Nonetheless, I often need to log things when locally debugging and that's a
different kind of logging than the kind you would include in the actual
released binary. For example if I want to log something from gocui, I would
rather not have gocui depend on lazygit's logging setup.
Go really doesn't like us doing anything inheritance-y: it does not support open recursion meaning
it's really hard to re-use code. As such, here we're falling back to conditional logic.
This fixes an issue where our ListContextTrait was calling FocusLine which was intended to be
overridden by ViewportListContextTrait, but the subclassed function wasn't being called. I'm
not actually sure how this went wrong given that it was working fine in the past, but at any rate,
the new code is easy to follow.
Missed a spot a couple PR's ago. We had an integration test which caught this but which was skipped due
to index.lock file issues. The test was also broken for other reasons due to it not having been running
for a while, so I've fixed that up too.
By constructing an arg vector manually, we no longer need to quote arguments
Mandate that args must be passed when building a command
Now you need to provide an args array when building a command.
There are a handful of places where we need to deal with a string,
such as with user-defined custom commands, and for those we now require
that at the callsite they use str.ToArgv to do that. I don't want
to provide a method out of the box for it because I want to discourage its
use.
For some reason we were invoking a command through a shell when amending a
commit, and I don't believe we needed to do that as there was nothing user-
supplied about the command. So I've switched to using a regular command out-
side the shell there
If a given menu item has an associated keybinding of 'enter', hitting enter won't actually execute
that item unless your cursor is on it. This creates confusion, and so we're going to use a strikethrough
style to communicate that the keybinding is reserved for something else.
The reason for this is that now our labels for navigation keybindings are larger so they
take up more realestate. It's not the kind of thing a user needs to be told anyway,
anybody is going to try out hjkl and the arrow keys when a TUI opens up.
We could map from <up> to the single character up unicode rune but given you can rebind this stuff
I'd rather keep it simple
The only exception is when moving a custom patch for an entire commit to an
earlier commit; in this case the source commit becomes empty, but we want to
keep it, mainly for consistency with moving the patch to a later commit, which
behaves the same.
In all other cases where we rebase, it's confusing when empty commits are kept;
the most common example is rebasing a branch onto master, where master already
contains some of the commits of our branch. In this case we simply want to drop
these.
The option doesn't have any affect in these views, so we don't need to toggle it
here. But the problem was the HandleFocus call at the end: this would activate
the wrong view, so we need to avoid it here.
Show an error if the user tries to turn the option on, to let them know that it
doesn't work here.
It's not possible to reliably stage things into a custom patch when "ignore
whitespace" is on, so always treat it as off here (like we do in the staging
panel).
It looks like this is a regression that was introduced in 8edad826ca.
It defaults to {"master", "main"}, but can be set to whatever branch names
are used as base branches, e.g. {"master", "devel", "v1.0-hotfixes"}. It is
used for color-coding the shas in the commit list, i.e. to decide whether
commits are green or yellow.
Our refresh code may try to push a context. It does this in two places:
1) when all merge conflicts are resolved, we push a 'continue merge?' confirmation context
2) when all conflicts of a given file are resolved and we're in the merge conflicts context,
we push the files context.
Sometimes we push the confirmation context and then push the files context over it, so the user
never sees the confirmation context.
This commit fixes the race condition by adding a check to ensure that we're still in the
merge conflicts panel before we try escaping from it
Previously, when rebasing a branch onto a newer master, all commits from the
previous fork point up to its head were marked red (unpushed), including the
commits that are on master already. While this is technically correct from the
perspective of the current branch's upstream, it's not what most people expect,
intuitively; they want to see where the current branch starts, relative to
master. So all commits of master should be green, and then the commits of the
current branch in red.
I don't see a reason why this restriction to have the selection be always
visible was necessary. Removing it has two benefits:
1. Scrolling a list view doesn't change the selection. A common scenario: you
look at one of the commits of your current branch; you want to see the how
many'th commit this is, but the beginning of the branch is scrolled off the
bottom of the commits panel. You scroll down to find the beginning of your
branch, but this changes the selection and shows a different commit now - not
what you want.
2. It is possible to scroll a panel that is not the current one without changing
the focus to it. That's how windows in other GUIs usually behave.
Enabling the filter selects the first entry in the filtered commits view. It's
useful to have a test that checks this, as I almost broke it in the following
commit (it needs an added FocusLine call in the setFiltering function in
filtering_menu_action.go).
We now refresh the staging panel when doing an unscoped refresh, so that if we commit from the staging panel we escape
back to the files panel if need be. But that causes flickering when doing an unscoped refresh from other contexts,
because the refreshStagingPanel function assumes that the staging panel has focus. So we're adding a guard at the top
of that function to early exit if we don't have focus.
When cycling history, we want to make it so that upon returning to the original prompt, you get your text back.
Importantly, we don't want to use the existing preservedMessage field for that because that's only for preserving
a NEW commit message, and we don't want the history stuff of the commit reword flow to overwrite that.
When we use the one panel for the entire commit message, its tricky to have a keybinding both for adding a newline and submitting.
By having two panels: one for the summary line and one for the description, we allow for 'enter' to submit the message when done from the summary panel,
and 'enter' to add a newline when done from the description panel. Alt-enter, for those who can use that key combo, also works for submitting the message
from the description panel. For those who can't use that key combo, and don't want to remap the keybinding, they can hit tab to go back to the summary panel
and then 'enter' to submit the message.
We have some awkwardness in that both contexts (i.e. panels) need to appear and disappear in tandem and we don't have a great way of handling that concept,
so we just push both contexts one after the other, and likewise remove both contexts when we escape.
This begins a big refactor of moving more code out of the Gui struct into contexts, controllers, and helpers. We also move some code into structs in the
gui package purely for the sake of better encapsulation
We now refresh the staging panel when doing an unscoped refresh, so that if we commit from the staging panel we escape
back to the files panel if need be. But that causes flickering when doing an unscoped refresh from other contexts,
because the refreshStagingPanel function assumes that the staging panel has focus. So we're adding a guard at the top
of that function to early exit if we don't have focus.
When cycling history, we want to make it so that upon returning to the original prompt, you get your text back.
Importantly, we don't want to use the existing preservedMessage field for that because that's only for preserving
a NEW commit message, and we don't want the history stuff of the commit reword flow to overwrite that.
When we use the one panel for the entire commit message, its tricky to have a keybinding both for adding a newline and submitting.
By having two panels: one for the summary line and one for the description, we allow for 'enter' to submit the message when done from the summary panel,
and 'enter' to add a newline when done from the description panel. Alt-enter, for those who can use that key combo, also works for submitting the message
from the description panel. For those who can't use that key combo, and don't want to remap the keybinding, they can hit tab to go back to the summary panel
and then 'enter' to submit the message.
We have some awkwardness in that both contexts (i.e. panels) need to appear and disappear in tandem and we don't have a great way of handling that concept,
so we just push both contexts one after the other, and likewise remove both contexts when we escape.
This broke with 40f6767cfc77; the symptom is that starting lazygit with a git
arg (e.g. "lazygit log") wouldn't activate the requested panel correctly. While
it would show in the expanded view, it didn't have a green frame, and keyboard
events would go to the files panel.
This fixes two problems with the "amend commit with staged changes" command:
1. Amending to a fixup commit didn't work (this would create a commmit with the
title "fixup! fixup! original title" and keep that at the top of the branch)
2. Unrelated fixup commits would be squashed too.
The added integration test verifies that both of these problems are fixed.
Instead of passing a bunch of different options in
PrepareInteractiveRebaseCommandOpts, where it was unclear how they interact if
several are set, have only a single field "instruction" which can be set to one
of various different instructions.
The functionality of replacing the entire todo file with our own is no longer
available; it is only possible to prepend todos to the existing file.
Also, instead of using different env vars for the various rebase operations that
we want to tell the daemon to do, use a single one that contains a json-encoded
struct with all available instructions. This makes the protocol much clearer,
and makes it easier to extend in the future.
At the moment it doesn't make a big difference, because the vast majority of
callers create a list of todos themselves to completely replace what git came up
with. We're changing this in the following commits though, and then it's helpful
to preserve merges.
We already show "merge" todo entries when starting an interactive rebase with
--rebase-merges outside of lazygit. Changing the type of a merge entry to "pick"
or "edit" doesn't make sense and shouldn't be allowed. Earlier in this branch we
have started to show "update-ref" entries, these can't be changed either (they
can be moved, though).
You might argue that it should be possible to change them to "drop", but in the
case of "update-ref" this doesn't make sense either, because "drop" needs a Sha
and we don't have one here. Also, you would then be able to later change it back
to "pick", so we would have to remember that this isn't allowed for this
particular drop entry; that's messy, so just disallow all editing.
It used to work on the assumption that rebasing commits in lazygit's model
correspond one-to-one to lines in the git-rebase-todo file, which isn't
necessarily true (e.g. when users use "git rebase --edit-todo" at the custom
command prompt and add a "break" between lines).
This is useful when working with stacked branches, because you can now move
"pick" entries across an update-ref command and you can tell exactly which
branch the commit will end up in.
It's also useful to spot situations where the --update-refs option didn't work
as desired. For example, if you duplicate a branch and want to rebase only one
of the branches but not the other (maybe for testing); if you have
rebase.updateRefs=true in your git config, then rebasing one branch will move
the other branch along. To solve this we'll have to introduce a way to delete
the update-ref entry (maybe by hitting backspace?); this is out of scope for
this PR, so for now users will have to type "git rebase --edit-todo" into the
custom command prompt to sort this out.
We will also have to prevent users from trying to turn update-ref commands into
other commands like "pick" or "drop"; we'll do this later in this branch.
So far the algorithm worked on the assumption that the output of the "git show"
command corresponds one-to-one to the lines of the rebase-todo file. This
assumption doesn't hold once we start to include todo lines that don't have a
sha (like update-ref), or when the todo file contains multiple entries for the
same sha. This should never happen normally, but it can if users manually edit
the todo file and duplicate a line.
The main reason for doing this (besides the reasons given for Status in the
previous commit) is that it allows us to easily convert from TodoCommand to
Action and back. This will be needed later in the branch. Fortunately,
TodoCommand is one-based, so this allows us to add an ActionNone constant with
the value 0.
This is unrelated to the changes in this PR, but since we are doing the same
thing for the commit.Action field in the next commit, it makes sense to do it
for Status too for consistency. Modelling this as an enum feels more natural
than modelling it as a string, since there's a finite set of possible values.
And it saves a little bit of memory (not very much, since none of the strings
were heap-allocated, but still).
We print this to the terminal after lazygit quits rather than showing it in a
panel at startup, so as to not annoy people too much. Hopefully it will still be
prominent enough this way.
Instead, query the platform defaults only if the config is empty. This will be
necessary later to distinguish an empty config from a default config, so that we
can give deprecation warnings.
The "open" command is supposed to behave in the same way as double-clicking a
file in the Finder/Explorer. The concept of jumping to a specific line in the
file doesn't make sense for this; use "edit" instead.
These files were renamed from os_windows_test.go to os_test_windows.go (etc.) in
95b2e9540a. Since then, the tests have no longer run, since go only looks for
tests in files ending with "test.go".
It isn't important that the file name ends with "_windows.go", since there are
already build constrains in the files themselves.
Pulled this out into a separate commit since it was unrelated to the
feature coming behind it.
This just cleans up the `commit_test.go` file slightly (for the method
that I was working on) so that the tests are built in a way that is
slightly more readable - testing each configuration option individually
without combining any of them.
Previously we would have tried to do the rebase, resulting in a long and
somewhat cryptic error message from git; now we check ourselves and show a less
intimidating message.
We refresh the view after reading just enough to fill it, so that we see the
initial content as quickly as possible, but then we continue reading enough
lines so that we can tell how long the scrollbar needs to be, and then we
refresh again. This can result in slight flicker of the scrollbar when it is
first drawn with a bigger size and then jumps to a smaller size; however, that's
a good tradeoff for a solution that provides both good speed and accuracy.
This fixes the problem that patching would stop at the first file that has a
conflict. We always want to patch all files.
Also, it's faster for large patches, and the code is a little bit simpler too.
The patch contains changes to two files; the first one conflicts, the second
doesn't. Note how it only applies changes to the first file at this point in the
branch; we'll fix this in the next commit.
This test would fail on master for multiple reasons.
All callers in this file now use reverseOnGenerate=false and
keepOriginalHeader=true, so hard-code that in the call to ModifiedPatchForLines
and get rid of the parameters.
The loop is pointless for two reasons:
- git apply --3way has this fallback built in already. If it can't do a
three-way merge, it will fall back to applying the patch normally.
- However, the only situation where it does this is when it can't do a 3-way
merge at all because it can't find the necessary ancestor blob. This can only
happen if you transfer a patch between different repos that don't have the
same blobs available; we are applying the patch to the same repo that is was
just generated from, so a 3-way merge is always possible. (Now that we fixed
the bug in the previous commit, that is.)
But the retry loop is not only pointless, it was actually harmful, because when
a 3-way patch fails with a conflict, git will put conflict markers in the
patched file and then exit with a non-zero exit status. So the retry loop would
try to patch the already patched file again, and this almost certainly fails,
but with a cryptic error message such as "error: main.go: does not exist in
index".
There's no reason to have two different ways of applying patches for whole-file
patches and partial patches; use --reverse for both. Not only does this simplify
the code a bit, but it fixes an actual problem: when reverseOnGenerate and
keepOriginalHeader are both true, the generated patch header is broken (the two
blobs in the line `index 6d1959b..6dc5f84 100644` are swapped). Git fails to do
a proper three-way merge in that case, as it expects the first of the two blobs
to be the common ancestor.
It would be possible to fix this by extending ModifiedPatchForLines to swap the
two blobs in this case; but this would prevent us from concatenating all patches
and apply them in one go, which we are going to do later in the branch.
We are going to add one more flag in the next commit.
Note that we are not using the struct inside patch_manager.go; we keep passing
the individual flags there. The reason for this will become more obvious later
in this branch.
This is the working tree state at the time the model commits were loaded. This
avoids a visual glitch with the "You Are Here" label appearing at times when it
is not supposed to.
Instead, derive it from context at display time (if we're rebasing, it's the
first non-todo commit). This fixes the problem that unfolding the current
commit's files in the local commits panel would show junk in the frame's title.
Along the way we make sure to only display the "<--- YOU ARE HERE" string in the
local commits panel; previously it would show for the top commit of a branch or
tag if mid-rebase.
If you ran this test enough times it would eventually fail; this happened
whenever the resulting squashed commit had a sha that happened to start with
"02". We test that "commit 02" does not appear in the diff window, but in that
case it did, at the very top of the window.
A better fix might be to change the commit message that we use in CreateNCommits
to something other than "commit XY", but that would require touching tons of
tests, so this is the easier fix.
It's not so much the total number of commits that matters here, it's just
whether we are on the first one. (This includes the other condition.)
This allows us to get rid of the condition in rebase.go.
