From f9ba2dac9d161d73b5c308a94aa1e03fb00df826 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 7 Jun 2024 18:13:02 +0200 Subject: [PATCH 1/3] Add test demonstrating the desired behavior It has two modified hunks, one for a master commit and one for a branch commit. Currently we get an error mentioning those two commits, but we would like to silently select the branch commit. --- ..._commit_for_fixup_disregard_main_branch.go | 56 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 2 files changed, 57 insertions(+) create mode 100644 pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go diff --git a/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go b/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go new file mode 100644 index 000000000..4b8937eb2 --- /dev/null +++ b/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go @@ -0,0 +1,56 @@ +package commit + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var FindBaseCommitForFixupDisregardMainBranch = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Finds the base commit to create a fixup for, disregarding changes to a commit that is already on master", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell. + EmptyCommit("1st commit"). + CreateFileAndAdd("file1", "file1 content\n"). + Commit("2nd commit"). + NewBranch("mybranch"). + CreateFileAndAdd("file2", "file2 content\n"). + Commit("3rd commit"). + EmptyCommit("4th commit"). + UpdateFile("file1", "file1 changed content"). + UpdateFile("file2", "file2 changed content") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Lines( + Contains("4th commit").IsSelected(), + Contains("3rd commit"), + Contains("2nd commit"), + Contains("1st commit"), + ) + + t.Views().Files(). + Focus(). + Press(keys.Files.FindBaseCommitForFixup) + + /* EXPECTED: + t.Views().Commits(). + IsFocused(). + Lines( + Contains("4th commit"), + Contains("3rd commit").IsSelected(), + Contains("2nd commit"), + Contains("1st commit"), + ) + */ + + // ACTUAL: + t.ExpectPopup().Alert().Title(Equals("Error")).Content( + Contains("Multiple base commits found."). + Contains("2nd commit"). + Contains("3rd commit"), + ).Confirm() + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 6386146ba..ea2b93f45 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -82,6 +82,7 @@ var tests = []*components.IntegrationTest{ commit.CreateTag, commit.DiscardOldFileChanges, commit.FindBaseCommitForFixup, + commit.FindBaseCommitForFixupDisregardMainBranch, commit.FindBaseCommitForFixupOnlyAddedLines, commit.FindBaseCommitForFixupWarningForAddedLines, commit.Highlight, From f3718ddfb0fbb97951092b2c3470ab2fdc0ac57f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 7 Jun 2024 17:31:44 +0200 Subject: [PATCH 2/3] Don't reference Model().Commits multiple times Copy the slice into a variable and use that throughout the whole operation; this makes us a little more robust against the model refreshing concurrently. --- pkg/gui/controllers/helpers/fixup_helper.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/gui/controllers/helpers/fixup_helper.go b/pkg/gui/controllers/helpers/fixup_helper.go index 7177398bb..c3ef28af5 100644 --- a/pkg/gui/controllers/helpers/fixup_helper.go +++ b/pkg/gui/controllers/helpers/fixup_helper.go @@ -50,6 +50,8 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error { deletedLineHunks, addedLineHunks := parseDiff(diff) + commits := self.c.Model().Commits + var hashes []string warnAboutAddedLines := false @@ -57,7 +59,7 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error { hashes, err = self.blameDeletedLines(deletedLineHunks) warnAboutAddedLines = len(addedLineHunks) > 0 } else if len(addedLineHunks) > 0 { - hashes, err = self.blameAddedLines(addedLineHunks) + hashes, err = self.blameAddedLines(commits, addedLineHunks) } else { return errors.New(self.c.Tr.NoChangedFiles) } @@ -81,9 +83,8 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error { return fmt.Errorf("%s\n\n%s", message, subjects) } - commit, index, ok := self.findCommit(hashes[0]) + commit, index, ok := self.findCommit(commits, hashes[0]) if !ok { - commits := self.c.Model().Commits if commits[len(commits)-1].Status == models.StatusMerged { // If the commit is not found, it's most likely because it's already // merged, and more than 300 commits away. Check if the last known @@ -225,7 +226,7 @@ func (self *FixupHelper) blameDeletedLines(deletedLineHunks []*hunk) ([]string, return result.ToSlice(), errg.Wait() } -func (self *FixupHelper) blameAddedLines(addedLineHunks []*hunk) ([]string, error) { +func (self *FixupHelper) blameAddedLines(commits []*models.Commit, addedLineHunks []*hunk) ([]string, error) { errg := errgroup.Group{} hashesChan := make(chan []string) @@ -288,8 +289,8 @@ func (self *FixupHelper) blameAddedLines(addedLineHunks []*hunk) ([]string, erro if hashes[0] == hashes[1] { result.Add(hashes[0]) } else { - _, index1, ok1 := self.findCommit(hashes[0]) - _, index2, ok2 := self.findCommit(hashes[1]) + _, index1, ok1 := self.findCommit(commits, hashes[0]) + _, index2, ok2 := self.findCommit(commits, hashes[1]) if ok1 && ok2 { result.Add(lo.Ternary(index1 < index2, hashes[0], hashes[1])) } else if ok1 { @@ -306,8 +307,8 @@ func (self *FixupHelper) blameAddedLines(addedLineHunks []*hunk) ([]string, erro return result.ToSlice(), errg.Wait() } -func (self *FixupHelper) findCommit(hash string) (*models.Commit, int, bool) { - return lo.FindIndexOf(self.c.Model().Commits, func(commit *models.Commit) bool { +func (self *FixupHelper) findCommit(commits []*models.Commit, hash string) (*models.Commit, int, bool) { + return lo.FindIndexOf(commits, func(commit *models.Commit) bool { return commit.Hash == hash }) } From 7780f1264ac34e32b5a52bed667feaa0a67fa8d2 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 7 Jun 2024 17:55:21 +0200 Subject: [PATCH 3/3] Disregard master commits when finding base commit for fixup If exactly one candidate from inside the current branch is found, we return that one even if there are also hunks belonging to master commits; we disregard those in this case. --- pkg/gui/controllers/helpers/fixup_helper.go | 62 ++++++++++++++----- ..._commit_for_fixup_disregard_main_branch.go | 9 --- 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/pkg/gui/controllers/helpers/fixup_helper.go b/pkg/gui/controllers/helpers/fixup_helper.go index c3ef28af5..9cb951408 100644 --- a/pkg/gui/controllers/helpers/fixup_helper.go +++ b/pkg/gui/controllers/helpers/fixup_helper.go @@ -72,8 +72,49 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error { // This should never happen return errors.New(self.c.Tr.NoBaseCommitsFound) } - if len(hashes) > 1 { - subjects, err := self.c.Git().Commit.GetHashesAndCommitMessagesFirstLine(hashes) + + // If a commit can't be found, and the last known commit is already merged, + // we know that the commit we're looking for is also merged. Otherwise we + // can't tell. + notFoundMeansMerged := len(commits) > 0 && commits[len(commits)-1].Status == models.StatusMerged + + const ( + MERGED int = iota + NOT_MERGED + CANNOT_TELL + ) + + // Group the hashes into buckets by merged status + hashGroups := lo.GroupBy(hashes, func(hash string) int { + commit, _, ok := self.findCommit(commits, hash) + if ok { + return lo.Ternary(commit.Status == models.StatusMerged, MERGED, NOT_MERGED) + } + return lo.Ternary(notFoundMeansMerged, MERGED, CANNOT_TELL) + }) + + if len(hashGroups[CANNOT_TELL]) > 0 { + // If we have any commits that we can't tell if they're merged, just + // show the generic "not in current view" error. This can only happen if + // a feature branch has more than 300 commits, or there is no main + // branch. Both are so unlikely that we don't bother returning a more + // detailed error message (e.g. we could say something about the commits + // that *are* in the current branch, but it's not worth it). + return errors.New(self.c.Tr.BaseCommitIsNotInCurrentView) + } + + if len(hashGroups[NOT_MERGED]) == 0 { + // If all the commits are merged, show the "already on main branch" + // error. It isn't worth doing a detailed report of which commits we + // found. + 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 + // them in the error message. But only the candidates from the current + // branch, not including any that are already merged. + subjects, err := self.c.Git().Commit.GetHashesAndCommitMessagesFirstLine(hashGroups[NOT_MERGED]) if err != nil { return err } @@ -83,20 +124,9 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error { return fmt.Errorf("%s\n\n%s", message, subjects) } - commit, index, ok := self.findCommit(commits, hashes[0]) - if !ok { - if commits[len(commits)-1].Status == models.StatusMerged { - // If the commit is not found, it's most likely because it's already - // merged, and more than 300 commits away. Check if the last known - // commit is already merged; if so, show the "already merged" error. - return errors.New(self.c.Tr.BaseCommitIsAlreadyOnMainBranch) - } - // If we get here, the current branch must have more then 300 commits. Unlikely... - return errors.New(self.c.Tr.BaseCommitIsNotInCurrentView) - } - if commit.Status == models.StatusMerged { - return errors.New(self.c.Tr.BaseCommitIsAlreadyOnMainBranch) - } + // 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]) doIt := func() error { if !hasStagedChanges { diff --git a/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go b/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go index 4b8937eb2..aea9074e6 100644 --- a/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go +++ b/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go @@ -35,7 +35,6 @@ var FindBaseCommitForFixupDisregardMainBranch = NewIntegrationTest(NewIntegratio Focus(). Press(keys.Files.FindBaseCommitForFixup) - /* EXPECTED: t.Views().Commits(). IsFocused(). Lines( @@ -44,13 +43,5 @@ var FindBaseCommitForFixupDisregardMainBranch = NewIntegrationTest(NewIntegratio Contains("2nd commit"), Contains("1st commit"), ) - */ - - // ACTUAL: - t.ExpectPopup().Alert().Title(Equals("Error")).Content( - Contains("Multiple base commits found."). - Contains("2nd commit"). - Contains("3rd commit"), - ).Confirm() }, })