Skip to content
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

Closed

Conversation

stefanhaller
Copy link
Collaborator

  • PR Description

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

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.

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.
@stefanhaller stefanhaller changed the title Fix support for old style non interactive rebase Fix support for old style non-interactive rebase Feb 26, 2024
@stefanhaller stefanhaller added the bug Something isn't working label Feb 26, 2024
Copy link

codacy-production bot commented Feb 26, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.06% (target: -2.00%) 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (af56065) 48350 40513 83.79%
Head commit (ddfec40) 48350 (+0) 40543 (+30) 83.85% (+0.06%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#3344) 4 4 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

@@ -69,7 +69,7 @@ func GetCommitListDisplayStrings(
}

// this is where my non-TODO commits begin
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

@stefanhaller
Copy link
Collaborator Author

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.

@jesseduffield
Copy link
Owner

jesseduffield commented Mar 2, 2024

I'm cool with this I'm cool with the PR that deletes the code

@stefanhaller
Copy link
Collaborator Author

Closing, we'll do #3348 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants