From d84ee606e879c9704fd60e28db18fa1cec703f6f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 13 Jun 2024 15:16:40 +0200 Subject: [PATCH 1/9] Remove unused enum entry StatusSelected --- pkg/commands/models/commit.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/commands/models/commit.go b/pkg/commands/models/commit.go index 50bcfab8fb0..752177d9896 100644 --- a/pkg/commands/models/commit.go +++ b/pkg/commands/models/commit.go @@ -18,7 +18,6 @@ const ( StatusPushed StatusMerged StatusRebasing - StatusSelected StatusReflog ) From 9c8f987934eaf5480cd8516a5d47dcbf6d509911 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 5 Apr 2025 10:48:32 +0200 Subject: [PATCH 2/9] Use commit.IsTODO instead of comparing Status against models.StatusRebasing This is equivalent in the current state of the code, but it will no longer be after the next commit, because we will introduce a new status value StatusConflicted. And in a later PR we might add yet another value StatusCherryPicking to distinguish rebase todos from cherry-pick todos; using commit.IsTODO is a safer way to check whether a commit is any of these. --- pkg/commands/git_commands/commit_loader.go | 2 +- pkg/gui/controllers/files_controller.go | 2 +- pkg/gui/controllers/helpers/merge_and_rebase_helper.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 22050b38472..556863de1f0 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -126,7 +126,7 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit, if commit.Hash == firstPushedCommit { passedFirstPushedCommit = true } - if commit.Status != models.StatusRebasing { + if !commit.IsTODO() { if passedFirstPushedCommit { commit.Status = models.StatusPushed } else { diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index 167e29f1b7e..45cb80d2f03 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -829,7 +829,7 @@ func (self *FilesController) handleAmendCommitPress() error { func (self *FilesController) isResolvingConflicts() bool { commits := self.c.Model().Commits for _, c := range commits { - if c.Status != models.StatusRebasing { + if !c.IsTODO() { break } if c.Action == models.ActionConflict { diff --git a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go index 3f2ced17e5c..8505110a0a0 100644 --- a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go +++ b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go @@ -122,7 +122,7 @@ func (self *MergeAndRebaseHelper) genericMergeCommand(command string) error { func (self *MergeAndRebaseHelper) hasExecTodos() bool { for _, commit := range self.c.Model().Commits { - if commit.Status != models.StatusRebasing { + if !commit.IsTODO() { break } if commit.Action == todo.Exec { From ff465e2581ac963d9052dd00b2873d92e7f7d9f6 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 13 Jun 2024 16:55:20 +0200 Subject: [PATCH 3/9] Show original todo action instead of "conflict", and show `<-- CONFLICT` instead It is useful to see if the conflicted commit was a "pick" or an "edit". What's more, we're about to add support for showing cherry-picks and reverts, and seeing that a conflicted commit was a revert is important because its diff is backwards compared to the diff of the conflicting files in the Files panel. --- pkg/commands/git_commands/commit_loader.go | 35 +++++++++--------- .../git_commands/commit_loader_test.go | 36 ++++++++++++------- pkg/commands/models/commit.go | 3 +- pkg/gui/controllers/files_controller.go | 6 ++-- .../controllers/local_commits_controller.go | 4 +-- pkg/gui/presentation/commits.go | 24 ++++++++----- pkg/i18n/english.go | 2 ++ .../tests/branch/rebase_and_drop.go | 4 +-- ...mend_when_there_are_conflicts_and_amend.go | 2 +- ...end_when_there_are_conflicts_and_cancel.go | 2 +- pkg/integration/tests/commit/shared.go | 2 +- .../amend_commit_with_conflict.go | 4 +-- .../edit_the_confl_commit.go | 4 +-- .../tests/interactive_rebase/shared.go | 4 +-- .../swap_in_rebase_with_conflict.go | 2 +- .../swap_in_rebase_with_conflict_and_edit.go | 2 +- .../interactive_rebase/swap_with_conflict.go | 2 +- .../sync/pull_rebase_interactive_conflict.go | 2 +- .../pull_rebase_interactive_conflict_drop.go | 4 +-- 19 files changed, 81 insertions(+), 63 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 556863de1f0..670a1412749 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -322,13 +322,8 @@ func (self *CommitLoader) getRebasingCommits() []*models.Commit { // See if the current commit couldn't be applied because it conflicted; if // so, add a fake entry for it - if conflictedCommitHash := self.getConflictedCommit(todos); conflictedCommitHash != "" { - commits = append(commits, &models.Commit{ - Hash: conflictedCommitHash, - Name: "", - Status: models.StatusRebasing, - Action: models.ActionConflict, - }) + if conflictedCommit := self.getConflictedCommit(todos); conflictedCommit != nil { + commits = append(commits, conflictedCommit) } for _, t := range todos { @@ -351,17 +346,17 @@ func (self *CommitLoader) getRebasingCommits() []*models.Commit { return commits } -func (self *CommitLoader) getConflictedCommit(todos []todo.Todo) string { +func (self *CommitLoader) getConflictedCommit(todos []todo.Todo) *models.Commit { bytesContent, err := self.readFile(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/done")) if err != nil { self.Log.Error(fmt.Sprintf("error occurred reading rebase-merge/done: %s", err.Error())) - return "" + return nil } doneTodos, err := todo.Parse(bytes.NewBuffer(bytesContent), self.config.GetCoreCommentChar()) if err != nil { self.Log.Error(fmt.Sprintf("error occurred while parsing rebase-merge/done file: %s", err.Error())) - return "" + return nil } amendFileExists := false @@ -372,15 +367,15 @@ func (self *CommitLoader) getConflictedCommit(todos []todo.Todo) string { return self.getConflictedCommitImpl(todos, doneTodos, amendFileExists) } -func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos []todo.Todo, amendFileExists bool) string { +func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos []todo.Todo, amendFileExists bool) *models.Commit { // Should never be possible, but just to be safe: if len(doneTodos) == 0 { self.Log.Error("no done entries in rebase-merge/done file") - return "" + return nil } lastTodo := doneTodos[len(doneTodos)-1] if lastTodo.Command == todo.Break || lastTodo.Command == todo.Exec || lastTodo.Command == todo.Reword { - return "" + return nil } // In certain cases, git reschedules commands that failed. One example is if @@ -391,7 +386,7 @@ func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos [ // same, the command was rescheduled. if len(doneTodos) > 0 && len(todos) > 0 && doneTodos[len(doneTodos)-1] == todos[0] { // Command was rescheduled, no need to display it - return "" + return nil } // Older versions of git have a bug whereby, if a command is rescheduled, @@ -416,26 +411,30 @@ func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos [ if len(doneTodos) >= 3 && len(todos) > 0 && doneTodos[len(doneTodos)-2] == todos[0] && doneTodos[len(doneTodos)-1] == doneTodos[len(doneTodos)-3] { // Command was rescheduled, no need to display it - return "" + return nil } if lastTodo.Command == todo.Edit { if amendFileExists { // Special case for "edit": if the "amend" file exists, the "edit" // command was successful, otherwise it wasn't - return "" + return nil } } // I don't think this is ever possible, but again, just to be safe: if lastTodo.Commit == "" { self.Log.Error("last command in rebase-merge/done file doesn't have a commit") - return "" + return nil } // Any other todo that has a commit associated with it must have failed with // a conflict, otherwise we wouldn't have stopped the rebase: - return lastTodo.Commit + return &models.Commit{ + Hash: lastTodo.Commit, + Action: lastTodo.Command, + Status: models.StatusConflicted, + } } func setCommitMergedStatuses(ancestor string, commits []*models.Commit) { diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index f7b07513454..652fb759cba 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -332,14 +332,14 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { todos []todo.Todo doneTodos []todo.Todo amendFileExists bool - expectedHash string + expectedResult *models.Commit }{ { testName: "no done todos", todos: []todo.Todo{}, doneTodos: []todo.Todo{}, amendFileExists: false, - expectedHash: "", + expectedResult: nil, }, { testName: "common case (conflict)", @@ -355,7 +355,11 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { }, }, amendFileExists: false, - expectedHash: "fa1afe1", + expectedResult: &models.Commit{ + Hash: "fa1afe1", + Action: todo.Pick, + Status: models.StatusConflicted, + }, }, { testName: "last command was 'break'", @@ -364,7 +368,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { {Command: todo.Break}, }, amendFileExists: false, - expectedHash: "", + expectedResult: nil, }, { testName: "last command was 'exec'", @@ -376,7 +380,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { }, }, amendFileExists: false, - expectedHash: "", + expectedResult: nil, }, { testName: "last command was 'reword'", @@ -385,7 +389,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { {Command: todo.Reword}, }, amendFileExists: false, - expectedHash: "", + expectedResult: nil, }, { testName: "'pick' was rescheduled", @@ -402,7 +406,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { }, }, amendFileExists: false, - expectedHash: "", + expectedResult: nil, }, { testName: "'pick' was rescheduled, buggy git version", @@ -427,7 +431,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { }, }, amendFileExists: false, - expectedHash: "", + expectedResult: nil, }, { testName: "conflicting 'pick' after 'exec'", @@ -452,7 +456,11 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { }, }, amendFileExists: false, - expectedHash: "fa1afe1", + expectedResult: &models.Commit{ + Hash: "fa1afe1", + Action: todo.Pick, + Status: models.StatusConflicted, + }, }, { testName: "'edit' with amend file", @@ -464,7 +472,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { }, }, amendFileExists: true, - expectedHash: "", + expectedResult: nil, }, { testName: "'edit' without amend file", @@ -476,7 +484,11 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { }, }, amendFileExists: false, - expectedHash: "fa1afe1", + expectedResult: &models.Commit{ + Hash: "fa1afe1", + Action: todo.Edit, + Status: models.StatusConflicted, + }, }, } for _, scenario := range scenarios { @@ -497,7 +509,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { } hash := builder.getConflictedCommitImpl(scenario.todos, scenario.doneTodos, scenario.amendFileExists) - assert.Equal(t, scenario.expectedHash, hash) + assert.Equal(t, scenario.expectedResult, hash) }) } } diff --git a/pkg/commands/models/commit.go b/pkg/commands/models/commit.go index 752177d9896..b6594705558 100644 --- a/pkg/commands/models/commit.go +++ b/pkg/commands/models/commit.go @@ -18,6 +18,7 @@ const ( StatusPushed StatusMerged StatusRebasing + StatusConflicted StatusReflog ) @@ -25,8 +26,6 @@ const ( // Conveniently for us, the todo package starts the enum at 1, and given // that it doesn't have a "none" value, we're setting ours to 0 ActionNone todo.TodoCommand = 0 - // "Comment" is the last one of the todo package's enum entries - ActionConflict = todo.Comment + 1 ) type Divergence int diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index 45cb80d2f03..3614e232e86 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -829,12 +829,12 @@ func (self *FilesController) handleAmendCommitPress() error { func (self *FilesController) isResolvingConflicts() bool { commits := self.c.Model().Commits for _, c := range commits { + if c.Status == models.StatusConflicted { + return true + } if !c.IsTODO() { break } - if c.Action == models.ActionConflict { - return true - } } return false } diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 0b1b81e72af..12def1c9b2e 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -1389,7 +1389,7 @@ func (self *LocalCommitsController) canMoveDown(selectedCommits []*models.Commit if self.isRebasing() { commits := self.c.Model().Commits - if !commits[endIdx+1].IsTODO() || commits[endIdx+1].Action == models.ActionConflict { + if !commits[endIdx+1].IsTODO() || commits[endIdx+1].Status == models.StatusConflicted { return &types.DisabledReason{Text: self.c.Tr.CannotMoveAnyFurther} } } @@ -1405,7 +1405,7 @@ func (self *LocalCommitsController) canMoveUp(selectedCommits []*models.Commit, if self.isRebasing() { commits := self.c.Model().Commits - if !commits[startIdx-1].IsTODO() || commits[startIdx-1].Action == models.ActionConflict { + if !commits[startIdx-1].IsTODO() || commits[startIdx-1].Status == models.StatusConflicted { return &types.DisabledReason{Text: self.c.Tr.CannotMoveAnyFurther} } } diff --git a/pkg/gui/presentation/commits.go b/pkg/gui/presentation/commits.go index 234a84e5c73..b2cff6e430b 100644 --- a/pkg/gui/presentation/commits.go +++ b/pkg/gui/presentation/commits.go @@ -186,7 +186,7 @@ func GetCommitListDisplayStrings( unfilteredIdx := i + startIdx bisectStatus = getBisectStatus(unfilteredIdx, commit.Hash, bisectInfo, bisectBounds) isYouAreHereCommit := false - if showYouAreHereLabel && (commit.Action == models.ActionConflict || unfilteredIdx == rebaseOffset) { + if showYouAreHereLabel && (commit.Status == models.StatusConflicted || unfilteredIdx == rebaseOffset) { isYouAreHereCommit = true showYouAreHereLabel = false } @@ -395,8 +395,7 @@ func displayCommit( actionString := "" if commit.Action != models.ActionNone { - todoString := lo.Ternary(commit.Action == models.ActionConflict, "conflict", commit.Action.String()) - actionString = actionColorMap(commit.Action).Sprint(todoString) + " " + actionString = actionColorMap(commit.Action, commit.Status).Sprint(commit.Action.String()) + " " } tagString := "" @@ -429,8 +428,13 @@ func displayCommit( mark := "" if isYouAreHereCommit { - color := lo.Ternary(commit.Action == models.ActionConflict, style.FgRed, style.FgYellow) - youAreHere := color.Sprintf("<-- %s ---", common.Tr.YouAreHere) + color := style.FgYellow + text := common.Tr.YouAreHere + if commit.Status == models.StatusConflicted { + color = style.FgRed + text = common.Tr.ConflictLabel + } + youAreHere := color.Sprintf("<-- %s ---", text) mark = fmt.Sprintf("%s ", youAreHere) } else if isMarkedBaseCommit { rebaseFromHere := style.FgYellow.Sprint(common.Tr.MarkedCommitMarker) @@ -501,7 +505,7 @@ func getHashColor( hashColor = style.FgYellow case models.StatusMerged: hashColor = style.FgGreen - case models.StatusRebasing: + case models.StatusRebasing, models.StatusConflicted: hashColor = style.FgBlue case models.StatusReflog: hashColor = style.FgBlue @@ -519,7 +523,11 @@ func getHashColor( return hashColor } -func actionColorMap(action todo.TodoCommand) style.TextStyle { +func actionColorMap(action todo.TodoCommand, status models.CommitStatus) style.TextStyle { + if status == models.StatusConflicted { + return style.FgRed + } + switch action { case todo.Pick: return style.FgCyan @@ -529,8 +537,6 @@ func actionColorMap(action todo.TodoCommand) style.TextStyle { return style.FgGreen case todo.Fixup: return style.FgMagenta - case models.ActionConflict: - return style.FgRed default: return style.FgYellow } diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index aa413c6b5b0..b316f3b8923 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -349,6 +349,7 @@ type TranslationSet struct { ErrorOccurred string NoRoom string YouAreHere string + ConflictLabel string YouDied string RewordNotSupported string ChangingThisActionIsNotAllowed string @@ -1416,6 +1417,7 @@ func EnglishTranslationSet() *TranslationSet { ErrorOccurred: "An error occurred! Please create an issue at", NoRoom: "Not enough room", YouAreHere: "YOU ARE HERE", + ConflictLabel: "CONFLICT", YouDied: "YOU DIED!", RewordNotSupported: "Rewording commits while interactively rebasing is not currently supported", ChangingThisActionIsNotAllowed: "Changing this kind of rebase todo entry is not allowed", diff --git a/pkg/integration/tests/branch/rebase_and_drop.go b/pkg/integration/tests/branch/rebase_and_drop.go index d6c655cf7e5..88e2e39c632 100644 --- a/pkg/integration/tests/branch/rebase_and_drop.go +++ b/pkg/integration/tests/branch/rebase_and_drop.go @@ -53,7 +53,7 @@ var RebaseAndDrop = NewIntegrationTest(NewIntegrationTestArgs{ TopLines( MatchesRegexp(`pick.*to keep`).IsSelected(), MatchesRegexp(`pick.*to remove`), - MatchesRegexp(`conflict.*YOU ARE HERE.*first change`), + MatchesRegexp(`pick.*CONFLICT.*first change`), MatchesRegexp("second-change-branch unrelated change"), MatchesRegexp("second change"), MatchesRegexp("original"), @@ -63,7 +63,7 @@ var RebaseAndDrop = NewIntegrationTest(NewIntegrationTestArgs{ TopLines( MatchesRegexp(`pick.*to keep`), MatchesRegexp(`drop.*to remove`).IsSelected(), - MatchesRegexp(`conflict.*YOU ARE HERE.*first change`), + MatchesRegexp(`pick.*CONFLICT.*first change`), MatchesRegexp("second-change-branch unrelated change"), MatchesRegexp("second change"), MatchesRegexp("original"), diff --git a/pkg/integration/tests/commit/amend_when_there_are_conflicts_and_amend.go b/pkg/integration/tests/commit/amend_when_there_are_conflicts_and_amend.go index 8541310f0c0..c031e0c3da5 100644 --- a/pkg/integration/tests/commit/amend_when_there_are_conflicts_and_amend.go +++ b/pkg/integration/tests/commit/amend_when_there_are_conflicts_and_amend.go @@ -30,7 +30,7 @@ var AmendWhenThereAreConflictsAndAmend = NewIntegrationTest(NewIntegrationTestAr Focus(). Lines( Contains("pick").Contains("commit three"), - Contains("conflict").Contains("<-- YOU ARE HERE --- file1 changed in branch"), + Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch"), Contains("commit two"), Contains("file1 changed in master"), Contains("base commit"), diff --git a/pkg/integration/tests/commit/amend_when_there_are_conflicts_and_cancel.go b/pkg/integration/tests/commit/amend_when_there_are_conflicts_and_cancel.go index 2f16fdf809a..12640963110 100644 --- a/pkg/integration/tests/commit/amend_when_there_are_conflicts_and_cancel.go +++ b/pkg/integration/tests/commit/amend_when_there_are_conflicts_and_cancel.go @@ -34,7 +34,7 @@ var AmendWhenThereAreConflictsAndCancel = NewIntegrationTest(NewIntegrationTestA Focus(). Lines( Contains("pick").Contains("commit three"), - Contains("conflict").Contains("<-- YOU ARE HERE --- file1 changed in branch"), + Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch"), Contains("commit two"), Contains("file1 changed in master"), Contains("base commit"), diff --git a/pkg/integration/tests/commit/shared.go b/pkg/integration/tests/commit/shared.go index b6861f13565..17b06108d9e 100644 --- a/pkg/integration/tests/commit/shared.go +++ b/pkg/integration/tests/commit/shared.go @@ -44,7 +44,7 @@ func doTheRebaseForAmendTests(t *TestDriver, keys config.KeybindingConfig) { t.Views().Commits(). Lines( Contains("pick").Contains("commit three"), - Contains("conflict").Contains("<-- YOU ARE HERE --- file1 changed in branch"), + Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch"), Contains("commit two"), Contains("file1 changed in master"), Contains("base commit"), diff --git a/pkg/integration/tests/interactive_rebase/amend_commit_with_conflict.go b/pkg/integration/tests/interactive_rebase/amend_commit_with_conflict.go index c20ab51e0f8..7cf49ba63eb 100644 --- a/pkg/integration/tests/interactive_rebase/amend_commit_with_conflict.go +++ b/pkg/integration/tests/interactive_rebase/amend_commit_with_conflict.go @@ -35,7 +35,7 @@ var AmendCommitWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ }). Lines( Contains("pick").Contains("three"), - Contains("conflict").Contains("<-- YOU ARE HERE --- fixup! two"), + Contains("fixup").Contains("<-- CONFLICT --- fixup! two"), Contains("two"), Contains("one"), ) @@ -66,7 +66,7 @@ var AmendCommitWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ t.Views().Commits(). Lines( - Contains("<-- YOU ARE HERE --- three"), + Contains("<-- CONFLICT --- three"), Contains("two"), Contains("one"), ) diff --git a/pkg/integration/tests/interactive_rebase/edit_the_confl_commit.go b/pkg/integration/tests/interactive_rebase/edit_the_confl_commit.go index 61b14fafdd5..17d3ffe0e0a 100644 --- a/pkg/integration/tests/interactive_rebase/edit_the_confl_commit.go +++ b/pkg/integration/tests/interactive_rebase/edit_the_confl_commit.go @@ -33,10 +33,10 @@ var EditTheConflCommit = NewIntegrationTest(NewIntegrationTestArgs{ Focus(). Lines( Contains("pick").Contains("commit two"), - Contains("conflict").Contains("<-- YOU ARE HERE --- commit three"), + Contains("pick").Contains("<-- CONFLICT --- commit three"), Contains("commit one"), ). - NavigateToLine(Contains("<-- YOU ARE HERE --- commit three")). + NavigateToLine(Contains("<-- CONFLICT --- commit three")). Press(keys.Commits.RenameCommit) t.ExpectToast(Contains("Disabled: Rewording commits while interactively rebasing is not currently supported")) diff --git a/pkg/integration/tests/interactive_rebase/shared.go b/pkg/integration/tests/interactive_rebase/shared.go index f34cbd7e22c..d2f75622f11 100644 --- a/pkg/integration/tests/interactive_rebase/shared.go +++ b/pkg/integration/tests/interactive_rebase/shared.go @@ -4,13 +4,13 @@ import ( . "github.com/jesseduffield/lazygit/pkg/integration/components" ) -func handleConflictsFromSwap(t *TestDriver) { +func handleConflictsFromSwap(t *TestDriver, expectedCommand string) { t.Common().AcknowledgeConflicts() t.Views().Commits(). Lines( Contains("pick").Contains("commit two"), - Contains("conflict").Contains("<-- YOU ARE HERE --- commit three"), + Contains(expectedCommand).Contains("<-- CONFLICT --- commit three"), Contains("commit one"), ) diff --git a/pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict.go b/pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict.go index d893da4bf99..f330fd5cf83 100644 --- a/pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict.go +++ b/pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict.go @@ -44,6 +44,6 @@ var SwapInRebaseWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ t.Common().ContinueRebase() }) - handleConflictsFromSwap(t) + handleConflictsFromSwap(t, "pick") }, }) diff --git a/pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict_and_edit.go b/pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict_and_edit.go index 494c79552f1..662a8e8cfc1 100644 --- a/pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict_and_edit.go +++ b/pkg/integration/tests/interactive_rebase/swap_in_rebase_with_conflict_and_edit.go @@ -47,6 +47,6 @@ var SwapInRebaseWithConflictAndEdit = NewIntegrationTest(NewIntegrationTestArgs{ t.Common().ContinueRebase() }) - handleConflictsFromSwap(t) + handleConflictsFromSwap(t, "edit") }, }) diff --git a/pkg/integration/tests/interactive_rebase/swap_with_conflict.go b/pkg/integration/tests/interactive_rebase/swap_with_conflict.go index bd498c707af..1ea71356e28 100644 --- a/pkg/integration/tests/interactive_rebase/swap_with_conflict.go +++ b/pkg/integration/tests/interactive_rebase/swap_with_conflict.go @@ -28,6 +28,6 @@ var SwapWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ ). Press(keys.Commits.MoveDownCommit) - handleConflictsFromSwap(t) + handleConflictsFromSwap(t, "pick") }, }) diff --git a/pkg/integration/tests/sync/pull_rebase_interactive_conflict.go b/pkg/integration/tests/sync/pull_rebase_interactive_conflict.go index eb767a1a340..7f1e7721aa7 100644 --- a/pkg/integration/tests/sync/pull_rebase_interactive_conflict.go +++ b/pkg/integration/tests/sync/pull_rebase_interactive_conflict.go @@ -49,7 +49,7 @@ var PullRebaseInteractiveConflict = NewIntegrationTest(NewIntegrationTestArgs{ t.Views().Commits(). Lines( Contains("pick").Contains("five"), - Contains("conflict").Contains("YOU ARE HERE").Contains("four"), + Contains("pick").Contains("CONFLICT").Contains("four"), Contains("three"), Contains("two"), Contains("one"), diff --git a/pkg/integration/tests/sync/pull_rebase_interactive_conflict_drop.go b/pkg/integration/tests/sync/pull_rebase_interactive_conflict_drop.go index 4aff8b4e0df..d9dd51c4593 100644 --- a/pkg/integration/tests/sync/pull_rebase_interactive_conflict_drop.go +++ b/pkg/integration/tests/sync/pull_rebase_interactive_conflict_drop.go @@ -50,7 +50,7 @@ var PullRebaseInteractiveConflictDrop = NewIntegrationTest(NewIntegrationTestArg Focus(). Lines( Contains("pick").Contains("five").IsSelected(), - Contains("conflict").Contains("YOU ARE HERE").Contains("four"), + Contains("pick").Contains("CONFLICT").Contains("four"), Contains("three"), Contains("two"), Contains("one"), @@ -58,7 +58,7 @@ var PullRebaseInteractiveConflictDrop = NewIntegrationTest(NewIntegrationTestArg Press(keys.Universal.Remove). Lines( Contains("drop").Contains("five").IsSelected(), - Contains("conflict").Contains("YOU ARE HERE").Contains("four"), + Contains("pick").Contains("CONFLICT").Contains("four"), Contains("three"), Contains("two"), Contains("one"), From 687bae48ed2370295e40964081684fd875a90b20 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 11 Jun 2024 14:45:51 +0200 Subject: [PATCH 4/9] Simplify the MergeRebasingCommits call in GetCommits (slightly) MergeRebasingCommits already merges the rebasing commits into the commits slice that is passed in, so it doesn't make sense to append the result to commits again. It isn't a problem, but only because commits is always empty. --- pkg/commands/git_commands/commit_loader.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 670a1412749..bffe4a9fb4e 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -71,15 +71,13 @@ type GetCommitsOptions struct { // GetCommits obtains the commits of the current branch func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit, error) { commits := []*models.Commit{} - var rebasingCommits []*models.Commit if opts.IncludeRebaseCommits && opts.FilterPath == "" { var err error - rebasingCommits, err = self.MergeRebasingCommits(commits) + commits, err = self.MergeRebasingCommits(commits) if err != nil { return nil, err } - commits = append(commits, rebasingCommits...) } wg := sync.WaitGroup{} From bb4d03db1fe560fa8d69ace690b3e6772b5aa342 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 11 Jun 2024 15:22:35 +0200 Subject: [PATCH 5/9] Show todos (and conflicting commit) for cherry-pick and revert --- pkg/commands/git_commands/commit_loader.go | 132 +++++++++++++++--- .../revert_with_conflict_multiple_commits.go | 87 ++++++++++++ .../revert_with_conflict_single_commit.go | 6 - ..._multiple_commits_in_interactive_rebase.go | 125 +++++++++++++++++ ...ert_single_commit_in_interactive_rebase.go | 99 +++++++++++++ pkg/integration/tests/test_list.go | 3 + 6 files changed, 426 insertions(+), 26 deletions(-) create mode 100644 pkg/integration/tests/commit/revert_with_conflict_multiple_commits.go create mode 100644 pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go create mode 100644 pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index bffe4a9fb4e..bf31bc287d6 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -169,19 +169,26 @@ func (self *CommitLoader) MergeRebasingCommits(commits []*models.Commit) ([]*mod } } - if !self.getWorkingTreeState().Rebasing { - // not in rebase mode so return original commits - return result, nil + workingTreeState := self.getWorkingTreeState() + addConflictedRebasingCommit := true + if workingTreeState.CherryPicking || workingTreeState.Reverting { + sequencerCommits, err := self.getHydratedSequencerCommits(workingTreeState) + if err != nil { + return nil, err + } + result = append(sequencerCommits, result...) + addConflictedRebasingCommit = false } - rebasingCommits, err := self.getHydratedRebasingCommits() - if err != nil { - return nil, err - } - if len(rebasingCommits) > 0 { - result = append(rebasingCommits, result...) + if workingTreeState.Rebasing { + rebasingCommits, err := self.getHydratedRebasingCommits(addConflictedRebasingCommit) + if err != nil { + return nil, err + } + if len(rebasingCommits) > 0 { + result = append(rebasingCommits, result...) + } } - return result, nil } @@ -240,14 +247,36 @@ func (self *CommitLoader) extractCommitFromLine(line string, showDivergence bool } } -func (self *CommitLoader) getHydratedRebasingCommits() ([]*models.Commit, error) { - commits := self.getRebasingCommits() +func (self *CommitLoader) getHydratedRebasingCommits(addConflictingCommit bool) ([]*models.Commit, error) { + todoFileHasShortHashes := self.version.IsOlderThan(2, 25, 2) + return self.getHydratedTodoCommits(self.getRebasingCommits(addConflictingCommit), todoFileHasShortHashes) +} - if len(commits) == 0 { +func (self *CommitLoader) getHydratedSequencerCommits(workingTreeState models.WorkingTreeState) ([]*models.Commit, error) { + commits := self.getSequencerCommits() + if len(commits) > 0 { + // If we have any commits in .git/sequencer/todo, then the last one of + // those is the conflicting one. + commits[len(commits)-1].Status = models.StatusConflicted + } else { + // For single-commit cherry-picks and reverts, git apparently doesn't + // use the sequencer; in that case, CHERRY_PICK_HEAD or REVERT_HEAD is + // our conflicting commit, so synthesize it here. + conflicedCommit := self.getConflictedSequencerCommit(workingTreeState) + if conflicedCommit != nil { + commits = append(commits, conflicedCommit) + } + } + + return self.getHydratedTodoCommits(commits, true) +} + +func (self *CommitLoader) getHydratedTodoCommits(todoCommits []*models.Commit, todoFileHasShortHashes bool) ([]*models.Commit, error) { + if len(todoCommits) == 0 { return nil, nil } - commitHashes := lo.FilterMap(commits, func(commit *models.Commit, _ int) (string, bool) { + commitHashes := lo.FilterMap(todoCommits, func(commit *models.Commit, _ int) (string, bool) { return commit.Hash, commit.Hash != "" }) @@ -271,7 +300,7 @@ func (self *CommitLoader) getHydratedRebasingCommits() ([]*models.Commit, error) return nil, err } - findFullCommit := lo.Ternary(self.version.IsOlderThan(2, 25, 2), + findFullCommit := lo.Ternary(todoFileHasShortHashes, func(hash string) *models.Commit { for s, c := range fullCommits { if strings.HasPrefix(s, hash) { @@ -284,8 +313,8 @@ func (self *CommitLoader) getHydratedRebasingCommits() ([]*models.Commit, error) return fullCommits[hash] }) - hydratedCommits := make([]*models.Commit, 0, len(commits)) - for _, rebasingCommit := range commits { + hydratedCommits := make([]*models.Commit, 0, len(todoCommits)) + for _, rebasingCommit := range todoCommits { if rebasingCommit.Hash == "" { hydratedCommits = append(hydratedCommits, rebasingCommit) } else if commit := findFullCommit(rebasingCommit.Hash); commit != nil { @@ -302,7 +331,7 @@ func (self *CommitLoader) getHydratedRebasingCommits() ([]*models.Commit, error) // git-rebase-todo example: // pick ac446ae94ee560bdb8d1d057278657b251aaef17 ac446ae // pick afb893148791a2fbd8091aeb81deba4930c73031 afb8931 -func (self *CommitLoader) getRebasingCommits() []*models.Commit { +func (self *CommitLoader) getRebasingCommits(addConflictingCommit bool) []*models.Commit { bytesContent, err := self.readFile(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/git-rebase-todo")) if err != nil { self.Log.Error(fmt.Sprintf("error occurred reading git-rebase-todo: %s", err.Error())) @@ -320,8 +349,10 @@ func (self *CommitLoader) getRebasingCommits() []*models.Commit { // See if the current commit couldn't be applied because it conflicted; if // so, add a fake entry for it - if conflictedCommit := self.getConflictedCommit(todos); conflictedCommit != nil { - commits = append(commits, conflictedCommit) + if addConflictingCommit { + if conflictedCommit := self.getConflictedCommit(todos); conflictedCommit != nil { + commits = append(commits, conflictedCommit) + } } for _, t := range todos { @@ -435,6 +466,67 @@ func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos [ } } +func (self *CommitLoader) getSequencerCommits() []*models.Commit { + bytesContent, err := self.readFile(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "sequencer/todo")) + if err != nil { + self.Log.Error(fmt.Sprintf("error occurred reading sequencer/todo: %s", err.Error())) + // we assume an error means the file doesn't exist so we just return + return nil + } + + commits := []*models.Commit{} + + todos, err := todo.Parse(bytes.NewBuffer(bytesContent), self.config.GetCoreCommentChar()) + if err != nil { + self.Log.Error(fmt.Sprintf("error occurred while parsing sequencer/todo file: %s", err.Error())) + return nil + } + + for _, t := range todos { + if t.Commit == "" { + // Command does not have a commit associated, skip + continue + } + commits = utils.Prepend(commits, &models.Commit{ + Hash: t.Commit, + Name: t.Msg, + Status: models.StatusRebasing, + Action: t.Command, + }) + } + + return commits +} + +func (self *CommitLoader) getConflictedSequencerCommit(workingTreeState models.WorkingTreeState) *models.Commit { + var shaFile string + var action todo.TodoCommand + if workingTreeState.CherryPicking { + shaFile = "CHERRY_PICK_HEAD" + action = todo.Pick + } else if workingTreeState.Reverting { + shaFile = "REVERT_HEAD" + action = todo.Revert + } else { + return nil + } + bytesContent, err := self.readFile(filepath.Join(self.repoPaths.WorktreeGitDirPath(), shaFile)) + if err != nil { + self.Log.Error(fmt.Sprintf("error occurred reading %s: %s", shaFile, err.Error())) + // we assume an error means the file doesn't exist so we just return + return nil + } + lines := strings.Split(string(bytesContent), "\n") + if len(lines) == 0 { + return nil + } + return &models.Commit{ + Hash: lines[0], + Status: models.StatusConflicted, + Action: action, + } +} + func setCommitMergedStatuses(ancestor string, commits []*models.Commit) { if ancestor == "" { return diff --git a/pkg/integration/tests/commit/revert_with_conflict_multiple_commits.go b/pkg/integration/tests/commit/revert_with_conflict_multiple_commits.go new file mode 100644 index 00000000000..b4186d55a20 --- /dev/null +++ b/pkg/integration/tests/commit/revert_with_conflict_multiple_commits.go @@ -0,0 +1,87 @@ +package commit + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var RevertWithConflictMultipleCommits = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Reverts a range of commits, the first of which conflicts", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(cfg *config.AppConfig) { + // TODO: use our revert UI once we support range-select for reverts + cfg.GetUserConfig().CustomCommands = []config.CustomCommand{ + { + Key: "X", + Context: "commits", + Command: "git -c core.editor=: revert HEAD^ HEAD^^", + }, + } + }, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("myfile", "") + shell.Commit("add empty file") + shell.CreateFileAndAdd("otherfile", "") + shell.Commit("unrelated change") + shell.CreateFileAndAdd("myfile", "first line\n") + shell.Commit("add first line") + shell.UpdateFileAndAdd("myfile", "first line\nsecond line\n") + shell.Commit("add second line") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("CI ◯ add second line").IsSelected(), + Contains("CI ◯ add first line"), + Contains("CI ◯ unrelated change"), + Contains("CI ◯ add empty file"), + ). + Press("X"). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Error")). + // The exact error message is different on different git versions, + // but they all contain the word 'conflict' somewhere. + Content(Contains("conflict")). + Confirm() + }). + Lines( + Contains("revert").Contains("CI unrelated change"), + Contains("revert").Contains("CI <-- CONFLICT --- add first line"), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ unrelated change"), + Contains("CI ◯ add empty file"), + ) + + t.Views().Options().Content(Contains("View revert options: m")) + t.Views().Information().Content(Contains("Reverting (Reset)")) + + t.Views().Files().Focus(). + Lines( + Contains("UU myfile").IsSelected(), + ). + PressEnter() + + t.Views().MergeConflicts().IsFocused(). + SelectNextItem(). + PressPrimaryAction() + + t.ExpectPopup().Alert(). + Title(Equals("Continue")). + Content(Contains("All merge conflicts resolved. Continue the revert?")). + Confirm() + + t.Views().Commits(). + Lines( + Contains(`CI ◯ Revert "unrelated change"`), + Contains(`CI ◯ Revert "add first line"`), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ unrelated change"), + Contains("CI ◯ add empty file"), + ) + }, +}) diff --git a/pkg/integration/tests/commit/revert_with_conflict_single_commit.go b/pkg/integration/tests/commit/revert_with_conflict_single_commit.go index 949bf4b3427..48095246420 100644 --- a/pkg/integration/tests/commit/revert_with_conflict_single_commit.go +++ b/pkg/integration/tests/commit/revert_with_conflict_single_commit.go @@ -39,16 +39,10 @@ var RevertWithConflictSingleCommit = NewIntegrationTest(NewIntegrationTestArgs{ Confirm() }). Lines( - /* EXPECTED: - Proper display of revert commits is not implemented yet; we'll do this in the next PR Contains("revert").Contains("CI <-- CONFLICT --- add first line"), Contains("CI ◯ add second line"), Contains("CI ◯ add first line"), Contains("CI ◯ add empty file"), - ACTUAL: */ - Contains("CI ◯ <-- YOU ARE HERE --- add second line"), - Contains("CI ◯ add first line"), - Contains("CI ◯ add empty file"), ) t.Views().Options().Content(Contains("View revert options: m")) diff --git a/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go b/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go new file mode 100644 index 00000000000..6a6dc771a2e --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go @@ -0,0 +1,125 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var RevertMultipleCommitsInInteractiveRebase = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Reverts a range of commits, the first of which conflicts, in the middle of an interactive rebase", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(cfg *config.AppConfig) { + // TODO: use our revert UI once we support range-select for reverts + cfg.GetUserConfig().CustomCommands = []config.CustomCommand{ + { + Key: "X", + Context: "commits", + Command: "git -c core.editor=: revert HEAD^ HEAD^^", + }, + } + }, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("myfile", "") + shell.Commit("add empty file") + shell.CreateFileAndAdd("otherfile", "") + shell.Commit("unrelated change 1") + shell.CreateFileAndAdd("myfile", "first line\n") + shell.Commit("add first line") + shell.UpdateFileAndAdd("myfile", "first line\nsecond line\n") + shell.Commit("add second line") + shell.EmptyCommit("unrelated change 2") + shell.EmptyCommit("unrelated change 3") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("CI ◯ unrelated change 3").IsSelected(), + Contains("CI ◯ unrelated change 2"), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ unrelated change 1"), + Contains("CI ◯ add empty file"), + ). + NavigateToLine(Contains("add second line")). + Press(keys.Universal.Edit). + Press("X"). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Error")). + // The exact error message is different on different git versions, + // but they all contain the word 'conflict' somewhere. + Content(Contains("conflict")). + Confirm() + }). + Lines( + Contains("CI unrelated change 3"), + Contains("CI unrelated change 2"), + Contains("revert").Contains("CI unrelated change 1"), + Contains("revert").Contains("CI <-- CONFLICT --- add first line"), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ unrelated change 1"), + Contains("CI ◯ add empty file"), + ) + + t.Views().Options().Content(Contains("View revert options: m")) + t.Views().Information().Content(Contains("Reverting (Reset)")) + + t.Views().Files().Focus(). + Lines( + Contains("UU myfile").IsSelected(), + ). + PressEnter() + + t.Views().MergeConflicts().IsFocused(). + SelectNextItem(). + PressPrimaryAction() + + t.ExpectPopup().Alert(). + Title(Equals("Continue")). + Content(Contains("All merge conflicts resolved. Continue the revert?")). + Confirm() + + t.Views().Commits(). + Lines( + /* EXPECTED: + Contains("pick").Contains("CI unrelated change 3"), + Contains("pick").Contains("CI unrelated change 2"), + Contains(`CI ◯ <-- YOU ARE HERE --- Revert "unrelated change 1"`), + Contains(`CI ◯ Revert "add first line"`), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ unrelated change 1"), + Contains("CI ◯ add empty file"), + ACTUAL: */ + Contains("pick").Contains("CI unrelated change 3"), + Contains("pick").Contains("CI unrelated change 2"), + Contains("edit CI <-- CONFLICT --- add second line"), + Contains(`CI ◯ Revert "unrelated change 1"`), + Contains(`CI ◯ Revert "add first line"`), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ unrelated change 1"), + Contains("CI ◯ add empty file"), + ) + + t.Views().Options().Content(Contains("View rebase options: m")) + t.Views().Information().Content(Contains("Rebasing (Reset)")) + + t.Common().ContinueRebase() + + t.Views().Commits(). + Lines( + Contains("CI ◯ unrelated change 3"), + Contains("CI ◯ unrelated change 2"), + Contains(`CI ◯ Revert "unrelated change 1"`), + Contains(`CI ◯ Revert "add first line"`), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ unrelated change 1"), + Contains("CI ◯ add empty file"), + ) + }, +}) diff --git a/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go b/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go new file mode 100644 index 00000000000..f6f104dc0c4 --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go @@ -0,0 +1,99 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var RevertSingleCommitInInteractiveRebase = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Reverts a commit that conflicts in the middle of an interactive rebase", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("myfile", "") + shell.Commit("add empty file") + shell.CreateFileAndAdd("myfile", "first line\n") + shell.Commit("add first line") + shell.UpdateFileAndAdd("myfile", "first line\nsecond line\n") + shell.Commit("add second line") + shell.EmptyCommit("unrelated change 1") + shell.EmptyCommit("unrelated change 2") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("CI ◯ unrelated change 2").IsSelected(), + Contains("CI ◯ unrelated change 1"), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ add empty file"), + ). + NavigateToLine(Contains("add second line")). + Press(keys.Universal.Edit). + SelectNextItem(). + Press(keys.Commits.RevertCommit). + Tap(func() { + t.ExpectPopup().Confirmation(). + Title(Equals("Revert commit")). + Content(MatchesRegexp(`Are you sure you want to revert \w+?`)). + Confirm() + t.ExpectPopup().Menu(). + Title(Equals("Conflicts!")). + Select(Contains("View conflicts")). + Confirm() + }). + Lines( + Contains("CI unrelated change 2"), + Contains("CI unrelated change 1"), + Contains("revert").Contains("CI <-- CONFLICT --- add first line"), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line").IsSelected(), + Contains("CI ◯ add empty file"), + ) + + t.Views().Options().Content(Contains("View revert options: m")) + t.Views().Information().Content(Contains("Reverting (Reset)")) + + t.Views().Files().IsFocused(). + Lines( + Contains("UU myfile").IsSelected(), + ). + PressEnter() + + t.Views().MergeConflicts().IsFocused(). + SelectNextItem(). + PressPrimaryAction() + + t.ExpectPopup().Alert(). + Title(Equals("Continue")). + Content(Contains("All merge conflicts resolved. Continue the revert?")). + Confirm() + + t.Views().Commits(). + Lines( + Contains("pick").Contains("CI unrelated change 2"), + Contains("pick").Contains("CI unrelated change 1"), + Contains(`CI ◯ <-- YOU ARE HERE --- Revert "add first line"`), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ add empty file"), + ) + + t.Views().Options().Content(Contains("View rebase options: m")) + t.Views().Information().Content(Contains("Rebasing (Reset)")) + + t.Common().ContinueRebase() + + t.Views().Commits(). + Lines( + Contains("CI ◯ unrelated change 2"), + Contains("CI ◯ unrelated change 1"), + Contains(`CI ◯ Revert "add first line"`), + Contains("CI ◯ add second line"), + Contains("CI ◯ add first line"), + Contains("CI ◯ add empty file"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 2c1e2d1697c..0d4980d882e 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -128,6 +128,7 @@ var tests = []*components.IntegrationTest{ commit.ResetAuthorRange, commit.Revert, commit.RevertMerge, + commit.RevertWithConflictMultipleCommits, commit.RevertWithConflictSingleCommit, commit.Reword, commit.Search, @@ -262,6 +263,8 @@ var tests = []*components.IntegrationTest{ interactive_rebase.QuickStartKeepSelectionRange, interactive_rebase.Rebase, interactive_rebase.RebaseWithCommitThatBecomesEmpty, + interactive_rebase.RevertMultipleCommitsInInteractiveRebase, + interactive_rebase.RevertSingleCommitInInteractiveRebase, interactive_rebase.RewordCommitWithEditorAndFail, interactive_rebase.RewordFirstCommit, interactive_rebase.RewordLastCommit, From 6b6d881624ef679337a26d83f4d94fa79c03c1ba Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 14 Jun 2024 14:52:20 +0200 Subject: [PATCH 6/9] Add test to check that an "edit" entry correctly shows a conflict This works correctly, we are only adding this as a regression test to verify that the change later in this branch doesn't break it. --- ...e_rebase_with_conflict_for_edit_command.go | 63 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 2 files changed, 64 insertions(+) create mode 100644 pkg/integration/tests/interactive_rebase/interactive_rebase_with_conflict_for_edit_command.go diff --git a/pkg/integration/tests/interactive_rebase/interactive_rebase_with_conflict_for_edit_command.go b/pkg/integration/tests/interactive_rebase/interactive_rebase_with_conflict_for_edit_command.go new file mode 100644 index 00000000000..fd09dfe3c83 --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/interactive_rebase_with_conflict_for_edit_command.go @@ -0,0 +1,63 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var InteractiveRebaseWithConflictForEditCommand = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Rebase a branch interactively, and edit a commit that will conflict", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(cfg *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.EmptyCommit("initial commit") + shell.CreateFileAndAdd("file.txt", "master content") + shell.Commit("master commit") + shell.NewBranchFrom("branch", "master^") + shell.CreateNCommits(3) + shell.CreateFileAndAdd("file.txt", "branch content") + shell.Commit("this will conflict") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("this will conflict").IsSelected(), + Contains("commit 03"), + Contains("commit 02"), + Contains("commit 01"), + Contains("initial commit"), + ) + + t.Views().Branches(). + Focus(). + NavigateToLine(Contains("master")). + Press(keys.Branches.RebaseBranch) + + t.ExpectPopup().Menu(). + Title(Equals("Rebase 'branch'")). + Select(Contains("Interactive rebase")). + Confirm() + + t.Views().Commits(). + IsFocused(). + NavigateToLine(Contains("this will conflict")). + Press(keys.Universal.Edit) + + t.Common().ContinueRebase() + t.ExpectPopup().Menu(). + Title(Equals("Conflicts!")). + Cancel() + + t.Views().Commits(). + Lines( + Contains("edit").Contains("<-- CONFLICT --- this will conflict").IsSelected(), + Contains("commit 03"), + Contains("commit 02"), + Contains("commit 01"), + Contains("master commit"), + Contains("initial commit"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 0d4980d882e..7db3f55ddb4 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -250,6 +250,7 @@ var tests = []*components.IntegrationTest{ interactive_rebase.FixupFirstCommit, interactive_rebase.FixupSecondCommit, interactive_rebase.InteractiveRebaseOfCopiedBranch, + interactive_rebase.InteractiveRebaseWithConflictForEditCommand, interactive_rebase.MidRebaseRangeSelect, interactive_rebase.Move, interactive_rebase.MoveAcrossBranchBoundaryOutsideRebase, From 9b88052a4e9d653effae9b5b49936e113b86c2ad Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 14 Jun 2024 13:23:12 +0200 Subject: [PATCH 7/9] Add test demonstrating problem with revert (or cherry-pick) during a rebase This problem can't happen inside of lazygit itself right now, but this will change in the future. It will only happen when you stopped in an interactive rebase on an "edit" entry, and then you perform a revert or cherry-pick consisting of more than one commit; in this situation lazygit will show a conflict although there is none. This is not possible with lazygit yet, as we don't support range-select for reverting, and we don't use `git cherry-pick` for cherry-picking. Both will change in the future, so it's good to fix this bug. --- ...vert_during_rebase_when_stopped_on_edit.go | 67 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 2 files changed, 68 insertions(+) create mode 100644 pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go diff --git a/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go b/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go new file mode 100644 index 00000000000..a2138e8742e --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go @@ -0,0 +1,67 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var RevertDuringRebaseWhenStoppedOnEdit = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Revert a series of commits while stopped in a rebase", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(cfg *config.AppConfig) { + // TODO: use our revert UI once we support range-select for reverts + cfg.GetUserConfig().CustomCommands = []config.CustomCommand{ + { + Key: "X", + Context: "commits", + Command: "git -c core.editor=: revert HEAD^ HEAD^^", + }, + } + }, + SetupRepo: func(shell *Shell) { + shell.EmptyCommit("master commit") + shell.NewBranch("branch") + shell.CreateNCommits(4) + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("commit 04").IsSelected(), + Contains("commit 03"), + Contains("commit 02"), + Contains("commit 01"), + Contains("master commit"), + ). + NavigateToLine(Contains("commit 03")). + Press(keys.Universal.Edit). + Lines( + Contains("pick").Contains("commit 04"), + Contains("<-- YOU ARE HERE --- commit 03").IsSelected(), + Contains("commit 02"), + Contains("commit 01"), + Contains("master commit"), + ). + Press("X"). + Lines( + /* EXPECTED: + Contains("pick").Contains("commit 04"), + Contains(`<-- YOU ARE HERE --- Revert "commit 01"`).IsSelected(), + Contains(`Revert "commit 02"`), + Contains("commit 03"), + Contains("commit 02"), + Contains("commit 01"), + Contains("master commit"), + ACTUAL: */ + Contains("pick").Contains("commit 04"), + Contains("edit").Contains("<-- CONFLICT --- commit 03").IsSelected(), + Contains(`Revert "commit 01"`), + Contains(`Revert "commit 02"`), + Contains("commit 03"), + Contains("commit 02"), + Contains("commit 01"), + Contains("master commit"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 7db3f55ddb4..ed051515f74 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -264,6 +264,7 @@ var tests = []*components.IntegrationTest{ interactive_rebase.QuickStartKeepSelectionRange, interactive_rebase.Rebase, interactive_rebase.RebaseWithCommitThatBecomesEmpty, + interactive_rebase.RevertDuringRebaseWhenStoppedOnEdit, interactive_rebase.RevertMultipleCommitsInInteractiveRebase, interactive_rebase.RevertSingleCommitInInteractiveRebase, interactive_rebase.RewordCommitWithEditorAndFail, From 5ba8d42c80563d1c169e75ef7730851a66c3e828 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 14 Jun 2024 14:36:31 +0200 Subject: [PATCH 8/9] Fix the bug described in the previous commit What happens here is that when stopping on an "edit" todo entry, we rely on the assumption that if the .git/rebase-merge/amend file exists, the command was successful, and if it doesn't, there was a conflict. The problem is that when you stop on an edit command, and then run a multi-commit cherry-pick or rebase, this will delete the amend file. You may or may not consider this a bug in git; to work around it, we also check the existence of the rebase-merge/message file, which will be deleted as well by the cherry-pick or revert. --- pkg/commands/git_commands/commit_loader.go | 18 +++++++---- .../git_commands/commit_loader_test.go | 31 ++++++++++++++----- ...vert_during_rebase_when_stopped_on_edit.go | 10 ------ ..._multiple_commits_in_interactive_rebase.go | 11 ------- 4 files changed, 35 insertions(+), 35 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index bf31bc287d6..2f4556bb969 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -388,15 +388,13 @@ func (self *CommitLoader) getConflictedCommit(todos []todo.Todo) *models.Commit return nil } - amendFileExists := false - if _, err := os.Stat(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/amend")); err == nil { - amendFileExists = true - } + amendFileExists, _ := self.os.FileExists(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/amend")) + messageFileExists, _ := self.os.FileExists(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/message")) - return self.getConflictedCommitImpl(todos, doneTodos, amendFileExists) + return self.getConflictedCommitImpl(todos, doneTodos, amendFileExists, messageFileExists) } -func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos []todo.Todo, amendFileExists bool) *models.Commit { +func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos []todo.Todo, amendFileExists bool, messageFileExists bool) *models.Commit { // Should never be possible, but just to be safe: if len(doneTodos) == 0 { self.Log.Error("no done entries in rebase-merge/done file") @@ -449,6 +447,14 @@ func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos [ // command was successful, otherwise it wasn't return nil } + + if !messageFileExists { + // As an additional check, see if the "message" file exists; if it + // doesn't, it must be because a multi-commit cherry-pick or revert + // was performed in the meantime, which deleted both the amend file + // and the message file. + return nil + } } // I don't think this is ever possible, but again, just to be safe: diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index 652fb759cba..e3e8c346bec 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -328,11 +328,12 @@ func TestGetCommits(t *testing.T) { func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { scenarios := []struct { - testName string - todos []todo.Todo - doneTodos []todo.Todo - amendFileExists bool - expectedResult *models.Commit + testName string + todos []todo.Todo + doneTodos []todo.Todo + amendFileExists bool + messageFileExists bool + expectedResult *models.Commit }{ { testName: "no done todos", @@ -475,7 +476,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { expectedResult: nil, }, { - testName: "'edit' without amend file", + testName: "'edit' without amend file but message file", todos: []todo.Todo{}, doneTodos: []todo.Todo{ { @@ -483,13 +484,27 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { Commit: "fa1afe1", }, }, - amendFileExists: false, + amendFileExists: false, + messageFileExists: true, expectedResult: &models.Commit{ Hash: "fa1afe1", Action: todo.Edit, Status: models.StatusConflicted, }, }, + { + testName: "'edit' without amend and without message file", + todos: []todo.Todo{}, + doneTodos: []todo.Todo{ + { + Command: todo.Edit, + Commit: "fa1afe1", + }, + }, + amendFileExists: false, + messageFileExists: false, + expectedResult: nil, + }, } for _, scenario := range scenarios { t.Run(scenario.testName, func(t *testing.T) { @@ -508,7 +523,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { }, } - hash := builder.getConflictedCommitImpl(scenario.todos, scenario.doneTodos, scenario.amendFileExists) + hash := builder.getConflictedCommitImpl(scenario.todos, scenario.doneTodos, scenario.amendFileExists, scenario.messageFileExists) assert.Equal(t, scenario.expectedResult, hash) }) } diff --git a/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go b/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go index a2138e8742e..1db0f0370d9 100644 --- a/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go +++ b/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go @@ -45,7 +45,6 @@ var RevertDuringRebaseWhenStoppedOnEdit = NewIntegrationTest(NewIntegrationTestA ). Press("X"). Lines( - /* EXPECTED: Contains("pick").Contains("commit 04"), Contains(`<-- YOU ARE HERE --- Revert "commit 01"`).IsSelected(), Contains(`Revert "commit 02"`), @@ -53,15 +52,6 @@ var RevertDuringRebaseWhenStoppedOnEdit = NewIntegrationTest(NewIntegrationTestA Contains("commit 02"), Contains("commit 01"), Contains("master commit"), - ACTUAL: */ - Contains("pick").Contains("commit 04"), - Contains("edit").Contains("<-- CONFLICT --- commit 03").IsSelected(), - Contains(`Revert "commit 01"`), - Contains(`Revert "commit 02"`), - Contains("commit 03"), - Contains("commit 02"), - Contains("commit 01"), - Contains("master commit"), ) }, }) diff --git a/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go b/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go index 6a6dc771a2e..2953e3c3a82 100644 --- a/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go +++ b/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go @@ -84,7 +84,6 @@ var RevertMultipleCommitsInInteractiveRebase = NewIntegrationTest(NewIntegration t.Views().Commits(). Lines( - /* EXPECTED: Contains("pick").Contains("CI unrelated change 3"), Contains("pick").Contains("CI unrelated change 2"), Contains(`CI ◯ <-- YOU ARE HERE --- Revert "unrelated change 1"`), @@ -93,16 +92,6 @@ var RevertMultipleCommitsInInteractiveRebase = NewIntegrationTest(NewIntegration Contains("CI ◯ add first line"), Contains("CI ◯ unrelated change 1"), Contains("CI ◯ add empty file"), - ACTUAL: */ - Contains("pick").Contains("CI unrelated change 3"), - Contains("pick").Contains("CI unrelated change 2"), - Contains("edit CI <-- CONFLICT --- add second line"), - Contains(`CI ◯ Revert "unrelated change 1"`), - Contains(`CI ◯ Revert "add first line"`), - Contains("CI ◯ add second line"), - Contains("CI ◯ add first line"), - Contains("CI ◯ unrelated change 1"), - Contains("CI ◯ add empty file"), ) t.Views().Options().Content(Contains("View rebase options: m")) From b350876f847cbd1c048388bb028d05678b1fbcfa Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 18 Jun 2024 18:01:14 +0200 Subject: [PATCH 9/9] Disallow any changes to commits or todos when cherry-picking or reverting We treat the .git/sequencer/todo file as read-only. Technically it seems it would be possible to treat it as modifiable in the same way as .git/rebase-merge/git-rebase-todo, effectively turning a cherry-pick or revert that stops at a conflict into an interactive rebase; however, git itself doesn't allow this (there is no "git cherry-pick --edit-todo"), so it seems safer not to rely on it. Theoretically it would be possible to allow modifying the rebase todos when a cherry-pick or revert conflicts in the middle of a rebase. However, it would introduce a bit of complexity to support this, as we would have to be able to distinguish between rebasing todos and cherry-picking/reverting todos, which we currently can't; it could also be a bit error-prone as far as edge cases are concerned. And it's really a pretty uncommon situation, so it doesn't seem worth it, and we just forbid all modifications to todos whenever we are cherry-picking or reverting. --- .../controllers/local_commits_controller.go | 21 +++++++++++++++++++ pkg/i18n/english.go | 2 ++ ...ert_single_commit_in_interactive_rebase.go | 14 ++++++++++--- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 12def1c9b2e..9564827204f 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -684,6 +684,11 @@ func (self *LocalCommitsController) isRebasing() bool { return self.c.Model().WorkingTreeStateAtLastCommitRefresh.Any() } +func (self *LocalCommitsController) isCherryPickingOrReverting() bool { + return self.c.Model().WorkingTreeStateAtLastCommitRefresh.CherryPicking || + self.c.Model().WorkingTreeStateAtLastCommitRefresh.Reverting +} + func (self *LocalCommitsController) moveDown(selectedCommits []*models.Commit, startIdx int, endIdx int) error { if self.isRebasing() { if err := self.c.Git().Rebase.MoveTodosDown(selectedCommits); err != nil { @@ -1415,6 +1420,10 @@ func (self *LocalCommitsController) canMoveUp(selectedCommits []*models.Commit, // Ensures that if we are mid-rebase, we're only selecting valid commits (non-conflict TODO commits) func (self *LocalCommitsController) midRebaseCommandEnabled(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason { + if self.isCherryPickingOrReverting() { + return &types.DisabledReason{Text: self.c.Tr.NotAllowedMidCherryPickOrRevert} + } + if !self.isRebasing() { return nil } @@ -1434,6 +1443,10 @@ func (self *LocalCommitsController) midRebaseCommandEnabled(selectedCommits []*m // Ensures that if we are mid-rebase, we're only selecting commits that can be moved func (self *LocalCommitsController) midRebaseMoveCommandEnabled(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason { + if self.isCherryPickingOrReverting() { + return &types.DisabledReason{Text: self.c.Tr.NotAllowedMidCherryPickOrRevert} + } + if !self.isRebasing() { if lo.SomeBy(selectedCommits, func(c *models.Commit) bool { return c.IsMerge() }) { return &types.DisabledReason{Text: self.c.Tr.CannotMoveMergeCommit} @@ -1458,6 +1471,10 @@ func (self *LocalCommitsController) midRebaseMoveCommandEnabled(selectedCommits } func (self *LocalCommitsController) canDropCommits(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason { + if self.isCherryPickingOrReverting() { + return &types.DisabledReason{Text: self.c.Tr.NotAllowedMidCherryPickOrRevert} + } + if !self.isRebasing() { if len(selectedCommits) > 1 && lo.SomeBy(selectedCommits, func(c *models.Commit) bool { return c.IsMerge() }) { return &types.DisabledReason{Text: self.c.Tr.DroppingMergeRequiresSingleSelection} @@ -1502,6 +1519,10 @@ func isChangeOfRebaseTodoAllowed(oldAction todo.TodoCommand) bool { } func (self *LocalCommitsController) pickEnabled(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason { + if self.isCherryPickingOrReverting() { + return &types.DisabledReason{Text: self.c.Tr.NotAllowedMidCherryPickOrRevert} + } + if !self.isRebasing() { // if not rebasing, we're going to do a pull so we don't care about the selection return nil diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index b316f3b8923..5642e696f42 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -353,6 +353,7 @@ type TranslationSet struct { YouDied string RewordNotSupported string ChangingThisActionIsNotAllowed string + NotAllowedMidCherryPickOrRevert string DroppingMergeRequiresSingleSelection string CherryPickCopy string CherryPickCopyTooltip string @@ -1421,6 +1422,7 @@ func EnglishTranslationSet() *TranslationSet { YouDied: "YOU DIED!", RewordNotSupported: "Rewording commits while interactively rebasing is not currently supported", ChangingThisActionIsNotAllowed: "Changing this kind of rebase todo entry is not allowed", + NotAllowedMidCherryPickOrRevert: "This action is not allowed while cherry-picking or reverting", DroppingMergeRequiresSingleSelection: "Dropping a merge commit requires a single selected item", CherryPickCopy: "Copy (cherry-pick)", CherryPickCopyTooltip: "Mark commit as copied. Then, within the local commits view, you can press `{{.paste}}` to paste (cherry-pick) the copied commit(s) into your checked out branch. At any time you can press `{{.escape}}` to cancel the selection.", diff --git a/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go b/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go index f6f104dc0c4..05849f21c43 100644 --- a/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go +++ b/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go @@ -42,7 +42,7 @@ var RevertSingleCommitInInteractiveRebase = NewIntegrationTest(NewIntegrationTes t.ExpectPopup().Menu(). Title(Equals("Conflicts!")). Select(Contains("View conflicts")). - Confirm() + Cancel() // stay in commits panel }). Lines( Contains("CI unrelated change 2"), @@ -51,12 +51,20 @@ var RevertSingleCommitInInteractiveRebase = NewIntegrationTest(NewIntegrationTes Contains("CI ◯ add second line"), Contains("CI ◯ add first line").IsSelected(), Contains("CI ◯ add empty file"), - ) + ). + Press(keys.Commits.MoveDownCommit). + Tap(func() { + t.ExpectToast(Equals("Disabled: This action is not allowed while cherry-picking or reverting")) + }). + Press(keys.Universal.Remove). + Tap(func() { + t.ExpectToast(Equals("Disabled: This action is not allowed while cherry-picking or reverting")) + }) t.Views().Options().Content(Contains("View revert options: m")) t.Views().Information().Content(Contains("Reverting (Reset)")) - t.Views().Files().IsFocused(). + t.Views().Files().Focus(). Lines( Contains("UU myfile").IsSelected(), ).