-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix support for old style non-interactive rebase #3344
Fix support for old style non-interactive rebase #3344
Conversation
When doing a non-interactive rebase using a version of git earlier than 2.26, or by explicitly calling `git -c rebase.backend=apply rebase`, lazygit can display the pending todos by parsing the numbered patch files in `.git/rebase-apply/`. Unfortunately, support for this has been broken for more than three years because of the change in 682db77 (the string literal "normal" should have been changed to REBASE_MODE_NORMAL instead of REBASE_MODE_MERGING). It's not an important bug since you can only get into this situation by doing a rebase outside of lazygit, and then only with a pretty old git version. So instead of fixing the bug we might consider removing this code.
…ebases When stopping in an old-style non-interactive rebase (see previous commit for an explanation of what that means), the YOU ARE HERE marker should point to the first non-rebasing commit, but it was pointing at the first rebasing commit instead. To keep the tests working, we need to set all commits that have an Action to StatusRebasing, which matches what happens in the real code.
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 preferencesYou may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation |
pkg/gui/presentation/commits.go
Outdated
@@ -69,7 +69,7 @@ func GetCommitListDisplayStrings( | |||
} | |||
|
|||
// this is where my non-TODO commits begin |
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.
Is this comment still true then?
Also, "first non rebasing commit" is the (let's say) oldest commit, right?
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.
Is this comment still true then?
Only conceptually, not technically. Fixed in ddfec40.
Also, "first non rebasing commit" is the (let's say) oldest commit, right?
No, the first one in the list (from the top), i.e. the newest one.
I added a new PR that simply removes the code instead of fixing the bug: #3348. I'm undecided which way we want to go, but I slightly lean towards removing the code. |
|
Closing, we'll do #3348 instead. |
When doing a non-interactive rebase using a version of git earlier than 2.26, or by explicitly calling
git -c rebase.backend=apply rebase
, lazygit can display the pending todos by parsing the numbered patch files in.git/rebase-apply/
. Unfortunately, support for this has been broken for more than three years because of the change in 682db77 (the string literal "normal" should have been changed toREBASE_MODE_NORMAL
instead ofREBASE_MODE_MERGING
).Also, the position of the YOU ARE HERE marker was wrong for these old-style rebases.
It's not an important bug since you can only get into this situation by doing a rebase outside of lazygit, and then only with a pretty old git version. So instead of fixing the bug we might consider removing this code.