We treat the .git/sequencer/todo file as read-only. Technically it seems it
would be possible to treat it as modifiable in the same way as
.git/rebase-merge/git-rebase-todo, effectively turning a cherry-pick or revert
that stops at a conflict into an interactive rebase; however, git itself doesn't
allow this (there is no "git cherry-pick --edit-todo"), so it seems safer not to
rely on it.
Theoretically it would be possible to allow modifying the rebase todos when a
cherry-pick or revert conflicts in the middle of a rebase. However, it would
introduce a bit of complexity to support this, as we would have to be able to
distinguish between rebasing todos and cherry-picking/reverting todos, which we
currently can't; it could also be a bit error-prone as far as edge cases are
concerned. And it's really a pretty uncommon situation, so it doesn't seem worth
it, and we just forbid all modifications to todos whenever we are cherry-picking
or reverting.
What happens here is that when stopping on an "edit" todo entry, we rely on the
assumption that if the .git/rebase-merge/amend file exists, the command was
successful, and if it doesn't, there was a conflict. The problem is that when
you stop on an edit command, and then run a multi-commit cherry-pick or rebase,
this will delete the amend file. You may or may not consider this a bug in git;
to work around it, we also check the existence of the rebase-merge/message file,
which will be deleted as well by the cherry-pick or revert.
This problem can't happen inside of lazygit itself right now, but this will
change in the future. It will only happen when you stopped in an interactive
rebase on an "edit" entry, and then you perform a revert or cherry-pick
consisting of more than one commit; in this situation lazygit will show a
conflict although there is none.
This is not possible with lazygit yet, as we don't support range-select for
reverting, and we don't use `git cherry-pick` for cherry-picking. Both will
change in the future, so it's good to fix this bug.
It is useful to see if the conflicted commit was a "pick" or an "edit". What's
more, we're about to add support for showing cherry-picks and reverts, and
seeing that a conflicted commit was a revert is important because its diff is
backwards compared to the diff of the conflicting files in the Files panel.
When you are in the middle of a rebase, and you cherry-pick a commit which
conflicts, it helps to be clear on whether you are prompted to continue the
cherry-pick or the rebase.
The situation is that you perform a rebase, and one of the commits becomes empty
during the rebase, in a way that couldn't be predicted before the rebase
started. To explain: git rebase has some logic where it immediately discards
commits if it can tell that they already exist in the branch being rebased onto;
it does this by looking at the patch ids of the commits to work out which
commits already exist upstream. But for those commits that become empty even
though there isn't a corresponding commit upstream, git stops with an error, and
lazygit detects this (in CheckMergeOrRebaseWithRefreshOptions) and automatically
continues the rebase.
This all works fine; I'm adding this test because I almost broke this during
development of this branch, so I'm adding it to guard against regressions.
We keep the same commit selected (even though its index changed because of the
added update-ref todo), which is nice; however, the main view shows the diff of
the wrong commit, which is very confusing. I'm suprised that this hasn't been
noticed yet.
The reason why this happens is that we first do the refresh, which includes
re-rendering the main view diff (with the same commit index as before, so the
wrong one), and then we restore the correct commit index but don't render the
main view again.
The test demonstrates that the behavior is undesirable right now: we move the
commit only past the update-ref todo of branch1, which means the order of
commits stays the same and only the branch head icon moves up by one. However,
we move the selection down by one, so the wrong commit is selected now. This is
especially bad if you type a bunch of ctrl-j quickly in a row, because now you
are moving the wrong commit.
There are two possible ways to fix this:
1) keep the moving behavior the same, but don't change the selection
2) change the behavior so that we move the commit not only past the update-ref,
but also past the next real commit.
You could argue that 1) is the more desirable fix, as it gives you more control
over where exactly the moved commit goes; however, it is much trickier to
implement, so we go with 2) for now (and that's what the commented-out
"EXPECTED" section documents here). If users need more fine-grained control,
they can always enter an interactive rebase first.
For non-merge commits we change "pick" to "drop" when we delete them. We do this
so that we can use the same code for dropping a commit no matter whether we are
in an interactive rebase or not. (If we aren't, we could just as well delete the
pick line from the todo list instead of setting it to "drop", but if we are, it
is better to keep the line around so that the user can change it back to "pick"
if they change their mind.)
However, merge commits can't be changed to "drop", so we have to delete them
from the todo file. We add a new daemon instruction that does this.
We still don't allow deleting a merge commit from within an interactive rebase.
The reason is that we don't show the "label" and "reset" todos in lazygit, so
deleting a merge commit would leave the commits from the branch that is being
merged in the list as "pick" commits, with no indication that they are going to
be dropped because they are on a different branch, and the merge commit that
would have brought them in is gone. This could be very confusing.
In 67b8ef449c we changed the "edit" command to insert a "break" after the
selected commit, rather than setting the selected todo to "edit". The reason for
doing this was that it now works for merge commits too.
Back then, I claimed "In most cases the behavior is exactly the same as before."
Unfortunately that's not true, there are two reasons why the previous behavior
was better (both are demonstrated by tests earlier in this branch):
- when editing the last commit of a branch in the middle of a stack of branches,
we are now missing the update-ref todo after it, which means that amending the
commit breaks the stack
- it breaks auto-amending (see the added test earlier in this branch for an
explanation)
For these reasons, we are going back to the previous approach of setting the
selected commit to "edit" whenever possible, i.e. unless it's a merge commit.
The only scenario where this could still be a problem is when you have a stack
of branches, and the last commit of one of the branches in the stack is a merge
commit, and you try to edit that. In my experience with stacked branches this is
very unlikely, in almost all cases my stacked branches are linear.
This is very similar to edit_range_select_outside_rebase.go, except that it
selects commits right after, and including, a merge commit.
This test already works correctly. The reason we add it is that we are going to
have two different implementations of the `e` command depending on whether the
last selected commit is a merge commit, and we want to make sure they both work
with a range selection.
Auto-amending is a little-known feature of git that is very convenient once you
know it: whenever you stop at a commit marked with `edit` in an interactive
rebase, you can make changes and stage them, and when you continue the rebase
they automatically get amended to the commit you had stopped at. This is so
convenient because making changes to a commit is one of the main reasons why you
edit a commit.
Unfortunately this currently doesn't work in lazygit because we don't actually
use `edit` to stop at the first commit (instead, we add a `break` todo after it,
which doesn't have the auto-amend functionality).
We'll improve this later in this branch.
Unfortunately it isn't possible to delete them. This would often be useful, but
our todo rewriting mechanisms rely on being able to find todos by some
identifier (hash for pick, ref for update-ref), and exec todos don't have a
unique identifier.
Put it into the individual menu items instead.
Again, this is necessary because we are going to add another entry to the menu
that is independent of the selected branch.
The rebase.updateRefs feature of git is very useful to rebase a stack of
branches and keep everything nicely stacked; however, it is usually in the way
when you make a copy of a branch and want to rebase it "away" from the original
branch in some way or other. For example, the original branch might sit on main,
and you want to rebase the copy onto devel to see if things still compile there.
Or you want to do some heavy history rewriting experiments on the copy, but keep
the original branch in case the experiments fail. Or you want to split a branch
in two because it contains two unrelated sets of changes; so you make a copy,
and drop half of the commits from the copy, then check out the original branch
and drop the other half of the commits from it.
In all these cases, git's updateRefs feature insists on moving the original
branch along with the copy in the first rebase that you make on the copy. I
think this is a bug in git, it should create update-ref todos only for branches
that point into the middle of your branch (because only then do they form a
stack), not when they point at the head (because then it's a copy). I had a long
discussion about this on the git mailing list [1], but people either don't agree
or don't care enough.
So we fix this on our side: whenever we start a rebase for whatever reason, be
it interactive, non-interactive, or behind-the-scenes, we drop any update-ref
todos that are at the very top of the todo list, which fixes all the
above-mentioned scenarios nicely.
I will admit that there's one scenario where git's behavior is the desired one,
and the fix in this PR makes it worse: when you create a new branch off of an
existing one, with the intention of creating a stack of branches, but before you
make the first commit on the new branch you realize some problem with the first
branch (e.g. a commit that needs to be reworded or dropped). It this case you do
want both branches to be affected by the change. In my experience this scenario
is much rarer than the other ones that I described above, and it's also much
easier to recover from: just check out the other branch again and hard-reset it
to the rebased one.
[1]
https://public-inbox.org/git/354f9fed-567f-42c8-9da9-148a5e223022@haller-berlin.de/
It is a bad idea to read a git-rebase-todo file, remove some update-ref todos,
and write it back out behind git's back. This will cause git to actually remove
the branches referenced by those update-ref todos when the rebase is continued.
The reason is that git remembers the refs affected by update-ref todos at the
beginning of the rebase, and remembers information about them in the file
.git/rebase-merge/update-refs. Then, whenever the user performs a "git rebase
--edit-todo" command, it updates that file based on whether update-ref todos
were added or removed by that edit. If we rewrite the git-rebase-todo file
behind git's back, this updating doesn't happen.
Fix this by not updating the git-rebase-todo file directly in this case, but
performing a "git rebase --edit-todo" command where we set ourselves as the
editor and change the file in there. This makes git update the bookkeeping
information properly.
Ideally we would use this method for all cases where we change the
git-rebase-todo file (e.g. moving todos up/down, or changing the type of a
todo); this would be cleaner because we wouldn't mess with git's private
implementation details. I tried this, but unfortunately it isn't fast enough.
Right now, moving a todo up or down takes between 1 and 2ms on my machine;
changing it to do a "git rebase --edit-todo" slows it down to over 100ms, which
is unacceptable.
In the test we simply removed the update-ref todo but didn't make any other
changes to the todos. This should really have kept everything the way it was,
including the other branch head. The fact that the star was gone was really
because of the bug that we are going to fix later in the branch.
Change the test so that it also makes a change before the update-ref todo; this
way we test that the star is gone because we deleted the update-ref, not because
of the bug.
To guard against the bug, we add another assertion for the branches view to test
that both branches are still there. This currently fails.
To support this, we turn the confirmation prompt of the "Create fixup commit"
command into a menu; creating a fixup commit is the first entry, so that
"shift-F, enter" behaves the same as before. But there are additional entries
for creating "amend!" commits, either with or without file changes. These make
it easy to reword commit messages of existing commits.
We have such a test already (squash_fixups_above_first_commit.go), but it can't
be used for what we want to check here, because it uses the first commit, and we
can't move down from there. So create a new one that basically does the same
thing, but for a commit in the middle. The focus of this new test is to check
how the selection behaves; as you can see, there is a problem both when creating
a fixup and when squashing fixups. We'll address these separately in the next
commits.
It starts a rebase on the bottom-most commit of the range, and sets all the
selected commits to "edit" (skipping merge commits, because they can't be
edited).
The additional branch head icon is more confusing than useful in this situation.
The update-ref entries show very clearly where the branch heads will go when
continuing the rebase; the information where the branch heads used to be before
the rebase is not really needed here, and just makes the display more confusing.
I'm not adding more tests here because the changes to the existing tests
demonstrate the change clearly enough.
To do that, change the "Apply fixup commits" command to show a menu with the two
choices "in current branch" and "above the selected commit"; we make "in current
branch" the default, as it's the more useful one most of the time, even though
it is a breaking change for those who are used to "shift-S enter" meaning
"squash above selected".
This adds a bunch of tooltips to keybindings and updates some keybinding descriptions (i.e. labels).
It's in preparation for displaying more keybindings on-screen (in the bottom right of the screen),
and so due in part to laziness it shortens some descriptions so that we don't need to manage both
a short and long description (for on-screen vs in-menu). Nonetheless I've added a ShortDescription
field for when we do want to have both a short and long description.
You'll notice that some keybindings I deemed unworthy of the options view have longer descriptions,
because I could get away with it.
This is useful if you want to move a range of commits, so you select them, and
then realize it's better to do it in an interactive rebase. Pressing 'i'
preserves the range now.
