Auto-select the conflicted commit when stopping in rebase (#5937)

When a rebase (or multi-commit cherry-pick or revert) stops with a
conflict, it is often useful to look at the diff of the "<-- CONFLICT"
commit to double-check that the conflict resolution matches the diff of
the original commit. To make that easier, select that commit
automatically.

pkg/gui/controllers/helpers/refresh_helper.go

@@ -861,9 +861,11 @@ func (self *RefreshHelper) refreshCommitsWithLimit(captured capturedCommitState,
 
 	self.onUIThreadUnlessRepoChanged(env, func() {
 		var selectionRange *localCommitSelectionRange
+		var newConflictedCommitIdx *int
 		if commitSelection == types.KeepCommitSelectionByHash {
 			selectedIdx, rangeStartIdx, rangeSelectMode := self.c.Contexts().LocalCommits.GetSelectionRangeAndMode()
 			selectionRange = captureLocalCommitSelectionRange(self.c.Model().Commits, selectedIdx, rangeStartIdx, rangeSelectMode)
+			newConflictedCommitIdx = findNewConflictedCommit(self.c.Model().Commits, commits)
 		}
 
 		self.c.Model().BisectInfo = bisectInfo
@@ -883,7 +885,9 @@ func (self *RefreshHelper) refreshCommitsWithLimit(captured capturedCommitState,
 				self.c.Contexts().LocalCommits.SetSelection(headCommitIdx)
 			}
 		case types.KeepCommitSelectionByHash:
-			if selectionRange != nil {
+			if newConflictedCommitIdx != nil {
+				self.c.Contexts().LocalCommits.SetSelection(*newConflictedCommitIdx)
+			} else if selectionRange != nil {
 				selectedIdx, rangeStartIdx, found := findLocalCommitSelectionRange(commits, selectionRange)
 				if found {
 					self.c.Contexts().LocalCommits.SetSelectionRangeAndMode(selectedIdx, rangeStartIdx, selectionRange.mode)
@@ -968,6 +972,24 @@ func hasRestorableCommitHash(commits []*models.Commit, idx int) bool {
 	return idx >= 0 && idx < len(commits) && commits[idx].Hash() != ""
 }
 
+// Returns the index of the conflicted commit in the new commits slice, if there is one and it has a
+// different hash than the one before had (or there wasn't one before). Otherwise returns nil.
+func findNewConflictedCommit(previousCommits []*models.Commit, commits []*models.Commit) *int {
+	previousConflictedCommit, _ := lo.Find(previousCommits, func(commit *models.Commit) bool {
+		return commit.Status == models.StatusConflicted
+	})
+
+	newConflictedCommit, idx, hasConflict := lo.FindIndexOf(commits, func(commit *models.Commit) bool {
+		return commit.Status == models.StatusConflicted
+	})
+
+	if hasConflict && (previousConflictedCommit == nil || previousConflictedCommit.Hash() != newConflictedCommit.Hash()) {
+		return &idx
+	}
+
+	return nil
+}
+
 // capturedSubCommitState holds the sub-commits refresh's model/context/mode
 // inputs, gathered on the UI thread (see captureSubCommitState) before the git
 // work is dispatched to a worker.

pkg/gui/controllers/helpers/refresh_helper_test.go

@@ -152,6 +152,62 @@ func TestFindLocalCommitSelectionRange(t *testing.T) {
 	}
 }
 
+func TestFindNewConflictedCommit(t *testing.T) {
+	testCases := []struct {
+		name            string
+		previousCommits []*models.Commit
+		commits         []*models.Commit
+		expectedIdx     *int
+	}{
+		{
+			name:            "finds a newly conflicted commit",
+			previousCommits: makeCommits("a", "b"),
+			commits: []*models.Commit{
+				makeCommits("a")[0],
+				makeConflictedCommit("b"),
+			},
+			expectedIdx: lo.ToPtr(1),
+		},
+		{
+			name: "finds a different conflicted commit",
+			previousCommits: []*models.Commit{
+				makeConflictedCommit("a"),
+			},
+			commits: []*models.Commit{
+				makeConflictedCommit("b"),
+			},
+			expectedIdx: lo.ToPtr(0),
+		},
+		{
+			name: "ignores the same conflicted commit",
+			previousCommits: []*models.Commit{
+				makeConflictedCommit("a"),
+			},
+			commits: []*models.Commit{
+				makeConflictedCommit("a"),
+			},
+			expectedIdx: nil,
+		},
+		{
+			name:            "reports not found when there is no conflict",
+			previousCommits: makeCommits("a"),
+			commits:         makeCommits("a", "b"),
+			expectedIdx:     nil,
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			idx := findNewConflictedCommit(testCase.previousCommits, testCase.commits)
+
+			assert.Equal(t, testCase.expectedIdx != nil, idx != nil)
+			if idx != nil {
+				assert.Equal(t, *testCase.expectedIdx, *idx)
+			}
+		})
+	}
+}
+
 func TestGetGithubBaseRemote(t *testing.T) {
 	cases := []struct {
 		name             string
@@ -320,3 +376,7 @@ func makeTodoCommit(action todo.TodoCommand) *models.Commit {
 func makeTodoCommitWithHash(hash string, action todo.TodoCommand) *models.Commit {
 	return models.NewCommit(&utils.StringPool{}, models.NewCommitOpts{Hash: hash, Action: action})
 }
+
+func makeConflictedCommit(hash string) *models.Commit {
+	return models.NewCommit(&utils.StringPool{}, models.NewCommitOpts{Hash: hash, Status: models.StatusConflicted})
+}

pkg/integration/tests/branch/rebase_and_drop.go

@@ -54,15 +54,15 @@ var RebaseAndDrop = NewIntegrationTest(NewIntegrationTestArgs{
 			Focus().
 			TopLines(
 				Contains("─── Pending rebase todos"),
-				MatchesRegexp(`pick.*to keep`).IsSelected(),
+				MatchesRegexp(`pick.*to keep`),
 				MatchesRegexp(`pick.*to remove`),
-				MatchesRegexp(`pick.*CONFLICT.*first change`),
+				MatchesRegexp(`pick.*CONFLICT.*first change`).IsSelected(),
 				Contains("─── Commits"),
 				MatchesRegexp("second-change-branch unrelated change"),
 				MatchesRegexp("second change"),
 				MatchesRegexp("original"),
 			).
-			SelectNextItem().
+			NavigateToLine(Contains("to remove")).
 			Press(keys.Universal.Remove).
 			TopLines(
 				Contains("─── Pending rebase todos"),

pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go

@@ -79,10 +79,9 @@ var CherryPickConflicts = NewIntegrationTest(NewIntegrationTestArgs{
 			Focus().
 			TopLines(
 				Contains("second-change-branch unrelated change"),
-				Contains("second change"),
+				Contains("second change").IsSelected(),
-				Contains("first change").IsSelected(),
+				Contains("first change"),
 			).
-			SelectPreviousItem().
 			Tap(func() {
 				// because we picked 'Second change' when resolving the conflict,
 				// we now see this commit as having replaced First Change with Second Change,

pkg/integration/tests/commit/amend_when_there_are_conflicts_and_amend.go

@@ -31,7 +31,7 @@ var AmendWhenThereAreConflictsAndAmend = NewIntegrationTest(NewIntegrationTestAr
 			Lines(
 				Contains("─── Pending rebase todos"),
 				Contains("pick").Contains("commit three"),
-				Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch"),
+				Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch").IsSelected(),
 				Contains("─── Commits"),
 				Contains("commit two"),
 				Contains("file1 changed in master"),

pkg/integration/tests/commit/amend_when_there_are_conflicts_and_cancel.go

@@ -35,7 +35,7 @@ var AmendWhenThereAreConflictsAndCancel = NewIntegrationTest(NewIntegrationTestA
 			Lines(
 				Contains("─── Pending rebase todos"),
 				Contains("pick").Contains("commit three"),
-				Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch"),
+				Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch").IsSelected(),
 				Contains("─── Commits"),
 				Contains("commit two"),
 				Contains("file1 changed in master"),

pkg/integration/tests/commit/revert_with_conflict_multiple_commits.go

@@ -46,7 +46,7 @@ var RevertWithConflictMultipleCommits = NewIntegrationTest(NewIntegrationTestArg
 			Lines(
 				Contains("─── Pending reverts"),
 				Contains("revert").Contains("CI unrelated change"),
-				Contains("revert").Contains("CI <-- CONFLICT --- add first line"),
+				Contains("revert").Contains("CI <-- CONFLICT --- add first line").IsSelected(),
 				Contains("─── Commits"),
 				Contains("CI ○ add second line"),
 				Contains("CI ○ add first line"),

pkg/integration/tests/commit/revert_with_conflict_single_commit.go

@@ -40,7 +40,7 @@ var RevertWithConflictSingleCommit = NewIntegrationTest(NewIntegrationTestArgs{
 			}).
 			Lines(
 				Contains("─── Pending reverts"),
-				Contains("revert").Contains("CI <-- CONFLICT --- add first line"),
+				Contains("revert").Contains("CI <-- CONFLICT --- add first line").IsSelected(),
 				Contains("─── Commits"),
 				Contains("CI ○ add second line"),
 				Contains("CI ○ add first line"),

pkg/integration/tests/commit/shared.go

@@ -45,7 +45,7 @@ func doTheRebaseForAmendTests(t *TestDriver, keys config.KeybindingConfig) {
 		Lines(
 			Contains("─── Pending rebase todos"),
 			Contains("pick").Contains("commit three"),
-			Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch"),
+			Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch").IsSelected(),
 			Contains("─── Commits"),
 			Contains("commit two"),
 			Contains("file1 changed in master"),

pkg/integration/tests/interactive_rebase/amend_commit_with_conflict.go

@@ -36,7 +36,7 @@ var AmendCommitWithConflict = NewIntegrationTest(NewIntegrationTestArgs{
 			Lines(
 				Contains("─── Pending rebase todos"),
 				Contains("pick").Contains("three"),
-				Contains("fixup").Contains("<-- CONFLICT --- fixup! two"),
+				Contains("fixup").Contains("<-- CONFLICT --- fixup! two").IsSelected(),
 				Contains("─── Commits"),
 				Contains("two"),
 				Contains("one"),
@@ -69,7 +69,7 @@ var AmendCommitWithConflict = NewIntegrationTest(NewIntegrationTestArgs{
 		t.Views().Commits().
 			Lines(
 				Contains("─── Pending rebase todos"),
-				Contains("<-- CONFLICT --- three"),
+				Contains("<-- CONFLICT --- three").IsSelected(),
 				Contains("─── Commits"),
 				Contains("two"),
 				Contains("one"),

pkg/integration/tests/interactive_rebase/edit_the_confl_commit.go

@@ -33,7 +33,7 @@ var EditTheConflCommit = NewIntegrationTest(NewIntegrationTestArgs{
 			Focus().
 			Lines(
 				Contains("─── Pending rebase todos"),
-				Contains("pick").Contains("commit two"),
+				Contains("pick").Contains("commit two").IsSelected(),
 				Contains("pick").Contains("<-- CONFLICT --- commit three"),
 				Contains("─── Commits"),
 				Contains("commit one"),

pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go

@@ -55,7 +55,7 @@ var RevertMultipleCommitsInInteractiveRebase = NewIntegrationTest(NewIntegration
 				Contains("CI unrelated change 2"),
 				Contains("─── Pending reverts"),
 				Contains("revert").Contains("CI unrelated change 1"),
-				Contains("revert").Contains("CI <-- CONFLICT --- add first line"),
+				Contains("revert").Contains("CI <-- CONFLICT --- add first line").IsSelected(),
 				Contains("─── Commits"),
 				Contains("CI ○ add second line"),
 				Contains("CI ○ add first line"),

pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go

@@ -49,10 +49,10 @@ var RevertSingleCommitInInteractiveRebase = NewIntegrationTest(NewIntegrationTes
 				Contains("CI unrelated change 2"),
 				Contains("CI unrelated change 1"),
 				Contains("─── Pending reverts"),
-				Contains("revert").Contains("CI <-- CONFLICT --- add first line"),
+				Contains("revert").Contains("CI <-- CONFLICT --- add first line").IsSelected(),
 				Contains("─── Commits"),
 				Contains("CI ○ add second line"),
-				Contains("CI ○ add first line").IsSelected(),
+				Contains("CI ○ add first line"),
 				Contains("CI ○ add empty file"),
 			).
 			Press(keys.Commits.MoveDownCommit).

pkg/integration/tests/interactive_rebase/shared.go

@@ -4,14 +4,25 @@ import (
 	. "github.com/jesseduffield/lazygit/pkg/integration/components"
 )
 
-func handleConflictsFromSwap(t *TestDriver, expectedCommand string) {
+func handleConflictsFromSwap(t *TestDriver, expectedCommand string, selectConflict bool) {
 	t.Common().AcknowledgeConflicts()
 
+	// If the conflict comes from directly moving a commit, we want to keep the moved commit
+	// selected, so selectConflict is false. In other cases (e.g. a conflict after "continue
+	// rebase") we want to select the conflict commit.
+	commitTwoMatcher := Contains("pick").Contains("commit two")
+	conflictMatcher := Contains(expectedCommand).Contains("<-- CONFLICT --- commit three")
+	if selectConflict {
+		conflictMatcher.IsSelected()
+	} else {
+		commitTwoMatcher.IsSelected()
+	}
+
 	t.Views().Commits().
 		Lines(
 			Contains("─── Pending rebase todos"),
-			Contains("pick").Contains("commit two"),
+			commitTwoMatcher,
-			Contains(expectedCommand).Contains("<-- CONFLICT --- commit three"),
+			conflictMatcher,
 			Contains("─── Commits"),
 			Contains("commit one"),
 		)

pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict.go

@@ -48,6 +48,6 @@ var SwapInRebaseWithConflict = NewIntegrationTest(NewIntegrationTestArgs{
 				t.Common().ContinueRebase()
 			})
 
-		handleConflictsFromSwap(t, "pick")
+		handleConflictsFromSwap(t, "pick", true)
 	},
 })

pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict_and_edit.go

@@ -51,6 +51,6 @@ var SwapInRebaseWithConflictAndEdit = NewIntegrationTest(NewIntegrationTestArgs{
 				t.Common().ContinueRebase()
 			})
 
-		handleConflictsFromSwap(t, "edit")
+		handleConflictsFromSwap(t, "edit", true)
 	},
 })

pkg/integration/tests/interactive_rebase/swap_with_conflict.go

@@ -28,6 +28,6 @@ var SwapWithConflict = NewIntegrationTest(NewIntegrationTestArgs{
 			).
 			Press(keys.Commits.MoveDownCommit)
 
-		handleConflictsFromSwap(t, "pick")
+		handleConflictsFromSwap(t, "pick", false)
 	},
 })

pkg/integration/tests/patch_building/move_to_earlier_commit_from_added_file.go

@@ -76,10 +76,11 @@ var MoveToEarlierCommitFromAddedFile = NewIntegrationTest(NewIntegrationTestArgs
 		t.Views().Commits().
 			Focus().
 			Lines(
-				Contains("commit to move from"),
+				Contains("commit to move from").IsSelected(),
-				Contains("destination commit").IsSelected(),
+				Contains("destination commit"),
 				Contains("first commit"),
 			).
+			NavigateToLine(Contains("destination commit")).
 			PressEnter()
 
 		t.Views().CommitFiles().

pkg/integration/tests/sync/pull_rebase_interactive_conflict.go

@@ -50,7 +50,7 @@ var PullRebaseInteractiveConflict = NewIntegrationTest(NewIntegrationTestArgs{
 			Lines(
 				Contains("─── Pending rebase todos"),
 				Contains("pick").Contains("five"),
-				Contains("pick").Contains("CONFLICT").Contains("four"),
+				Contains("pick").Contains("CONFLICT").Contains("four").IsSelected(),
 				Contains("─── Commits"),
 				Contains("three"),
 				Contains("two"),
@@ -83,13 +83,12 @@ var PullRebaseInteractiveConflict = NewIntegrationTest(NewIntegrationTestArgs{
 		t.Views().Commits().
 			Focus().
 			Lines(
-				Contains("five").IsSelected(),
+				Contains("five"),
-				Contains("four"),
+				Contains("four").IsSelected(),
 				Contains("three"),
 				Contains("two"),
 				Contains("one"),
-			).
+			)
-			SelectNextItem()
 
 		t.Views().Main().
 			Content(

pkg/integration/tests/sync/pull_rebase_interactive_conflict_drop.go

@@ -50,13 +50,14 @@ var PullRebaseInteractiveConflictDrop = NewIntegrationTest(NewIntegrationTestArg
 			Focus().
 			Lines(
 				Contains("─── Pending rebase todos"),
-				Contains("pick").Contains("five").IsSelected(),
+				Contains("pick").Contains("five"),
-				Contains("pick").Contains("CONFLICT").Contains("four"),
+				Contains("pick").Contains("CONFLICT").Contains("four").IsSelected(),
 				Contains("─── Commits"),
 				Contains("three"),
 				Contains("two"),
 				Contains("one"),
 			).
+			NavigateToLine(Contains("five")).
 			Press(keys.Universal.Remove).
 			Lines(
 				Contains("─── Pending rebase todos"),