diff options
author | Stefan Haller <stefan@haller-berlin.de> | 2024-06-10 12:03:22 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-06-10 12:03:22 +0200 |
commit | 906d21f3cea91d5a11ce06630804aae99115a74c (patch) | |
tree | 5da2fd997128a5098f50a092fedc083495a274b5 /pkg | |
parent | 6cb2ac6fcce09830cd426e49a2c5f6b79db31a32 (diff) | |
parent | 7780f1264ac34e32b5a52bed667feaa0a67fa8d2 (diff) |
Improve "Find base commit for fixup" command when there are changes for master commits (#3645)
- **PR Description**
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.
- **Please check if the PR fulfills these requirements**
* [x] Cheatsheets are up-to-date (run `go generate ./...`)
* [x] Code has been formatted (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting))
* [x] Tests have been added/updated (see
[here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md)
for the integration test guide)
* [ ] Text is internationalised (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation))
* [ ] Docs have been updated if necessary
* [x] You've read through your own file changes for silly mistakes etc
Diffstat (limited to 'pkg')
-rw-r--r-- | pkg/gui/controllers/helpers/fixup_helper.go | 77 | ||||
-rw-r--r-- | pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go | 47 | ||||
-rw-r--r-- | pkg/integration/tests/test_list.go | 1 |
3 files changed, 102 insertions, 23 deletions
diff --git a/pkg/gui/controllers/helpers/fixup_helper.go b/pkg/gui/controllers/helpers/fixup_helper.go index 7177398bb..9cb951408 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) } @@ -70,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 } @@ -81,21 +124,9 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error { return fmt.Errorf("%s\n\n%s", message, subjects) } - commit, index, ok := self.findCommit(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 - // 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 { @@ -225,7 +256,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 +319,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 +337,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 }) } 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..aea9074e6 --- /dev/null +++ b/pkg/integration/tests/commit/find_base_commit_for_fixup_disregard_main_branch.go @@ -0,0 +1,47 @@ +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) + + t.Views().Commits(). + IsFocused(). + Lines( + Contains("4th commit"), + Contains("3rd commit").IsSelected(), + Contains("2nd commit"), + Contains("1st commit"), + ) + }, +}) 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, |