From 18903a7dde07a57a023649bf1eb87d3ce77b6347 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 29 Jul 2023 20:36:16 +0200 Subject: [PATCH 1/3] Make setCommitMergedStatuses a non-member function It doesn't depend on anything in CommitLoader, so it can be free-standing, and that makes it easier to test (see next commit). --- pkg/commands/git_commands/commit_loader.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index f138adfaf..22b92ea0a 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -135,7 +135,7 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit, } if ancestor != "" { - commits = self.setCommitMergedStatuses(ancestor, commits) + commits = setCommitMergedStatuses(ancestor, commits) } return commits, nil @@ -492,7 +492,7 @@ func (self *CommitLoader) commitFromPatch(content string) *models.Commit { } } -func (self *CommitLoader) setCommitMergedStatuses(ancestor string, commits []*models.Commit) []*models.Commit { +func setCommitMergedStatuses(ancestor string, commits []*models.Commit) []*models.Commit { passedAncestor := false for i, commit := range commits { if strings.HasPrefix(ancestor, commit.Sha) { From 774df817fd7b3f8c1a08a41ad5d23b08ed24f0b0 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 29 Jul 2023 20:37:05 +0200 Subject: [PATCH 2/3] Add tests for setCommitMergedStatuses The test for update-ref shows demonstrates a problem. See next commit for the fix. --- .../git_commands/commit_loader_test.go | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index bcbc33c02..a281cb775 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -506,3 +506,50 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { }) } } + +func TestCommitLoader_setCommitMergedStatuses(t *testing.T) { + type scenario struct { + testName string + commits []*models.Commit + ancestor string + expectedCommits []*models.Commit + } + + scenarios := []scenario{ + { + testName: "basic", + commits: []*models.Commit{ + {Sha: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed}, + {Sha: "67890", Name: "2", Action: models.ActionNone, Status: models.StatusPushed}, + {Sha: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusPushed}, + }, + ancestor: "67890", + expectedCommits: []*models.Commit{ + {Sha: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed}, + {Sha: "67890", Name: "2", Action: models.ActionNone, Status: models.StatusMerged}, + {Sha: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusMerged}, + }, + }, + { + testName: "with update-ref", + commits: []*models.Commit{ + {Sha: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed}, + {Sha: "", Name: "", Action: todo.UpdateRef, Status: models.StatusNone}, + {Sha: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusPushed}, + }, + ancestor: "deadbeef", + expectedCommits: []*models.Commit{ + {Sha: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed}, + {Sha: "", Name: "", Action: todo.UpdateRef, Status: models.StatusNone}, + {Sha: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusMerged}, // Wrong, expect Pushed + }, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.testName, func(t *testing.T) { + expectedCommits := setCommitMergedStatuses(scenario.ancestor, scenario.commits) + assert.Equal(t, scenario.expectedCommits, expectedCommits) + }) + } +} From 8ab05d6834dc1e3bf9b270f9faad8907e99f507c Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 29 Jul 2023 20:15:00 +0200 Subject: [PATCH 3/3] Fix merge status of commits when update-ref command is present Update-ref commands have an empty sha, and strings.HasPrefix returns true when called with an empty second argument, so whenever an update-ref command is present in a rebase, all commits from there on down were drawn with a green sha. --- pkg/commands/git_commands/commit_loader.go | 3 ++- pkg/commands/git_commands/commit_loader_test.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 22b92ea0a..ab13e6523 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -495,7 +495,8 @@ func (self *CommitLoader) commitFromPatch(content string) *models.Commit { func setCommitMergedStatuses(ancestor string, commits []*models.Commit) []*models.Commit { passedAncestor := false for i, commit := range commits { - if strings.HasPrefix(ancestor, commit.Sha) { + // some commits aren't really commits and don't have sha's, such as the update-ref todo + if commit.Sha != "" && strings.HasPrefix(ancestor, commit.Sha) { passedAncestor = true } if commit.Status != models.StatusPushed && commit.Status != models.StatusUnpushed { diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index a281cb775..c3cfb0585 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -541,7 +541,7 @@ func TestCommitLoader_setCommitMergedStatuses(t *testing.T) { expectedCommits: []*models.Commit{ {Sha: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed}, {Sha: "", Name: "", Action: todo.UpdateRef, Status: models.StatusNone}, - {Sha: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusMerged}, // Wrong, expect Pushed + {Sha: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusPushed}, }, }, }