Skip to content

Commit d3fb22a

Browse files
stefanhallerstefanhaller
authored
Fix lazygit getting unresponsive when pasting commits that become empty (#4958)
Lazygit would become unresponsive when cherry-picking a commit that becomes empty at the destination. There are two ways this can happen: one where a cherry-picked commit simply becomes empty "by itself", because the change is already present on the destination branch (this was only a problem with git versions older than 2.45), and another where the cherry-pick stops with conflicts, and the user resolves them such that no changes are left, and then continues the cherry-pick. This would still fail even with git 2.45 or later. The fix is the same for both cases though. Fixes #4763.
2 parents 9adcc8e + 3ac9003 commit d3fb22a

File tree

4 files changed

+194
-1
lines changed

4 files changed

+194
-1
lines changed

‎pkg/gui/controllers/helpers/merge_and_rebase_helper.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ func (self *MergeAndRebaseHelper) CheckMergeOrRebaseWithRefreshOptions(result er
157157
} else if strings.Contains(result.Error(), "No changes - did you forget to use") {
158158
return self.genericMergeCommand(REBASE_OPTION_SKIP)
159159
} else if strings.Contains(result.Error(), "The previous cherry-pick is now empty") {
160-
return self.genericMergeCommand(REBASE_OPTION_CONTINUE)
160+
return self.genericMergeCommand(REBASE_OPTION_SKIP)
161161
} else if strings.Contains(result.Error(), "No rebase in progress?") {
162162
// assume in this case that we're already done
163163
return nil

‎pkg/integration/tests/cherry_pick/cherry_pick_commit_that_becomes_empty.go‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
package cherry_pick
2+
3+
import (
4+
"github.com/jesseduffield/lazygit/pkg/config"
5+
. "github.com/jesseduffield/lazygit/pkg/integration/components"
6+
)
7+
8+
var CherryPickCommitThatBecomesEmpty = NewIntegrationTest(NewIntegrationTestArgs{
9+
Description: "Cherry-pick a commit that becomes empty at the destination",
10+
ExtraCmdArgs: []string{},
11+
Skip: false,
12+
SetupConfig: func(config *config.AppConfig) {},
13+
SetupRepo: func(shell *Shell) {
14+
shell.
15+
EmptyCommit("base").
16+
CreateFileAndAdd("file1", "change 1\n").
17+
CreateFileAndAdd("file2", "change 2\n").
18+
Commit("two changes in one commit").
19+
NewBranchFrom("branch", "HEAD^").
20+
CreateFileAndAdd("file1", "change 1\n").
21+
Commit("single change").
22+
CreateFileAndAdd("file3", "change 3\n").
23+
Commit("unrelated change").
24+
Checkout("master")
25+
},
26+
Run: func(t *TestDriver, keys config.KeybindingConfig) {
27+
t.Views().Branches().
28+
Focus().
29+
Lines(
30+
Contains("master").IsSelected(),
31+
Contains("branch"),
32+
).
33+
SelectNextItem().
34+
PressEnter()
35+
36+
t.Views().SubCommits().
37+
IsFocused().
38+
Lines(
39+
Contains("unrelated change").IsSelected(),
40+
Contains("single change"),
41+
Contains("base"),
42+
).
43+
Press(keys.Universal.RangeSelectDown).
44+
Press(keys.Commits.CherryPickCopy).
45+
Tap(func() {
46+
t.Views().Information().Content(Contains("2 commits copied"))
47+
})
48+
49+
t.Views().Commits().
50+
Focus().
51+
Lines(
52+
Contains("two changes in one commit").IsSelected(),
53+
Contains("base"),
54+
).
55+
Press(keys.Commits.PasteCommits).
56+
Tap(func() {
57+
t.ExpectPopup().Alert().
58+
Title(Equals("Cherry-pick")).
59+
Content(Contains("Are you sure you want to cherry-pick the 2 copied commit(s) onto this branch?")).
60+
Confirm()
61+
})
62+
63+
if t.Git().Version().IsAtLeast(2, 45, 0) {
64+
t.Views().Commits().
65+
Lines(
66+
Contains("unrelated change"),
67+
Contains("single change"),
68+
Contains("two changes in one commit").IsSelected(),
69+
Contains("base"),
70+
).
71+
SelectPreviousItem()
72+
73+
// Cherry-picked commit is empty
74+
t.Views().Main().Content(DoesNotContain("diff --git"))
75+
} else {
76+
t.Views().Commits().
77+
// We have a bug with how the selection is updated in this case; normally you would
78+
// expect the "two changes in one commit" commit to be selected because it was
79+
// selected before pasting, and we try to maintain that selection. This is broken
80+
// for two reasons:
81+
// 1. We increment the selected line index after pasting by the number of pasted
82+
// commits; this is wrong because we skipped the commit that became empty. So
83+
// according to this bug, the "base" commit should be selected.
84+
// 2. We only update the selected line index after pasting if the currently selected
85+
// commit is not a rebase TODO commit, on the assumption that if it is, we are in a
86+
// rebase and the cherry-picked commits end up below the selection. In this case,
87+
// however, we still think we are cherry-picking because the final refresh after the
88+
// CheckMergeOrRebase in CherryPickHelper.Paste is async and hasn't completed yet;
89+
// so the "unrelated change" still has a "pick" action.
90+
//
91+
// Since this only happens for older git versions, we don't bother fixing it.
92+
Lines(
93+
Contains("unrelated change").IsSelected(),
94+
Contains("two changes in one commit"),
95+
Contains("base"),
96+
)
97+
}
98+
},
99+
})

‎pkg/integration/tests/cherry_pick/cherry_pick_conflicts_empty_commit_after_resolving.go‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
package cherry_pick
2+
3+
import (
4+
"github.com/jesseduffield/lazygit/pkg/config"
5+
. "github.com/jesseduffield/lazygit/pkg/integration/components"
6+
"github.com/jesseduffield/lazygit/pkg/integration/tests/shared"
7+
)
8+
9+
var CherryPickConflictsEmptyCommitAfterResolving = NewIntegrationTest(NewIntegrationTestArgs{
10+
Description: "Cherry pick commits with conflicts, resolve them so that the commit becomes empty",
11+
ExtraCmdArgs: []string{},
12+
Skip: false,
13+
SetupConfig: func(config *config.AppConfig) {
14+
config.GetUserConfig().Git.LocalBranchSortOrder = "recency"
15+
},
16+
SetupRepo: func(shell *Shell) {
17+
shared.MergeConflictsSetup(shell)
18+
},
19+
Run: func(t *TestDriver, keys config.KeybindingConfig) {
20+
t.Views().Branches().
21+
Focus().
22+
Lines(
23+
Contains("first-change-branch"),
24+
Contains("second-change-branch"),
25+
Contains("original-branch"),
26+
).
27+
SelectNextItem().
28+
PressEnter()
29+
30+
t.Views().SubCommits().
31+
IsFocused().
32+
TopLines(
33+
Contains("second-change-branch unrelated change"),
34+
Contains("second change"),
35+
).
36+
Press(keys.Universal.RangeSelectDown).
37+
Press(keys.Commits.CherryPickCopy)
38+
39+
t.Views().Information().Content(Contains("2 commits copied"))
40+
41+
t.Views().Commits().
42+
Focus().
43+
TopLines(
44+
Contains("first change").IsSelected(),
45+
).
46+
Press(keys.Commits.PasteCommits)
47+
48+
t.ExpectPopup().Alert().
49+
Title(Equals("Cherry-pick")).
50+
Content(Contains("Are you sure you want to cherry-pick the 2 copied commit(s) onto this branch?")).
51+
Confirm()
52+
53+
t.Common().AcknowledgeConflicts()
54+
55+
t.Views().Files().
56+
IsFocused().
57+
SelectedLine(Contains("file")).
58+
Press(keys.Universal.Remove)
59+
60+
t.ExpectPopup().Menu().
61+
Title(Equals("Discard changes")).
62+
Select(Contains("Discard all changes")).
63+
Confirm()
64+
65+
t.Common().ContinueOnConflictsResolved("cherry-pick")
66+
67+
t.Views().Files().IsEmpty()
68+
69+
t.Views().Commits().
70+
Focus().
71+
TopLines(
72+
// We have a bug with how the selection is updated in this case; normally you would
73+
// expect the "first change" commit to be selected because it was selected before
74+
// pasting, and we try to maintain that selection. This is broken for two reasons:
75+
// 1. We increment the selected line index after pasting by the number of pasted
76+
// commits; this is wrong because we skipped the commit that became empty. So
77+
// according to this bug, the "original" commit should be selected.
78+
// 2. We only update the selected line index after pasting if the currently selected
79+
// commit is not a rebase TODO commit, on the assumption that if it is, we are in a
80+
// rebase and the cherry-picked commits end up below the selection. In this case,
81+
// however, we still think we are cherry-picking because the final refresh after the
82+
// CheckMergeOrRebase in CherryPickHelper.Paste is async and hasn't completed yet;
83+
// so the "second-change-branch unrelated change" still has a "pick" action.
84+
//
85+
// We don't bother fixing it for now because it's a pretty niche case, and the
86+
// nature of the problem is only cosmetic.
87+
Contains("second-change-branch unrelated change").IsSelected(),
88+
Contains("first change"),
89+
Contains("original"),
90+
)
91+
},
92+
})

‎pkg/integration/tests/test_list.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,9 @@ var tests = []*components.IntegrationTest{
8686
branch.Suggestions,
8787
branch.UnsetUpstream,
8888
cherry_pick.CherryPick,
89+
cherry_pick.CherryPickCommitThatBecomesEmpty,
8990
cherry_pick.CherryPickConflicts,
91+
cherry_pick.CherryPickConflictsEmptyCommitAfterResolving,
9092
cherry_pick.CherryPickDuringRebase,
9193
cherry_pick.CherryPickMerge,
9294
cherry_pick.CherryPickRange,

0 commit comments

Comments
 (0)