-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Continue/abort a conflicted cherry-pick or revert #4441
base: master
Are you sure you want to change the base?
Continue/abort a conflicted cherry-pick or revert #4441
Conversation
253b2f7
to
82ebc80
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
82ebc80
to
0badb30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. One thing
pkg/commands/types/enums/enums.go
Outdated
REBASE_MODE_REBASING | ||
REBASE_MODE_MERGING | ||
// this means we're neither rebasing nor merging, cherry-picking, or reverting | ||
WORKING_TREE_STATE_NONE WorkingTreeState = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like how we're using the same type (WorkingTreeState
) for the bits as for the bitfield. Naively I would expect that I could directly check if workingTreeState == WORKING_TREE_STATE_CHERRY_PICKING
but that's really only going to return true if we're only cherry picking and not also merging. I also can't do if workingTreeState == WORKING_TREE_STATE_CHERRY_PICKING || workingTreeState == WORKING_TREE_STATE_REBASING
to ask if it's either cherry picking or rebasing.
So I'm wondering, how about we just have a struct of booleans:
type WorkingTreeState struct {
rebasing bool
merging bool
cherryPicking bool
reverting bool
}
And then define methods on that struct. We can still have the .Effective()
method which returns the enum value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0badb30
to
5d957ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
When debugging an integration test that involves some behind-the-scenes rebasing, the daemon lazygit would also wait for a debugger to attach. This is very confusing because the test seems to hang; once you figured out what's going on, it's inconvenient because you need to attach a debugger to the daemon every time you debug the test. Now, it would sometimes be useful to be able to debug the daemon itself (whether inside an integration test, or during normal usage), and I have often wished to be able to do that. We might introduce an additional env var (and command-line option?) to enable this; but that's out of scope here.
All test cases set it to enums.REBASE_MODE_NONE, so we can simplify things a little bit by hard-coding that. This makes the changes in the following commits a little easier.
We want to get rid of RebaseMode, and using the more general WorkingTreeState will later allow us to also show cherry-pick or revert todos.
- Remove REBASE_MODE_NORMAL. It is not the "normal" mode anyway, rather a legacy mode; we have removed support for it in eb0f7e3, so there's no point in representing it in the enum. - Remove distinction between REBASE_MODE_REBASING and REBASE_MODE_INTERACTIVE; these are the same now. - Rename StatusCommands.IsInInteractiveRebase to IsInRebase. - Remove StatusCommands.RebaseMode; use StatusCommands.IsInRebase instead.
We're about to add more possible values (reverting and cherry-picking), so working tree state seems like a more suitable name.
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.
It looks like enums.go was supposed to be file that collects a bunch of enums, but actually there's only one in there, and since it has methods, it deserves to be in a file of its own, named after the type.
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.
This works already (except that the "you are here" marker is not right yet, but that's an issue for another PR), just add a test that verifies it.
This way we get the usual menu for viewing the conflicts or aborting, like for rebases.
5d957ce
to
2d897e1
Compare
This is part one of a four part series of PRs that improve the cherry-pick and revert experience.
With this first PR we make it possible to continue or abort a cherry-pick or revert operation, in the same way you can continue or abort a rebase or merge. Currently this is only relevant for revert, because lazygit doesn't use git cherry-pick for copying/pasting commits. This will change in a later PR in this series though, so here we are already preparing for that.
Fixes #1807
go generate ./...
)