From debfe1a21f8045ae1afce87b35e9897197493c2d Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 29 Nov 2024 16:24:28 +0100 Subject: [PATCH] Improve editing a commit In 67b8ef449c we changed the "edit" command to insert a "break" after the selected commit, rather than setting the selected todo to "edit". The reason for doing this was that it now works for merge commits too. Back then, I claimed "In most cases the behavior is exactly the same as before." Unfortunately that's not true, there are two reasons why the previous behavior was better (both are demonstrated by tests earlier in this branch): - when editing the last commit of a branch in the middle of a stack of branches, we are now missing the update-ref todo after it, which means that amending the commit breaks the stack - it breaks auto-amending (see the added test earlier in this branch for an explanation) For these reasons, we are going back to the previous approach of setting the selected commit to "edit" whenever possible, i.e. unless it's a merge commit. The only scenario where this could still be a problem is when you have a stack of branches, and the last commit of one of the branches in the stack is a merge commit, and you try to edit that. In my experience with stacked branches this is very unlikely, in almost all cases my stacked branches are linear. --- .../controllers/local_commits_controller.go | 18 ++++++++++++++++-- .../interactive_rebase/edit_and_auto_amend.go | 3 --- .../edit_last_commit_of_stacked_branch.go | 5 ----- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 0d6121d70..96c2ea18b 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -116,7 +116,7 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ }, { Key: opts.GetKey(editCommitKey), - Handler: self.withItems(self.edit), + Handler: self.withItemsRange(self.edit), GetDisabledReason: self.require( self.itemRangeSelected(self.midRebaseCommandEnabled), ), @@ -511,11 +511,25 @@ func (self *LocalCommitsController) drop(selectedCommits []*models.Commit, start return nil } -func (self *LocalCommitsController) edit(selectedCommits []*models.Commit) error { +func (self *LocalCommitsController) edit(selectedCommits []*models.Commit, startIdx int, endIdx int) error { if self.isRebasing() { return self.updateTodos(todo.Edit, selectedCommits) } + commits := self.c.Model().Commits + if !commits[endIdx].IsMerge() { + selectionRangeAndMode := self.getSelectionRangeAndMode() + err := self.c.Git().Rebase.InteractiveRebase(commits, startIdx, endIdx, todo.Edit) + return self.c.Helpers().MergeAndRebase.CheckMergeOrRebaseWithRefreshOptions( + err, + types.RefreshOptions{ + Mode: types.BLOCK_UI, Then: func() error { + self.restoreSelectionRangeAndMode(selectionRangeAndMode) + return nil + }, + }) + } + return self.startInteractiveRebaseWithEdit(selectedCommits) } diff --git a/pkg/integration/tests/interactive_rebase/edit_and_auto_amend.go b/pkg/integration/tests/interactive_rebase/edit_and_auto_amend.go index 3171893ce..8c569ede6 100644 --- a/pkg/integration/tests/interactive_rebase/edit_and_auto_amend.go +++ b/pkg/integration/tests/interactive_rebase/edit_and_auto_amend.go @@ -50,9 +50,6 @@ var EditAndAutoAmend = NewIntegrationTest(NewIntegrationTestArgs{ ) t.Views().Main(). - /* EXPECTED: Content(Contains("fixup content")) - ACTUAL: */ - Content(DoesNotContain("fixup content")) }, }) diff --git a/pkg/integration/tests/interactive_rebase/edit_last_commit_of_stacked_branch.go b/pkg/integration/tests/interactive_rebase/edit_last_commit_of_stacked_branch.go index 92571173e..35d89e8e9 100644 --- a/pkg/integration/tests/interactive_rebase/edit_last_commit_of_stacked_branch.go +++ b/pkg/integration/tests/interactive_rebase/edit_last_commit_of_stacked_branch.go @@ -39,9 +39,7 @@ var EditLastCommitOfStackedBranch = NewIntegrationTest(NewIntegrationTestArgs{ Lines( Contains("pick").Contains("CI commit 05"), Contains("pick").Contains("CI commit 04"), - /* EXPECTED: Contains("update-ref").Contains("branch1"), - */ Contains("<-- YOU ARE HERE --- * commit 03").IsSelected(), Contains("CI commit 02"), Contains("CI commit 01"), @@ -68,10 +66,7 @@ var EditLastCommitOfStackedBranch = NewIntegrationTest(NewIntegrationTestArgs{ Lines( Contains("CI commit 05"), Contains("CI commit 04"), - /* EXPECTED: Contains("CI * commit 03"), - ACTUAL: */ - Contains("CI commit 03"), Contains("CI commit 02"), Contains("CI commit 01"), )