diff --git a/pkg/gui/controllers/branches_controller.go b/pkg/gui/controllers/branches_controller.go index 24ef84d54..c2c0595c3 100644 --- a/pkg/gui/controllers/branches_controller.go +++ b/pkg/gui/controllers/branches_controller.go @@ -594,7 +594,11 @@ func (self *BranchesController) createNewBranchWithName(newBranchName string) er } self.c.Helpers().Refs.SelectFirstBranchAndFirstCommit() - self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC, KeepBranchSelectionIndex: true}) + self.c.Refresh(types.RefreshOptions{ + Mode: types.ASYNC, + KeepBranchSelectionIndex: true, + CommitSelection: types.KeepCommitSelectionIndex, + }) return nil } diff --git a/pkg/gui/controllers/helpers/cherry_pick_helper.go b/pkg/gui/controllers/helpers/cherry_pick_helper.go index 079fdedcf..e2fe46545 100644 --- a/pkg/gui/controllers/helpers/cherry_pick_helper.go +++ b/pkg/gui/controllers/helpers/cherry_pick_helper.go @@ -100,16 +100,6 @@ func (self *CherryPickHelper) Paste() error { return result } - // Move the selection down by the number of commits we just - // cherry-picked, to keep the same commit selected as before. - // Don't do this if a rebase todo is selected, because in this - // case we are in a rebase and the cherry-picked commits end up - // below the selection. - if commit := self.c.Contexts().LocalCommits.GetSelected(); commit != nil && !commit.IsTODO() { - self.c.Contexts().LocalCommits.MoveSelection(len(cherryPickedCommits)) - self.c.Contexts().LocalCommits.FocusLine(true) - } - // If we're in the cherry-picking state at this point, it must // be because there were conflicts. Don't clear the copied // commits in this case, since we might want to abort and try diff --git a/pkg/gui/controllers/helpers/gpg_helper.go b/pkg/gui/controllers/helpers/gpg_helper.go index 46fbee703..fb8fae628 100644 --- a/pkg/gui/controllers/helpers/gpg_helper.go +++ b/pkg/gui/controllers/helpers/gpg_helper.go @@ -31,6 +31,21 @@ func (self *GpgHelper) WithGpgHandling( cmdObj, configKey, waitingStatus, onSuccess, refreshOptions, refreshOptions) } +// WithGpgHandlingAndSelectHeadCommit is like WithGpgHandling, but on success it +// selects the new HEAD commit rather than restoring the previous selection. For +// committing, where the commit we just created is the one we want selected. +func (self *GpgHelper) WithGpgHandlingAndSelectHeadCommit( + cmdObj *oscommands.CmdObj, + configKey git_commands.GpgConfigKey, + waitingStatus string, + onSuccess func() error, +) error { + failureRefreshOptions := types.RefreshOptions{Mode: types.ASYNC} + successRefreshOptions := types.RefreshOptions{Mode: types.ASYNC, CommitSelection: types.SelectHeadCommit} + return self.withGpgHandling( + cmdObj, configKey, waitingStatus, onSuccess, failureRefreshOptions, successRefreshOptions) +} + // Currently there is a bug where if we switch to a subprocess from within // WithWaitingStatus we get stuck there and can't return to lazygit. We could // fix this bug, or just stop running subprocesses from within there, given that diff --git a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go index ab7d11c29..536c254dd 100644 --- a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go +++ b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go @@ -95,6 +95,8 @@ func (self *MergeAndRebaseHelper) genericMergeCommand(command string) error { } commandType := status.CommandName() + selectHeadCommitOnSuccess := command == REBASE_OPTION_CONTINUE && + effectiveStatus == models.WORKING_TREE_STATE_MERGING // we should end up with a command like 'git merge --continue' @@ -106,12 +108,29 @@ func (self *MergeAndRebaseHelper) genericMergeCommand(command string) error { if needsSubprocess { // TODO: see if we should be calling more of the code from self.Git.Rebase.GenericMergeOrRebaseAction - return self.c.RunSubprocessAndRefresh( - self.c.Git().Rebase.GenericMergeOrRebaseActionCmdObj(commandType, command), - ) + success, err := self.c.RunSubprocess(self.c.Git().Rebase.GenericMergeOrRebaseActionCmdObj(commandType, command)) + self.c.Refresh(types.RefreshOptions{ + Mode: types.ASYNC, + CommitSelection: commitSelectionAfterMerge(success && selectHeadCommitOnSuccess), + }) + return err } result := self.c.Git().Rebase.GenericMergeOrRebaseAction(commandType, command) - return self.CheckMergeOrRebase(result) + return self.CheckMergeOrRebaseWithRefreshOptions(result, + types.RefreshOptions{ + Mode: types.ASYNC, + CommitSelection: commitSelectionAfterMerge(result == nil && selectHeadCommitOnSuccess), + }) +} + +// commitSelectionAfterMerge maps whether a merge/rebase/pull created a new +// commit at HEAD to the corresponding commit-selection behavior: select that +// new commit, or otherwise keep the previous selection by hash. +func commitSelectionAfterMerge(createdNewCommit bool) types.CommitSelectionBehavior { + if createdNewCommit { + return types.SelectHeadCommit + } + return types.KeepCommitSelectionByHash } func (self *MergeAndRebaseHelper) hasExecTodos() bool { @@ -166,6 +185,15 @@ func (self *MergeAndRebaseHelper) CheckMergeOrRebase(result error) error { return self.CheckMergeOrRebaseWithRefreshOptions(result, types.RefreshOptions{Mode: types.ASYNC}) } +// Like CheckMergeOrRebase, but for operations that create a new commit at HEAD +// (a merge, or a pull that merges): on success it selects that new commit, +// which the keep-selection-by-hash logic can't do since the commit didn't exist +// before the refresh. +func (self *MergeAndRebaseHelper) CheckMergeOrRebaseAndSelectHeadCommit(result error) error { + return self.CheckMergeOrRebaseWithRefreshOptions(result, + types.RefreshOptions{Mode: types.ASYNC, CommitSelection: commitSelectionAfterMerge(result == nil)}) +} + func (self *MergeAndRebaseHelper) CheckForConflicts(result error) error { if result == nil { return nil @@ -489,7 +517,7 @@ func (self *MergeAndRebaseHelper) RegularMerge(refName string, variant git_comma return func() error { self.c.LogAction(self.c.Tr.Actions.Merge) err := self.c.Git().Branch.Merge(refName, variant) - return self.CheckMergeOrRebase(err) + return self.CheckMergeOrRebaseAndSelectHeadCommit(err) } } diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 31035d104..3ea742c8e 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -12,6 +12,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/config" "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/gui/context" + "github.com/jesseduffield/lazygit/pkg/gui/context/traits" "github.com/jesseduffield/lazygit/pkg/gui/filetree" "github.com/jesseduffield/lazygit/pkg/gui/mergeconflicts" "github.com/jesseduffield/lazygit/pkg/gui/presentation" @@ -164,7 +165,9 @@ func (self *RefreshHelper) Refresh(options types.RefreshOptions) { // whenever we change commits, we should update branches because the upstream/downstream // counts can change. Whenever we change branches we should also change commits // e.g. in the case of switching branches. - refresh("commits and commit files", self.refreshCommitsAndCommitFiles) + refresh("commits and commit files", func() { + self.refreshCommitsAndCommitFiles(options.CommitSelection) + }) includeWorktreesWithBranches = scopeSet.Includes(types.WORKTREES) if self.c.UserConfig().Git.LocalBranchSortOrder == "recency" { @@ -385,8 +388,8 @@ func (self *RefreshHelper) refreshReflogAndBranches(refreshWorktrees bool, keepB self.refreshBranches(refreshWorktrees, keepBranchSelectionIndex, loadBehindCounts) } -func (self *RefreshHelper) refreshCommitsAndCommitFiles() { - _ = self.refreshCommitsWithLimit() +func (self *RefreshHelper) refreshCommitsAndCommitFiles(commitSelection types.CommitSelectionBehavior) { + _ = self.refreshCommitsWithLimit(commitSelection) ctx := self.c.Contexts().CommitFiles.GetParentContext() if ctx != nil && ctx.GetKey() == context.LOCAL_COMMITS_CONTEXT_KEY { // This makes sense when we've e.g. just amended a commit, meaning we get a new commit hash at the same position. @@ -430,10 +433,16 @@ func (self *RefreshHelper) determineCheckedOutRef() models.Ref { return nil } -func (self *RefreshHelper) refreshCommitsWithLimit() error { +func (self *RefreshHelper) refreshCommitsWithLimit(commitSelection types.CommitSelectionBehavior) error { self.c.Mutexes().LocalCommitsMutex.Lock() defer self.c.Mutexes().LocalCommitsMutex.Unlock() + var selectionRange *localCommitSelectionRange + if commitSelection == types.KeepCommitSelectionByHash { + selectedIdx, rangeStartIdx, rangeSelectMode := self.c.Contexts().LocalCommits.GetSelectionRangeAndMode() + selectionRange = captureLocalCommitSelectionRange(self.c.Model().Commits, selectedIdx, rangeStartIdx, rangeSelectMode) + } + checkedOutRef := self.determineCheckedOutRef() commits, err := self.c.Git().Loaders.CommitLoader.GetCommits( git_commands.GetCommitsOptions{ @@ -460,10 +469,88 @@ func (self *RefreshHelper) refreshCommitsWithLimit() error { self.c.Model().CheckedOutBranch = "" } + scrollSelectionIntoView := false + switch commitSelection { + case types.SelectHeadCommit: + if headCommitIdx := models.HeadCommitIdx(commits); headCommitIdx >= 0 { + self.c.Contexts().LocalCommits.SetSelection(headCommitIdx) + scrollSelectionIntoView = true + } + case types.KeepCommitSelectionByHash: + if selectionRange != nil { + selectedIdx, rangeStartIdx, didMove, found := findLocalCommitSelectionRange(commits, selectionRange) + if found { + self.c.Contexts().LocalCommits.SetSelectionRangeAndMode(selectedIdx, rangeStartIdx, selectionRange.mode) + scrollSelectionIntoView = didMove + } + } + case types.KeepCommitSelectionIndex: + // The caller set the selection index deliberately; leave it untouched. + } + self.refreshView(self.c.Contexts().LocalCommits) + if scrollSelectionIntoView { + self.c.OnUIThread(func() error { + self.c.Contexts().LocalCommits.FocusLine(true) + return nil + }) + } return nil } +type localCommitSelectionRange struct { + selectedHash string + selectedIsTODO bool + rangeStartHash string + rangeStartIsTODO bool + selectedIdx int + rangeStartIdx int + mode traits.RangeSelectMode +} + +func captureLocalCommitSelectionRange( + commits []*models.Commit, + selectedIdx int, + rangeStartIdx int, + mode traits.RangeSelectMode, +) *localCommitSelectionRange { + if !hasRestorableCommitHash(commits, selectedIdx) || !hasRestorableCommitHash(commits, rangeStartIdx) { + return nil + } + + return &localCommitSelectionRange{ + selectedHash: commits[selectedIdx].Hash(), + selectedIsTODO: commits[selectedIdx].IsTODO(), + rangeStartHash: commits[rangeStartIdx].Hash(), + rangeStartIsTODO: commits[rangeStartIdx].IsTODO(), + selectedIdx: selectedIdx, + rangeStartIdx: rangeStartIdx, + mode: mode, + } +} + +func findLocalCommitSelectionRange( + commits []*models.Commit, + selectionRange *localCommitSelectionRange, +) (int, int, bool, bool) { + _, selectedIdx, foundSelected := lo.FindIndexOf(commits, func(commit *models.Commit) bool { + return commit.Hash() == selectionRange.selectedHash && commit.IsTODO() == selectionRange.selectedIsTODO + }) + _, rangeStartIdx, foundRangeStart := lo.FindIndexOf(commits, func(commit *models.Commit) bool { + return commit.Hash() == selectionRange.rangeStartHash && commit.IsTODO() == selectionRange.rangeStartIsTODO + }) + if !foundSelected || !foundRangeStart { + return 0, 0, false, false + } + + didMove := selectedIdx != selectionRange.selectedIdx || rangeStartIdx != selectionRange.rangeStartIdx + return selectedIdx, rangeStartIdx, didMove, true +} + +func hasRestorableCommitHash(commits []*models.Commit, idx int) bool { + return idx >= 0 && idx < len(commits) && commits[idx].Hash() != "" +} + func (self *RefreshHelper) refreshSubCommitsWithLimit() error { if self.c.Contexts().SubCommits.GetRef() == nil { return nil diff --git a/pkg/gui/controllers/helpers/refresh_helper_test.go b/pkg/gui/controllers/helpers/refresh_helper_test.go index cebd044c4..e8be06d61 100644 --- a/pkg/gui/controllers/helpers/refresh_helper_test.go +++ b/pkg/gui/controllers/helpers/refresh_helper_test.go @@ -5,10 +5,148 @@ import ( "github.com/jesseduffield/lazygit/pkg/commands/hosting_service" "github.com/jesseduffield/lazygit/pkg/commands/models" + "github.com/jesseduffield/lazygit/pkg/gui/context/traits" + "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" + "github.com/stefanhaller/git-todo-parser/todo" "github.com/stretchr/testify/assert" ) +func TestCaptureLocalCommitSelectionRange(t *testing.T) { + testCases := []struct { + name string + commits []*models.Commit + selectedIdx int + rangeStartIdx int + expected *localCommitSelectionRange + }{ + { + name: "captures selected commit and range start", + commits: makeCommits("a", "b"), + selectedIdx: 1, + rangeStartIdx: 0, + expected: &localCommitSelectionRange{ + selectedHash: "b", + rangeStartHash: "a", + selectedIdx: 1, + rangeStartIdx: 0, + mode: traits.RangeSelectModeSticky, + }, + }, + { + name: "ignores invalid range start index", + commits: makeCommits("a"), + selectedIdx: 0, + rangeStartIdx: 1, + expected: nil, + }, + { + name: "ignores empty selected hash", + commits: append(makeCommits("a"), makeTodoCommit(todo.UpdateRef)), + selectedIdx: 1, + rangeStartIdx: 0, + expected: nil, + }, + { + name: "ignores empty range start hash", + commits: append(makeCommits("a"), makeTodoCommit(todo.Exec)), + selectedIdx: 0, + rangeStartIdx: 1, + expected: nil, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + selectionRange := captureLocalCommitSelectionRange( + testCase.commits, + testCase.selectedIdx, + testCase.rangeStartIdx, + traits.RangeSelectModeSticky, + ) + + assert.Equal(t, testCase.expected, selectionRange) + }) + } +} + +func TestFindLocalCommitSelectionRange(t *testing.T) { + type expectation struct { + selectedIdx int + rangeStartIdx int + moved bool + found bool + } + + selectionRange := localCommitSelectionRange{ + selectedHash: "b", + rangeStartHash: "c", + selectedIdx: 1, + rangeStartIdx: 2, + mode: traits.RangeSelectModeSticky, + } + + testCases := []struct { + name string + commits []*models.Commit + expected expectation + }{ + { + name: "finds selection after commits are inserted above it", + commits: makeCommits("new", "a", "b", "c"), + expected: expectation{ + selectedIdx: 2, + rangeStartIdx: 3, + moved: true, + found: true, + }, + }, + { + name: "finds selection that did not move", + commits: makeCommits("a", "b", "c"), + expected: expectation{ + selectedIdx: 1, + rangeStartIdx: 2, + found: true, + }, + }, + { + name: "reports not found when a hash is missing", + commits: makeCommits("a", "b"), + expected: expectation{}, + }, + { + name: "skips todo entries with the same hash as a selected commit", + commits: []*models.Commit{ + makeTodoCommitWithHash("b", todo.Revert), + makeCommits("a")[0], + makeCommits("b")[0], + makeCommits("c")[0], + }, + expected: expectation{ + selectedIdx: 2, + rangeStartIdx: 3, + moved: true, + found: true, + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + selectedIdx, rangeStartIdx, moved, found := findLocalCommitSelectionRange(testCase.commits, &selectionRange) + actual := expectation{ + selectedIdx: selectedIdx, + rangeStartIdx: rangeStartIdx, + moved: moved, + found: found, + } + + assert.Equal(t, testCase.expected, actual) + }) + } +} + func TestGetGithubBaseRemote(t *testing.T) { cases := []struct { name string @@ -122,3 +260,18 @@ func makeAuthenticatedGithubRemoteInfo(name string, webDomain string, authToken info.authToken = authToken return info } + +func makeCommits(hashes ...string) []*models.Commit { + hashPool := &utils.StringPool{} + return lo.Map(hashes, func(hash string, _ int) *models.Commit { + return models.NewCommit(hashPool, models.NewCommitOpts{Hash: hash}) + }) +} + +func makeTodoCommit(action todo.TodoCommand) *models.Commit { + return models.NewCommit(&utils.StringPool{}, models.NewCommitOpts{Action: action}) +} + +func makeTodoCommitWithHash(hash string, action todo.TodoCommand) *models.Commit { + return models.NewCommit(&utils.StringPool{}, models.NewCommitOpts{Hash: hash, Action: action}) +} diff --git a/pkg/gui/controllers/helpers/refs_helper.go b/pkg/gui/controllers/helpers/refs_helper.go index a3db043ef..99e9f47ec 100644 --- a/pkg/gui/controllers/helpers/refs_helper.go +++ b/pkg/gui/controllers/helpers/refs_helper.go @@ -66,7 +66,12 @@ func (self *RefsHelper) CheckoutRef(ref string, options types.CheckoutRefOptions if options.RefreshPullRequests { scope = append(scope, types.PULL_REQUESTS) } - self.c.Refresh(types.RefreshOptions{Mode: types.BLOCK_UI, Scope: scope, KeepBranchSelectionIndex: true}) + self.c.Refresh(types.RefreshOptions{ + Mode: types.BLOCK_UI, + Scope: scope, + KeepBranchSelectionIndex: true, + CommitSelection: types.KeepCommitSelectionIndex, + }) } localBranch, found := lo.Find(self.c.Model().Branches, func(branch *models.Branch) bool { @@ -209,7 +214,7 @@ func (self *RefsHelper) ResetToRef(ref string, strength string, envVars []string // loading a heap of commits is slow so we limit them whenever doing a reset self.c.Contexts().LocalCommits.SetLimitCommits(true) - self.c.Refresh(types.RefreshOptions{Scope: []types.RefreshableView{types.FILES, types.BRANCHES, types.REFLOG, types.COMMITS}}) + self.c.Refresh(types.RefreshOptions{Scope: []types.RefreshableView{types.FILES, types.BRANCHES, types.REFLOG, types.COMMITS}, CommitSelection: types.KeepCommitSelectionIndex}) return nil } @@ -370,7 +375,11 @@ func (self *RefsHelper) NewBranch(from string, fromFormattedName string, suggest self.SelectFirstBranchAndFirstCommit() - self.c.Refresh(types.RefreshOptions{Mode: types.BLOCK_UI, KeepBranchSelectionIndex: true}) + self.c.Refresh(types.RefreshOptions{ + Mode: types.BLOCK_UI, + KeepBranchSelectionIndex: true, + CommitSelection: types.KeepCommitSelectionIndex, + }) } self.c.Prompt(types.PromptOpts{ @@ -525,7 +534,11 @@ func (self *RefsHelper) moveCommitsToNewBranchStackedOnCurrentBranch(newBranchNa self.SelectFirstBranchAndFirstCommit() - self.c.Refresh(types.RefreshOptions{Mode: types.BLOCK_UI, KeepBranchSelectionIndex: true}) + self.c.Refresh(types.RefreshOptions{ + Mode: types.BLOCK_UI, + KeepBranchSelectionIndex: true, + CommitSelection: types.KeepCommitSelectionIndex, + }) return nil } @@ -563,7 +576,11 @@ func (self *RefsHelper) moveCommitsToNewBranchOffOfMainBranch(newBranchName stri self.SelectFirstBranchAndFirstCommit() - self.c.Refresh(types.RefreshOptions{Mode: types.BLOCK_UI, KeepBranchSelectionIndex: true}) + self.c.Refresh(types.RefreshOptions{ + Mode: types.BLOCK_UI, + KeepBranchSelectionIndex: true, + CommitSelection: types.KeepCommitSelectionIndex, + }) return nil } diff --git a/pkg/gui/controllers/helpers/working_tree_helper.go b/pkg/gui/controllers/helpers/working_tree_helper.go index 3ad2c54cf..7e070ba31 100644 --- a/pkg/gui/controllers/helpers/working_tree_helper.go +++ b/pkg/gui/controllers/helpers/working_tree_helper.go @@ -147,11 +147,11 @@ func (self *WorkingTreeHelper) HandleCommitPressWithMessage(initialMessage strin func (self *WorkingTreeHelper) handleCommit(summary string, description string, forceSkipHooks bool) error { cmdObj := self.c.Git().Commit.CommitCmdObj(summary, description, forceSkipHooks) self.c.LogAction(self.c.Tr.Actions.Commit) - return self.gpgHelper.WithGpgHandling(cmdObj, git_commands.CommitGpgSign, self.c.Tr.CommittingStatus, + return self.gpgHelper.WithGpgHandlingAndSelectHeadCommit(cmdObj, git_commands.CommitGpgSign, self.c.Tr.CommittingStatus, func() error { self.commitsHelper.ClearPreservedCommitMessage() return nil - }, nil) + }) } func (self *WorkingTreeHelper) switchFromCommitMessagePanelToEditor(filepath string, forceSkipHooks bool) error { diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 4ab436bbc..083da4f4d 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -767,7 +767,9 @@ func (self *LocalCommitsController) moveDown(selectedCommits []*models.Commit, s self.context().HandleFocus(types.OnFocusOpts{ScrollSelectionIntoView: true}) self.c.Refresh(types.RefreshOptions{ - Mode: types.SYNC, Scope: []types.RefreshableView{types.REBASE_COMMITS}, + Mode: types.SYNC, + Scope: []types.RefreshableView{types.REBASE_COMMITS}, + CommitSelection: types.KeepCommitSelectionIndex, }) return nil } @@ -780,7 +782,7 @@ func (self *LocalCommitsController) moveDown(selectedCommits []*models.Commit, s self.context().HandleFocus(types.OnFocusOpts{ScrollSelectionIntoView: true}) } return self.c.Helpers().MergeAndRebase.CheckMergeOrRebaseWithRefreshOptions( - err, types.RefreshOptions{Mode: types.SYNC}) + err, types.RefreshOptions{Mode: types.SYNC, CommitSelection: types.KeepCommitSelectionIndex}) }) } @@ -793,7 +795,9 @@ func (self *LocalCommitsController) moveUp(selectedCommits []*models.Commit, sta self.context().HandleFocus(types.OnFocusOpts{ScrollSelectionIntoView: true}) self.c.Refresh(types.RefreshOptions{ - Mode: types.SYNC, Scope: []types.RefreshableView{types.REBASE_COMMITS}, + Mode: types.SYNC, + Scope: []types.RefreshableView{types.REBASE_COMMITS}, + CommitSelection: types.KeepCommitSelectionIndex, }) return nil } @@ -806,7 +810,7 @@ func (self *LocalCommitsController) moveUp(selectedCommits []*models.Commit, sta self.context().HandleFocus(types.OnFocusOpts{ScrollSelectionIntoView: true}) } return self.c.Helpers().MergeAndRebase.CheckMergeOrRebaseWithRefreshOptions( - err, types.RefreshOptions{Mode: types.SYNC}) + err, types.RefreshOptions{Mode: types.SYNC, CommitSelection: types.KeepCommitSelectionIndex}) }) } @@ -966,8 +970,6 @@ func (self *LocalCommitsController) revert(commits []*models.Commit, start, end if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebaseWithRefreshOptions(result, types.RefreshOptions{Mode: types.SYNC}); err != nil { return err } - self.context().MoveSelection(len(commits)) - self.context().HandleFocus(types.OnFocusOpts{ScrollSelectionIntoView: true}) if mustStash { if err := self.c.Git().Stash.Pop(0); err != nil { @@ -1013,7 +1015,6 @@ func (self *LocalCommitsController) createFixupCommit(commit *models.Commit) err return err } - self.context().MoveSelectedLine(1) self.c.Refresh(types.RefreshOptions{Mode: types.SYNC}) return nil }) @@ -1114,7 +1115,6 @@ func (self *LocalCommitsController) createAmendCommit(commit *models.Commit, inc return err } - self.context().MoveSelectedLine(1) self.c.Refresh(types.RefreshOptions{Mode: types.SYNC}) return nil }) diff --git a/pkg/gui/controllers/remotes_controller.go b/pkg/gui/controllers/remotes_controller.go index b2e30f231..8f5dd1ae6 100644 --- a/pkg/gui/controllers/remotes_controller.go +++ b/pkg/gui/controllers/remotes_controller.go @@ -374,6 +374,7 @@ func (self *RemotesController) fetchAndCheckout(remote *models.Remote, branchNam self.c.Context().Push(self.c.Contexts().Branches, types.OnFocusOpts{}) self.c.Helpers().Refs.SelectFirstBranchAndFirstCommit() refreshOptions.KeepBranchSelectionIndex = true + refreshOptions.CommitSelection = types.KeepCommitSelectionIndex } } self.c.Refresh(refreshOptions) diff --git a/pkg/gui/controllers/sync_controller.go b/pkg/gui/controllers/sync_controller.go index 649b53338..f1b794e97 100644 --- a/pkg/gui/controllers/sync_controller.go +++ b/pkg/gui/controllers/sync_controller.go @@ -175,7 +175,7 @@ func (self *SyncController) pullWithLock(task gocui.Task, opts PullFilesOptions) }, ) - return self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err) + return self.c.Helpers().MergeAndRebase.CheckMergeOrRebaseAndSelectHeadCommit(err) } type pushOpts struct { diff --git a/pkg/gui/types/refresh.go b/pkg/gui/types/refresh.go index c9e156180..17d917bf6 100644 --- a/pkg/gui/types/refresh.go +++ b/pkg/gui/types/refresh.go @@ -33,6 +33,28 @@ const ( BLOCK_UI // wrap code in an update call to ensure UI updates all at once and keybindings aren't executed till complete ) +// CommitSelectionBehavior controls which local commit is selected after the +// commits list is reloaded by a refresh. +type CommitSelectionBehavior int + +const ( + // Keep the same commit selected by hash (and the same range, when + // range-selecting), restoring it at its new position if it moved. This is + // the right default whenever the list reloads underneath a selection the + // user hasn't deliberately changed. + KeepCommitSelectionByHash CommitSelectionBehavior = iota + + // Leave the selection index untouched, because the caller set it itself + // before refreshing. Used when jumping to the top of the list after a + // checkout, and when following a commit that was just moved up or down. + KeepCommitSelectionIndex + + // Select the HEAD commit. Used by operations that create a new commit at + // HEAD (committing, merging, pulling with a merge); the by-hash behavior + // can't restore a commit that didn't exist before the refresh. + SelectHeadCommit +) + type RefreshOptions struct { Then func() Scope []RefreshableView // e.g. []RefreshableView{COMMITS, BRANCHES}. Leave empty to refresh everything @@ -45,6 +67,10 @@ type RefreshOptions struct { // head, and selecting index 0. KeepBranchSelectionIndex bool + // Controls which local commit is selected after the refresh. Defaults to + // KeepCommitSelectionByHash. + CommitSelection CommitSelectionBehavior + // When true, this refresh was initiated by a background routine rather than // by a user action. We use it to keep background `git status` calls from // taking optional git locks, so they don't contend for index.lock with git diff --git a/pkg/integration/tests/cherry_pick/cherry_pick_commit_that_becomes_empty.go b/pkg/integration/tests/cherry_pick/cherry_pick_commit_that_becomes_empty.go index fbd8ee9a6..081a71f66 100644 --- a/pkg/integration/tests/cherry_pick/cherry_pick_commit_that_becomes_empty.go +++ b/pkg/integration/tests/cherry_pick/cherry_pick_commit_that_becomes_empty.go @@ -73,25 +73,11 @@ var CherryPickCommitThatBecomesEmpty = NewIntegrationTest(NewIntegrationTestArgs // Cherry-picked commit is empty t.Views().Main().Content(DoesNotContain("diff --git")) } else { + // Older git versions drop the commit that became empty t.Views().Commits(). - // We have a bug with how the selection is updated in this case; normally you would - // expect the "two changes in one commit" commit to be selected because it was - // selected before pasting, and we try to maintain that selection. This is broken - // for two reasons: - // 1. We increment the selected line index after pasting by the number of pasted - // commits; this is wrong because we skipped the commit that became empty. So - // according to this bug, the "base" commit should be selected. - // 2. We only update the selected line index after pasting if the currently selected - // commit is not a rebase TODO commit, on the assumption that if it is, we are in a - // rebase and the cherry-picked commits end up below the selection. In this case, - // however, we still think we are cherry-picking because the final refresh after the - // CheckMergeOrRebase in CherryPickHelper.Paste is async and hasn't completed yet; - // so the "unrelated change" still has a "pick" action. - // - // Since this only happens for older git versions, we don't bother fixing it. Lines( - Contains("unrelated change").IsSelected(), - Contains("two changes in one commit"), + Contains("unrelated change"), + Contains("two changes in one commit").IsSelected(), Contains("base"), ) } diff --git a/pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go b/pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go index b135bfd7f..7468f921c 100644 --- a/pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go +++ b/pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go @@ -78,11 +78,11 @@ var CherryPickConflicts = NewIntegrationTest(NewIntegrationTestArgs{ t.Views().Commits(). Focus(). TopLines( - Contains("second-change-branch unrelated change").IsSelected(), + Contains("second-change-branch unrelated change"), Contains("second change"), - Contains("first change"), + Contains("first change").IsSelected(), ). - SelectNextItem(). + 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, diff --git a/pkg/integration/tests/cherry_pick/cherry_pick_conflicts_empty_commit_after_resolving.go b/pkg/integration/tests/cherry_pick/cherry_pick_conflicts_empty_commit_after_resolving.go index ff9efda3c..7af67791f 100644 --- a/pkg/integration/tests/cherry_pick/cherry_pick_conflicts_empty_commit_after_resolving.go +++ b/pkg/integration/tests/cherry_pick/cherry_pick_conflicts_empty_commit_after_resolving.go @@ -69,23 +69,8 @@ var CherryPickConflictsEmptyCommitAfterResolving = NewIntegrationTest(NewIntegra t.Views().Commits(). Focus(). TopLines( - // We have a bug with how the selection is updated in this case; normally you would - // expect the "first change" commit to be selected because it was selected before - // pasting, and we try to maintain that selection. This is broken for two reasons: - // 1. We increment the selected line index after pasting by the number of pasted - // commits; this is wrong because we skipped the commit that became empty. So - // according to this bug, the "original" commit should be selected. - // 2. We only update the selected line index after pasting if the currently selected - // commit is not a rebase TODO commit, on the assumption that if it is, we are in a - // rebase and the cherry-picked commits end up below the selection. In this case, - // however, we still think we are cherry-picking because the final refresh after the - // CheckMergeOrRebase in CherryPickHelper.Paste is async and hasn't completed yet; - // so the "second-change-branch unrelated change" still has a "pick" action. - // - // We don't bother fixing it for now because it's a pretty niche case, and the - // nature of the problem is only cosmetic. - Contains("second-change-branch unrelated change").IsSelected(), - Contains("first change"), + Contains("second-change-branch unrelated change"), + Contains("first change").IsSelected(), Contains("original"), ) }, diff --git a/pkg/integration/tests/commit/keep_selected_commit_after_external_commit.go b/pkg/integration/tests/commit/keep_selected_commit_after_external_commit.go new file mode 100644 index 000000000..82cc66ab2 --- /dev/null +++ b/pkg/integration/tests/commit/keep_selected_commit_after_external_commit.go @@ -0,0 +1,46 @@ +package commit + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var KeepSelectedCommitAfterExternalCommit = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Keep the same commit selected after an external commit is created", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("file", "first content") + shell.Commit("first commit") + shell.UpdateFile("file", "second content") + shell.GitAddAll() + shell.Commit("second commit") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("second commit"), + Contains("first commit"), + ). + NavigateToLine(Contains("first commit")) + + t.Views().Main().Content(Contains("+first content")) + + t.GlobalPress(keys.Universal.ExecuteShellCommand) + t.ExpectPopup().Prompt(). + Title(Equals("Shell command:")). + Type("git commit --allow-empty -m 'external commit'"). + Confirm() + + t.Views().Commits(). + Lines( + Contains("external commit"), + Contains("second commit"), + Contains("first commit").IsSelected(), + ) + + t.Views().Main().Content(Contains("+first content")) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 1b264e50d..fa7b7e26b 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -139,6 +139,7 @@ var tests = []*components.IntegrationTest{ commit.Highlight, commit.History, commit.HistoryComplex, + commit.KeepSelectedCommitAfterExternalCommit, commit.NewBranch, commit.PasteCommitMessage, commit.PasteCommitMessageOverExisting,