Have "Find base commit for fixup" ignore fixup commits for the found base commit (#5210)

If the ctrl-f function (Find base commit for fixup) finds a base commit
plus a bunch of fixup! commits for that base commit, it ignores those
fixups and selects the base commit. For the purpose of creating another
fixup commit on top this is always what you want, so I'm doing this
without adding a confirmation. If the user presses shift-A after that to
amend their changes though, they are guaranteed to get conflicts, so for
that case a warning might be useful; however, I find it unlikely that
users will want to amend changes to a commit that they already created
fixups for, so I'm just hoping that this won't happen in practice.

This implementation is a bit stricter than #5201, in that it only
ignores fixups if the first found commit is not a fixup itself; because
if it is, it's not clear whether the user wants to create another fixup
for both on top, or amend the changes into each one, in which case they
need to be staged individually.

pkg/gui/controllers/helpers/fixup_helper.go

@@ -110,20 +110,24 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error {
 		return errors.New(self.c.Tr.BaseCommitIsAlreadyOnMainBranch)
 	}
 
-	if len(hashGroups[NOT_MERGED]) > 1 {
-		// If there are multiple commits that could be the base commit, list
+	foundCommits := getCommitsForHashes(commits, hashGroups[NOT_MERGED])
+	// If there are multiple commits that could be the base commit, remove all
+	// those that are fixups for the last one.
+	foundCommits = removeFixupCommits(foundCommits)
+
+	if len(foundCommits) > 1 {
+		// If there are still multiple commits that could be the base commit, list
 		// them in the error message. But only the candidates from the current
 		// branch, not including any that are already merged.
-		subjects := self.getHashesAndSubjects(commits, hashGroups[NOT_MERGED])
+		subjects := getHashesAndSubjects(foundCommits)
 		message := lo.Ternary(hasStagedChanges,
 			self.c.Tr.MultipleBaseCommitsFoundStaged,
 			self.c.Tr.MultipleBaseCommitsFoundUnstaged)
 		return fmt.Errorf("%s\n\n%s", message, subjects)
 	}
 
-	// At this point we know that the NOT_MERGED bucket has exactly one commit,
-	// and that's the one we want to select.
-	_, index, _ := self.findCommit(commits, hashGroups[NOT_MERGED][0])
+	// Now we know that foundCommits has exactly one commit, so find its index
+	_, index, _ := self.findCommit(commits, foundCommits[0].Hash())
 
 	return self.c.ConfirmIf(warnAboutAddedLines, types.ConfirmOpts{
 		Title:  self.c.Tr.FindBaseCommitForFixup,
@@ -144,23 +148,52 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error {
 	})
 }
 
-func (self *FixupHelper) getHashesAndSubjects(commits []*models.Commit, hashes []string) string {
+func getCommitsForHashes(commits []*models.Commit, hashes []string) []*models.Commit {
 	// This is called only for the NOT_MERGED commits, and we know that all of them are contained in
 	// the commits slice.
 	commitsSet := set.NewFromSlice(hashes)
-	subjects := make([]string, 0, len(hashes))
+	result := make([]*models.Commit, 0, len(hashes))
 	for _, c := range commits {
 		if commitsSet.Includes(c.Hash()) {
-			subjects = append(subjects, fmt.Sprintf("%s %s", c.ShortRefName(), c.Name))
+			result = append(result, c)
 			commitsSet.Remove(c.Hash())
 			if commitsSet.Len() == 0 {
 				break
 			}
 		}
 	}
+	return result
+}
+
+func getHashesAndSubjects(commits []*models.Commit) string {
+	subjects := lo.Map(commits, func(c *models.Commit, _ int) string {
+		return fmt.Sprintf("%s %s", c.ShortRefName(), c.Name)
+	})
 	return strings.Join(subjects, "\n")
 }
 
+func removeFixupCommits(commits []*models.Commit) []*models.Commit {
+	if len(commits) <= 1 {
+		return commits
+	}
+
+	// If the last found commit is itself a fixup, don't eliminate anything.
+	baseSubject, lastIsFixup := IsFixupCommit(commits[len(commits)-1].Name)
+	if lastIsFixup {
+		return commits
+	}
+
+	// need to go backwards because we are mutating the slice as we go
+	for i := len(commits) - 2; i >= 0; i-- {
+		subject, isFixup := IsFixupCommit(commits[i].Name)
+		if isFixup && subject == baseSubject {
+			commits = utils.Remove(commits, i)
+		}
+	}
+
+	return commits
+}
+
 func (self *FixupHelper) getDiff() (string, bool, error) {
 	args := []string{"-U0", "--ignore-submodules=all", "HEAD", "--"}
 
@@ -350,3 +383,30 @@ func (self *FixupHelper) findCommit(commits []*models.Commit, hash string) (*mod
 		return commit.Hash() == hash
 	})
 }
+
+// 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 trimmedSubject, ok := strings.CutPrefix(s, prefix); ok {
+				return trimmedSubject, 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
+}

pkg/gui/controllers/helpers/fixup_helper_test.go

@@ -3,6 +3,9 @@ package helpers
 import (
 	"testing"
 
+	"github.com/jesseduffield/lazygit/pkg/commands/models"
+	"github.com/jesseduffield/lazygit/pkg/utils"
+	"github.com/samber/lo"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -155,3 +158,184 @@ index 9ce8efb33..0632e41b0 100644
 		})
 	}
 }
+
+func TestFixupHelper_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)
+		})
+	}
+}
+
+func TestFixupHelper_removeFixupCommits(t *testing.T) {
+	hashPool := &utils.StringPool{}
+
+	type commitDesc struct {
+		Hash string
+		Name string
+	}
+
+	scenarios := []struct {
+		name           string
+		commits        []commitDesc
+		expectedResult []commitDesc
+	}{
+		{
+			name:           "empty list",
+			commits:        []commitDesc{},
+			expectedResult: []commitDesc{},
+		},
+		{
+			name: "single commit",
+			commits: []commitDesc{
+				{"abc123", "Some feature"},
+			},
+			expectedResult: []commitDesc{
+				{"abc123", "Some feature"},
+			},
+		},
+		{
+			name: "two unrelated commits",
+			commits: []commitDesc{
+				{"abc123", "First feature"},
+				{"def456", "Second feature"},
+			},
+			expectedResult: []commitDesc{
+				{"abc123", "First feature"},
+				{"def456", "Second feature"},
+			},
+		},
+		{
+			name: "fixup commit for last commit",
+			commits: []commitDesc{
+				{"abc123", "fixup! Some feature"},
+				{"def456", "Some feature"},
+			},
+			expectedResult: []commitDesc{
+				{"def456", "Some feature"},
+			},
+		},
+		{
+			name: "amend and squash commits for last commit",
+			commits: []commitDesc{
+				{"abc123", "squash! Some feature"},
+				{"def456", "amend! Some feature"},
+				{"ghi789", "Some feature"},
+			},
+			expectedResult: []commitDesc{
+				{"ghi789", "Some feature"},
+			},
+		},
+		{
+			name: "fixup commit for different commit",
+			commits: []commitDesc{
+				{"abc123", "fixup! Other feature"},
+				{"def456", "Some feature"},
+			},
+			expectedResult: []commitDesc{
+				{"abc123", "fixup! Other feature"},
+				{"def456", "Some feature"},
+			},
+		},
+		{
+			name: "last commit is a fixup itself",
+			commits: []commitDesc{
+				{"abc123", "fixup! Some feature"},
+				{"def456", "fixup! Some feature"},
+			},
+			expectedResult: []commitDesc{
+				{"abc123", "fixup! Some feature"},
+				{"def456", "fixup! Some feature"},
+			},
+		},
+		{
+			name: "nested fixup commit",
+			commits: []commitDesc{
+				{"abc123", "fixup! fixup! Some feature"},
+				{"def456", "amend! squash! fixup! Some feature"},
+				{"ghi789", "Some feature"},
+			},
+			expectedResult: []commitDesc{
+				{"ghi789", "Some feature"},
+			},
+		},
+		{
+			name: "fixup commits mixed with unrelated commits",
+			commits: []commitDesc{
+				{Hash: "abc123", Name: "fixup! Base commit"},
+				{Hash: "def456", Name: "Unrelated commit"},
+				{Hash: "ghi789", Name: "fixup! Base commit"},
+				{Hash: "jkl012", Name: "Base commit"},
+			},
+			expectedResult: []commitDesc{
+				{Hash: "def456", Name: "Unrelated commit"},
+				{Hash: "jkl012", Name: "Base commit"},
+			},
+		},
+		{
+			name: "only fixup commits for last commit removed, others preserved",
+			commits: []commitDesc{
+				{Hash: "abc123", Name: "fixup! First feature"},
+				{Hash: "def456", Name: "fixup! Second feature"},
+				{Hash: "ghi789", Name: "Second feature"},
+				{Hash: "jkl012", Name: "First feature"},
+			},
+			expectedResult: []commitDesc{
+				{Hash: "def456", Name: "fixup! Second feature"},
+				{Hash: "ghi789", Name: "Second feature"},
+				{Hash: "jkl012", Name: "First feature"},
+			},
+		},
+	}
+
+	makeCommitFromDesc := func(desc commitDesc, _ int) *models.Commit {
+		return models.NewCommit(hashPool, models.NewCommitOpts{Hash: desc.Hash, Name: desc.Name})
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.name, func(t *testing.T) {
+			commits := lo.Map(s.commits, makeCommitFromDesc)
+			result := removeFixupCommits(commits)
+			expectedCommits := lo.Map(s.expectedResult, makeCommitFromDesc)
+			assert.Equal(t, expectedCommits, result)
+		})
+	}
+}

pkg/gui/controllers/local_commits_controller.go

@@ -1096,7 +1096,7 @@ func countSquashableCommitsAbove(commits []*models.Commit, selectedIdx int, reba
 	// 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 {
+		if baseSubject, isFixup := helpers.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] {
@@ -1109,33 +1109,6 @@ func countSquashableCommitsAbove(commits []*models.Commit, selectedIdx int, reba
 	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 trimmedSubject, ok := strings.CutPrefix(s, prefix); ok {
-				return trimmedSubject, 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 {
 	return self.c.Helpers().Tags.OpenCreateTagPrompt(commit.Hash(), func() {})
 }

pkg/gui/controllers/local_commits_controller_test.go

@@ -93,49 +93,3 @@ func Test_countSquashableCommitsAbove(t *testing.T) {
 		})
 	}
 }
-
-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/integration/tests/commit/find_base_commit_for_fixup_disregard_fixups_for_same_base_commit.go

@@ -0,0 +1,46 @@
+package commit
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var FindBaseCommitForFixupDisregardFixupsForSameBaseCommit = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Finds the base commit to create a fixup for, disregarding fixup commits for the same base commit",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.
+			EmptyCommit("1st commit").
+			NewBranch("mybranch").
+			CreateFileAndAdd("file1", "1\n2\n3\n").
+			Commit("2nd commit").
+			UpdateFileAndAdd("file1", "1\n2\n3a\n").
+			Commit("fixup! 2nd commit").
+			EmptyCommit("3rd commit").
+			UpdateFileAndAdd("file1", "1a\n2\n3b\n")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Commits().
+			Lines(
+				Contains("3rd commit").IsSelected(),
+				Contains("fixup! 2nd commit"),
+				Contains("2nd commit"),
+				Contains("1st commit"),
+			)
+
+		t.Views().Files().
+			Focus().
+			Press(keys.Files.FindBaseCommitForFixup)
+
+		t.Views().Commits().
+			IsFocused().
+			Lines(
+				Contains("3rd commit"),
+				Contains("fixup! 2nd commit"),
+				Contains("2nd commit").IsSelected(),
+				Contains("1st commit"),
+			)
+	},
+})

pkg/integration/tests/test_list.go

@@ -130,6 +130,7 @@ var tests = []*components.IntegrationTest{
 	commit.DoNotShowBranchMarkerForHeadCommit,
 	commit.FailHooksThenCommitNoHooks,
 	commit.FindBaseCommitForFixup,
+	commit.FindBaseCommitForFixupDisregardFixupsForSameBaseCommit,
 	commit.FindBaseCommitForFixupDisregardMainBranch,
 	commit.FindBaseCommitForFixupOnlyAddedLines,
 	commit.FindBaseCommitForFixupWarningForAddedLines,