Adjust selection after squashing fixups (#3338)

- **PR Description**

Keep the same commit selected after squashing fixup commits, and after
creating fixup commits.

Edge case: it is now possible to have a range of commits selected when
squashing fixups (by using the "in current branch" version of the
command), and in that case we don't bother preserving the range. It
would be possible, but would require more code, and I don't think it's
worth it, so I'm simply collapsing the range in that case.

pkg/gui/controllers/local_commits_controller.go

@@ -2,6 +2,7 @@ package controllers
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/fsmiamoto/git-todo-parser/todo"
 	"github.com/go-errors/errors"
@@ -811,11 +812,14 @@ func (self *LocalCommitsController) createFixupCommit(commit *models.Commit) err
 		HandleConfirm: func() error {
 			return self.c.Helpers().WorkingTree.WithEnsureCommitableFiles(func() error {
 				self.c.LogAction(self.c.Tr.Actions.CreateFixupCommit)
-				if err := self.c.Git().Commit.CreateFixupCommit(commit.Sha); err != nil {
-					return self.c.Error(err)
-				}
+				return self.c.WithWaitingStatusSync(self.c.Tr.CreatingFixupCommitStatus, func() error {
+					if err := self.c.Git().Commit.CreateFixupCommit(commit.Sha); err != nil {
+						return self.c.Error(err)
+					}
 
-				return self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC})
+					self.context().MoveSelectedLine(1)
+					return self.c.Refresh(types.RefreshOptions{Mode: types.SYNC})
+				})
 			})
 		},
 	})
@@ -844,37 +848,89 @@ func (self *LocalCommitsController) squashFixupCommits() error {
 }
 
 func (self *LocalCommitsController) squashAllFixupsAboveSelectedCommit(commit *models.Commit) error {
-	return self.c.WithWaitingStatus(self.c.Tr.SquashingStatus, func(gocui.Task) error {
-		self.c.LogAction(self.c.Tr.Actions.SquashAllAboveFixupCommits)
-		err := self.c.Git().Rebase.SquashAllAboveFixupCommits(commit)
-		return self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err)
-	})
+	return self.squashFixupsImpl(commit, self.context().GetSelectedLineIdx())
 }
 
 func (self *LocalCommitsController) squashAllFixupsInCurrentBranch() error {
-	commit, err := self.findCommitForSquashFixupsInCurrentBranch()
+	commit, rebaseStartIdx, err := self.findCommitForSquashFixupsInCurrentBranch()
 	if err != nil {
 		return self.c.Error(err)
 	}
 
-	return self.c.WithWaitingStatus(self.c.Tr.SquashingStatus, func(gocui.Task) error {
+	return self.squashFixupsImpl(commit, rebaseStartIdx)
+}
+
+func (self *LocalCommitsController) squashFixupsImpl(commit *models.Commit, rebaseStartIdx int) error {
+	selectionOffset := countSquashableCommitsAbove(self.c.Model().Commits, self.context().GetSelectedLineIdx(), rebaseStartIdx)
+	return self.c.WithWaitingStatusSync(self.c.Tr.SquashingStatus, func() error {
 		self.c.LogAction(self.c.Tr.Actions.SquashAllAboveFixupCommits)
 		err := self.c.Git().Rebase.SquashAllAboveFixupCommits(commit)
-		return self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err)
+		self.context().MoveSelectedLine(-selectionOffset)
+		return self.c.Helpers().MergeAndRebase.CheckMergeOrRebaseWithRefreshOptions(
+			err, types.RefreshOptions{Mode: types.SYNC})
 	})
 }
 
-func (self *LocalCommitsController) findCommitForSquashFixupsInCurrentBranch() (*models.Commit, error) {
+func (self *LocalCommitsController) findCommitForSquashFixupsInCurrentBranch() (*models.Commit, int, error) {
 	commits := self.c.Model().Commits
 	_, index, ok := lo.FindIndexOf(commits, func(c *models.Commit) bool {
 		return c.IsMerge() || c.Status == models.StatusMerged
 	})
 
 	if !ok || index == 0 {
-		return nil, errors.New(self.c.Tr.CannotSquashCommitsInCurrentBranch)
+		return nil, -1, errors.New(self.c.Tr.CannotSquashCommitsInCurrentBranch)
 	}
 
-	return commits[index-1], nil
+	return commits[index-1], index - 1, nil
+}
+
+// Anticipate how many commits above the selectedIdx are going to get squashed
+// by the SquashAllAboveFixupCommits call, so that we can adjust the selection
+// afterwards. Let's hope we're matching git's behavior correctly here.
+func countSquashableCommitsAbove(commits []*models.Commit, selectedIdx int, rebaseStartIdx int) int {
+	result := 0
+
+	// For each commit _above_ the selection, ...
+	for i, commit := range commits[0:selectedIdx] {
+		// ... see if it is a fixup commit, and get the base subject it applies to
+		if baseSubject, isFixup := isFixupCommit(commit.Name); isFixup {
+			// Then, for each commit after the fixup, up to and including the
+			// rebase start commit, see if we find the base commit
+			for _, baseCommit := range commits[i+1 : rebaseStartIdx+1] {
+				if strings.HasPrefix(baseCommit.Name, baseSubject) {
+					result++
+				}
+			}
+		}
+	}
+	return result
+}
+
+// Check whether the given subject line is the subject of a fixup commit, and
+// returns (trimmedSubject, true) if so (where trimmedSubject is the subject
+// with all fixup prefixes removed), or (subject, false) if not.
+func isFixupCommit(subject string) (string, bool) {
+	prefixes := []string{"fixup! ", "squash! ", "amend! "}
+	trimPrefix := func(s string) (string, bool) {
+		for _, prefix := range prefixes {
+			if strings.HasPrefix(s, prefix) {
+				return strings.TrimPrefix(s, prefix), true
+			}
+		}
+		return s, false
+	}
+
+	if subject, wasTrimmed := trimPrefix(subject); wasTrimmed {
+		for {
+			// handle repeated prefixes like "fixup! amend! fixup! Subject"
+			if subject, wasTrimmed = trimPrefix(subject); !wasTrimmed {
+				break
+			}
+		}
+		return subject, true
+	}
+
+	return subject, false
 }
 
 func (self *LocalCommitsController) createTag(commit *models.Commit) error {
@@ -1067,7 +1123,7 @@ func (self *LocalCommitsController) canFindCommitForQuickStart() *types.Disabled
 }
 
 func (self *LocalCommitsController) canFindCommitForSquashFixupsInCurrentBranch() *types.DisabledReason {
-	if _, err := self.findCommitForSquashFixupsInCurrentBranch(); err != nil {
+	if _, _, err := self.findCommitForSquashFixupsInCurrentBranch(); err != nil {
 		return &types.DisabledReason{Text: err.Error()}
 	}
 

pkg/gui/controllers/local_commits_controller_test.go

@@ -0,0 +1,141 @@
+package controllers
+
+import (
+	"testing"
+
+	"github.com/jesseduffield/lazygit/pkg/commands/models"
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_countSquashableCommitsAbove(t *testing.T) {
+	scenarios := []struct {
+		name           string
+		commits        []*models.Commit
+		selectedIdx    int
+		rebaseStartIdx int
+		expectedResult int
+	}{
+		{
+			name: "no squashable commits",
+			commits: []*models.Commit{
+				{Name: "abc"},
+				{Name: "def"},
+				{Name: "ghi"},
+			},
+			selectedIdx:    2,
+			rebaseStartIdx: 2,
+			expectedResult: 0,
+		},
+		{
+			name: "some squashable commits, including for the selected commit",
+			commits: []*models.Commit{
+				{Name: "fixup! def"},
+				{Name: "fixup! ghi"},
+				{Name: "abc"},
+				{Name: "def"},
+				{Name: "ghi"},
+			},
+			selectedIdx:    4,
+			rebaseStartIdx: 4,
+			expectedResult: 2,
+		},
+		{
+			name: "base commit is below rebase start",
+			commits: []*models.Commit{
+				{Name: "fixup! def"},
+				{Name: "abc"},
+				{Name: "def"},
+			},
+			selectedIdx:    1,
+			rebaseStartIdx: 1,
+			expectedResult: 0,
+		},
+		{
+			name: "base commit does not exist at all",
+			commits: []*models.Commit{
+				{Name: "fixup! xyz"},
+				{Name: "abc"},
+				{Name: "def"},
+			},
+			selectedIdx:    2,
+			rebaseStartIdx: 2,
+			expectedResult: 0,
+		},
+		{
+			name: "selected commit is in the middle of fixups",
+			commits: []*models.Commit{
+				{Name: "fixup! def"},
+				{Name: "abc"},
+				{Name: "fixup! ghi"},
+				{Name: "def"},
+				{Name: "ghi"},
+			},
+			selectedIdx:    1,
+			rebaseStartIdx: 4,
+			expectedResult: 1,
+		},
+		{
+			name: "selected commit is after rebase start",
+			commits: []*models.Commit{
+				{Name: "fixup! def"},
+				{Name: "abc"},
+				{Name: "def"},
+				{Name: "ghi"},
+			},
+			selectedIdx:    3,
+			rebaseStartIdx: 2,
+			expectedResult: 1,
+		},
+	}
+	for _, s := range scenarios {
+		t.Run(s.name, func(t *testing.T) {
+			assert.Equal(t, s.expectedResult, countSquashableCommitsAbove(s.commits, s.selectedIdx, s.rebaseStartIdx))
+		})
+	}
+}
+
+func Test_isFixupCommit(t *testing.T) {
+	scenarios := []struct {
+		subject                string
+		expectedTrimmedSubject string
+		expectedIsFixup        bool
+	}{
+		{
+			subject:                "Bla",
+			expectedTrimmedSubject: "Bla",
+			expectedIsFixup:        false,
+		},
+		{
+			subject:                "fixup Bla",
+			expectedTrimmedSubject: "fixup Bla",
+			expectedIsFixup:        false,
+		},
+		{
+			subject:                "fixup! Bla",
+			expectedTrimmedSubject: "Bla",
+			expectedIsFixup:        true,
+		},
+		{
+			subject:                "fixup! fixup! Bla",
+			expectedTrimmedSubject: "Bla",
+			expectedIsFixup:        true,
+		},
+		{
+			subject:                "amend! squash! Bla",
+			expectedTrimmedSubject: "Bla",
+			expectedIsFixup:        true,
+		},
+		{
+			subject:                "fixup!",
+			expectedTrimmedSubject: "fixup!",
+			expectedIsFixup:        false,
+		},
+	}
+	for _, s := range scenarios {
+		t.Run(s.subject, func(t *testing.T) {
+			trimmedSubject, isFixupCommit := isFixupCommit(s.subject)
+			assert.Equal(t, s.expectedTrimmedSubject, trimmedSubject)
+			assert.Equal(t, s.expectedIsFixup, isFixupCommit)
+		})
+	}
+}

pkg/i18n/english.go

@@ -341,6 +341,7 @@ type TranslationSet struct {
 	CheckingOutStatus                     string
 	CommittingStatus                      string
 	RevertingStatus                       string
+	CreatingFixupCommitStatus             string
 	CommitFiles                           string
 	SubCommitsDynamicTitle                string
 	CommitFilesDynamicTitle               string
@@ -1289,6 +1290,7 @@ func EnglishTranslationSet() TranslationSet {
 		CheckingOutStatus:                   "Checking out",
 		CommittingStatus:                    "Committing",
 		RevertingStatus:                     "Reverting",
+		CreatingFixupCommitStatus:           "Creating fixup commit",
 		CommitFiles:                         "Commit files",
 		SubCommitsDynamicTitle:              "Commits (%s)",
 		CommitFilesDynamicTitle:             "Diff files (%s)",

pkg/integration/tests/interactive_rebase/squash_fixups_above.go

@@ -0,0 +1,56 @@
+package interactive_rebase
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var SquashFixupsAbove = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Squashes all fixups above a commit and checks that the selected line stays correct.",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.
+			CreateNCommits(3).
+			CreateFileAndAdd("fixup-file", "fixup content")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Commits().
+			Focus().
+			Lines(
+				Contains("commit 03"),
+				Contains("commit 02"),
+				Contains("commit 01"),
+			).
+			NavigateToLine(Contains("commit 02")).
+			Press(keys.Commits.CreateFixupCommit).
+			Tap(func() {
+				t.ExpectPopup().Confirmation().
+					Title(Equals("Create fixup commit")).
+					Content(Contains("Are you sure you want to create a fixup! commit for commit")).
+					Confirm()
+			}).
+			Lines(
+				Contains("fixup! commit 02"),
+				Contains("commit 03"),
+				Contains("commit 02").IsSelected(),
+				Contains("commit 01"),
+			).
+			Press(keys.Commits.SquashAboveCommits).
+			Tap(func() {
+				t.ExpectPopup().Menu().
+					Title(Equals("Apply fixup commits")).
+					Select(Contains("Above the selected commit")).
+					Confirm()
+			}).
+			Lines(
+				Contains("commit 03"),
+				Contains("commit 02").IsSelected(),
+				Contains("commit 01"),
+			)
+
+		t.Views().Main().
+			Content(Contains("fixup content"))
+	},
+})

pkg/integration/tests/interactive_rebase/squash_fixups_in_current_branch.go

@@ -27,10 +27,12 @@ var SquashFixupsInCurrentBranch = NewIntegrationTest(NewIntegrationTestArgs{
 	Run: func(t *TestDriver, keys config.KeybindingConfig) {
 		t.Views().Commits().
 			Focus().
+			SelectNextItem().
+			SelectNextItem().
 			Lines(
 				Contains("fixup! commit 01"),
 				Contains("commit 02"),
-				Contains("commit 01"),
+				Contains("commit 01").IsSelected(),
 				Contains("fixup! master commit"),
 				Contains("master commit"),
 			).
@@ -43,11 +45,10 @@ var SquashFixupsInCurrentBranch = NewIntegrationTest(NewIntegrationTestArgs{
 			}).
 			Lines(
 				Contains("commit 02"),
-				Contains("commit 01"),
+				Contains("commit 01").IsSelected(),
 				Contains("fixup! master commit"),
 				Contains("master commit"),
-			).
-			NavigateToLine(Contains("commit 01"))
+			)
 
 		t.Views().Main().
 			Content(Contains("fixup content"))

pkg/integration/tests/test_list.go

@@ -188,6 +188,7 @@ var tests = []*components.IntegrationTest{
 	interactive_rebase.RewordYouAreHereCommitWithEditor,
 	interactive_rebase.SquashDownFirstCommit,
 	interactive_rebase.SquashDownSecondCommit,
+	interactive_rebase.SquashFixupsAbove,
 	interactive_rebase.SquashFixupsAboveFirstCommit,
 	interactive_rebase.SquashFixupsInCurrentBranch,
 	interactive_rebase.SwapInRebaseWithConflict,