diff --git a/pkg/gui/controllers/helpers/fixup_helper.go b/pkg/gui/controllers/helpers/fixup_helper.go index c670b9dfb..dfde8365b 100644 --- a/pkg/gui/controllers/helpers/fixup_helper.go +++ b/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 := 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, @@ -161,13 +165,35 @@ func getCommitsForHashes(commits []*models.Commit, hashes []string) []*models.Co return result } -func getHashesAndSubjects(commits []*models.Commit, hashes []string) string { - subjects := lo.Map(getCommitsForHashes(commits, hashes), func(c *models.Commit, _ int) string { +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", "--"} diff --git a/pkg/gui/controllers/helpers/fixup_helper_test.go b/pkg/gui/controllers/helpers/fixup_helper_test.go index e281a4c28..466e90087 100644 --- a/pkg/gui/controllers/helpers/fixup_helper_test.go +++ b/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" ) @@ -201,3 +204,138 @@ func TestFixupHelper_IsFixupCommit(t *testing.T) { }) } } + +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) + }) + } +} diff --git a/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_fixups_for_same_base_commit.go b/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_fixups_for_same_base_commit.go index 07f32126a..94b579190 100644 --- a/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_fixups_for_same_base_commit.go +++ b/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_fixups_for_same_base_commit.go @@ -34,13 +34,13 @@ var FindBaseCommitForFixupDisregardFixupsForSameBaseCommit = NewIntegrationTest( Focus(). Press(keys.Files.FindBaseCommitForFixup) - t.ExpectPopup().Alert(). - Title(Equals("Error")). - Content( - MatchesRegexp("Multiple base commits found.*\n\n" + - ".*fixup! 2nd commit\n" + - ".*2nd commit"), - ). - Confirm() + t.Views().Commits(). + IsFocused(). + Lines( + Contains("3rd commit"), + Contains("fixup! 2nd commit"), + Contains("2nd commit").IsSelected(), + Contains("1st commit"), + ) }, })