Skip to content

Commit

Permalink
Improve editing a commit
Browse files Browse the repository at this point in the history
In 67b8ef4 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.
  • Loading branch information
stefanhaller committed Dec 1, 2024
1 parent d849868 commit debfe1a
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 10 deletions.
18 changes: 16 additions & 2 deletions pkg/gui/controllers/local_commits_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
),
Expand Down Expand Up @@ -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)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ var EditAndAutoAmend = NewIntegrationTest(NewIntegrationTestArgs{
)

t.Views().Main().
/* EXPECTED:
Content(Contains("fixup content"))
ACTUAL: */
Content(DoesNotContain("fixup content"))
},
})
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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"),
)
Expand Down

0 comments on commit debfe1a

Please sign in to comment.