From fd8ce7d779902de3db5b5c9886ef62ed08cae8aa Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 24 Feb 2024 15:28:11 +0100 Subject: [PATCH 1/3] Add test demonstrating the current (undesired) behavior --- .../dont_show_branch_heads_for_todo_items.go | 56 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 2 files changed, 57 insertions(+) create mode 100644 pkg/integration/tests/interactive_rebase/dont_show_branch_heads_for_todo_items.go diff --git a/pkg/integration/tests/interactive_rebase/dont_show_branch_heads_for_todo_items.go b/pkg/integration/tests/interactive_rebase/dont_show_branch_heads_for_todo_items.go new file mode 100644 index 000000000..7c7037188 --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/dont_show_branch_heads_for_todo_items.go @@ -0,0 +1,56 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var DontShowBranchHeadsForTodoItems = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Check that branch heads are shown for normal commits during interactive rebase, but not for todo items", + ExtraCmdArgs: []string{}, + Skip: false, + GitVersion: AtLeast("2.38.0"), + SetupConfig: func(config *config.AppConfig) { + config.AppState.GitLogShowGraph = "never" + }, + SetupRepo: func(shell *Shell) { + shell. + NewBranch("branch1"). + CreateNCommits(2). + NewBranch("branch2"). + CreateNCommitsStartingAt(4, 3). + NewBranch("branch3"). + CreateNCommitsStartingAt(3, 7) + + shell.SetConfig("rebase.updateRefs", "true") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("CI commit 09"), + Contains("CI commit 08"), + Contains("CI commit 07"), + Contains("CI * commit 06"), + Contains("CI commit 05"), + Contains("CI commit 04"), + Contains("CI commit 03"), + Contains("CI * commit 02"), + Contains("CI commit 01"), + ). + NavigateToLine(Contains("commit 04")). + Press(keys.Universal.Edit). + Lines( + Contains("pick").Contains("CI commit 09"), + Contains("pick").Contains("CI commit 08"), + Contains("pick").Contains("CI commit 07"), + Contains("update-ref").Contains("branch2"), + Contains("pick").Contains("CI * commit 06"), // the star is undesired here; it's confusing because of the update-ref right above + Contains("pick").Contains("CI commit 05"), + Contains("CI <-- YOU ARE HERE --- commit 04"), + Contains("CI commit 03"), + Contains("CI * commit 02"), // this star is fine though + Contains("CI commit 01"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index e4efef40a..e26a0731f 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -162,6 +162,7 @@ var tests = []*components.IntegrationTest{ interactive_rebase.AmendHeadCommitDuringRebase, interactive_rebase.AmendMerge, interactive_rebase.AmendNonHeadCommitDuringRebase, + interactive_rebase.DontShowBranchHeadsForTodoItems, interactive_rebase.DropTodoCommitWithUpdateRef, interactive_rebase.DropWithCustomCommentChar, interactive_rebase.EditFirstCommit, From 418b316fabf394e14a7c1153f514a52bee48ec39 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 24 Feb 2024 12:27:27 +0100 Subject: [PATCH 2/3] Rename showBranchMarkerForHeadCommit parameter to hasRebaseUpdateRefsConfig Name it after what it is rather than what it is used for. In the next commit we will use it for another condition. --- pkg/gui/context/local_commits_context.go | 4 ++-- pkg/gui/context/sub_commits_context.go | 4 ++-- pkg/gui/presentation/commits.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/gui/context/local_commits_context.go b/pkg/gui/context/local_commits_context.go index cc166a8ec..604d51205 100644 --- a/pkg/gui/context/local_commits_context.go +++ b/pkg/gui/context/local_commits_context.go @@ -38,14 +38,14 @@ func NewLocalCommitsContext(c *ContextCommon) *LocalCommitsContext { } showYouAreHereLabel := c.Model().WorkingTreeStateAtLastCommitRefresh == enums.REBASE_MODE_REBASING - showBranchMarkerForHeadCommit := c.Git().Config.GetRebaseUpdateRefs() + hasRebaseUpdateRefsConfig := c.Git().Config.GetRebaseUpdateRefs() return presentation.GetCommitListDisplayStrings( c.Common, c.Model().Commits, c.Model().Branches, c.Model().CheckedOutBranch, - showBranchMarkerForHeadCommit, + hasRebaseUpdateRefsConfig, c.State().GetRepoState().GetScreenMode() != types.SCREEN_NORMAL, c.Modes().CherryPicking.SelectedShaSet(), c.Modes().Diffing.Ref, diff --git a/pkg/gui/context/sub_commits_context.go b/pkg/gui/context/sub_commits_context.go index 7a797e61d..d98188b91 100644 --- a/pkg/gui/context/sub_commits_context.go +++ b/pkg/gui/context/sub_commits_context.go @@ -55,13 +55,13 @@ func NewSubCommitsContext( if viewModel.GetShowBranchHeads() { branches = c.Model().Branches } - showBranchMarkerForHeadCommit := c.Git().Config.GetRebaseUpdateRefs() + hasRebaseUpdateRefsConfig := c.Git().Config.GetRebaseUpdateRefs() return presentation.GetCommitListDisplayStrings( c.Common, c.Model().SubCommits, branches, viewModel.GetRef().RefName(), - showBranchMarkerForHeadCommit, + hasRebaseUpdateRefsConfig, c.State().GetRepoState().GetScreenMode() != types.SCREEN_NORMAL, c.Modes().CherryPicking.SelectedShaSet(), c.Modes().Diffing.Ref, diff --git a/pkg/gui/presentation/commits.go b/pkg/gui/presentation/commits.go index ac1006c3d..97f7c801d 100644 --- a/pkg/gui/presentation/commits.go +++ b/pkg/gui/presentation/commits.go @@ -41,7 +41,7 @@ func GetCommitListDisplayStrings( commits []*models.Commit, branches []*models.Branch, currentBranchName string, - showBranchMarkerForHeadCommit bool, + hasRebaseUpdateRefsConfig bool, fullDescription bool, cherryPickedCommitShaSet *set.Set[string], diffName string, @@ -123,7 +123,7 @@ func GetCommitListDisplayStrings( !lo.Contains(common.UserConfig.Git.MainBranches, b.Name) && // Don't show a marker for the head commit unless the // rebase.updateRefs config is on - (showBranchMarkerForHeadCommit || b.CommitHash != commits[0].Sha) + (hasRebaseUpdateRefsConfig || b.CommitHash != commits[0].Sha) })) lines := make([][]string, 0, len(filteredCommits)) From 416a40b0e69605399bde4b03a0ed48977da1dae8 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 24 Feb 2024 12:38:49 +0100 Subject: [PATCH 3/3] Don't show branch head on rebase todos if the rebase.updateRefs config is on The additional branch head icon is more confusing than useful in this situation. The update-ref entries show very clearly where the branch heads will go when continuing the rebase; the information where the branch heads used to be before the rebase is not really needed here, and just makes the display more confusing. I'm not adding more tests here because the changes to the existing tests demonstrate the change clearly enough. --- pkg/gui/presentation/commits.go | 8 +++++++- .../dont_show_branch_heads_for_todo_items.go | 2 +- .../drop_todo_commit_with_update_ref.go | 2 +- .../interactive_rebase/quick_start_keep_selection.go | 2 +- .../quick_start_keep_selection_range.go | 4 ++-- .../interactive_rebase/view_files_of_todo_entries.go | 2 +- 6 files changed, 13 insertions(+), 7 deletions(-) diff --git a/pkg/gui/presentation/commits.go b/pkg/gui/presentation/commits.go index 97f7c801d..bd4057e41 100644 --- a/pkg/gui/presentation/commits.go +++ b/pkg/gui/presentation/commits.go @@ -145,6 +145,7 @@ func GetCommitListDisplayStrings( common, commit, branchHeadsToVisualize, + hasRebaseUpdateRefsConfig, cherryPickedCommitShaSet, isMarkedBaseCommit, willBeRebased, @@ -296,6 +297,7 @@ func displayCommit( common *common.Common, commit *models.Commit, branchHeadsToVisualize *set.Set[string], + hasRebaseUpdateRefsConfig bool, cherryPickedCommitShaSet *set.Set[string], isMarkedBaseCommit bool, willBeRebased bool, @@ -329,7 +331,11 @@ func displayCommit( tagString = theme.DiffTerminalColor.SetBold().Sprint(strings.Join(commit.Tags, " ")) + " " } - if branchHeadsToVisualize.Includes(commit.Sha) && commit.Status != models.StatusMerged { + if branchHeadsToVisualize.Includes(commit.Sha) && + // Don't show branch head on commits that are already merged to a main branch + commit.Status != models.StatusMerged && + // Don't show branch head on a "pick" todo if the rebase.updateRefs config is on + !(commit.IsTODO() && hasRebaseUpdateRefsConfig) { tagString = style.FgCyan.SetBold().Sprint( lo.Ternary(icons.IsIconEnabled(), icons.BRANCH_ICON, "*") + " " + tagString) } diff --git a/pkg/integration/tests/interactive_rebase/dont_show_branch_heads_for_todo_items.go b/pkg/integration/tests/interactive_rebase/dont_show_branch_heads_for_todo_items.go index 7c7037188..7b433fa17 100644 --- a/pkg/integration/tests/interactive_rebase/dont_show_branch_heads_for_todo_items.go +++ b/pkg/integration/tests/interactive_rebase/dont_show_branch_heads_for_todo_items.go @@ -45,7 +45,7 @@ var DontShowBranchHeadsForTodoItems = NewIntegrationTest(NewIntegrationTestArgs{ Contains("pick").Contains("CI commit 08"), Contains("pick").Contains("CI commit 07"), Contains("update-ref").Contains("branch2"), - Contains("pick").Contains("CI * commit 06"), // the star is undesired here; it's confusing because of the update-ref right above + Contains("pick").Contains("CI commit 06"), // no star on this entry, even though branch2 points to it Contains("pick").Contains("CI commit 05"), Contains("CI <-- YOU ARE HERE --- commit 04"), Contains("CI commit 03"), diff --git a/pkg/integration/tests/interactive_rebase/drop_todo_commit_with_update_ref.go b/pkg/integration/tests/interactive_rebase/drop_todo_commit_with_update_ref.go index afc0fd073..94e851efa 100644 --- a/pkg/integration/tests/interactive_rebase/drop_todo_commit_with_update_ref.go +++ b/pkg/integration/tests/interactive_rebase/drop_todo_commit_with_update_ref.go @@ -44,7 +44,7 @@ var DropTodoCommitWithUpdateRef = NewIntegrationTest(NewIntegrationTestArgs{ Contains("pick").Contains("CI commit 06"), Contains("pick").Contains("CI commit 05"), Contains("update-ref").Contains("branch1").DoesNotContain("*"), - Contains("pick").Contains("CI * commit 04"), + Contains("pick").Contains("CI commit 04"), Contains("pick").Contains("CI commit 03"), Contains("<-- YOU ARE HERE --- commit 02").IsSelected(), Contains("CI commit 01"), diff --git a/pkg/integration/tests/interactive_rebase/quick_start_keep_selection.go b/pkg/integration/tests/interactive_rebase/quick_start_keep_selection.go index 2216b89b7..ba694b222 100644 --- a/pkg/integration/tests/interactive_rebase/quick_start_keep_selection.go +++ b/pkg/integration/tests/interactive_rebase/quick_start_keep_selection.go @@ -43,7 +43,7 @@ var QuickStartKeepSelection = NewIntegrationTest(NewIntegrationTestArgs{ Contains("pick").Contains("CI commit 06"), Contains("pick").Contains("CI commit 05"), Contains("update-ref").Contains("branch1"), - Contains("pick").Contains("CI * commit 04"), + Contains("pick").Contains("CI commit 04"), Contains("pick").Contains("CI commit 03"), Contains("CI commit 02").IsSelected(), Contains("CI <-- YOU ARE HERE --- commit 01"), diff --git a/pkg/integration/tests/interactive_rebase/quick_start_keep_selection_range.go b/pkg/integration/tests/interactive_rebase/quick_start_keep_selection_range.go index 20005ba6b..ea863e408 100644 --- a/pkg/integration/tests/interactive_rebase/quick_start_keep_selection_range.go +++ b/pkg/integration/tests/interactive_rebase/quick_start_keep_selection_range.go @@ -46,10 +46,10 @@ var QuickStartKeepSelectionRange = NewIntegrationTest(NewIntegrationTestArgs{ Contains("CI commit 07"), Contains("CI commit 06"), Contains("update-ref").Contains("branch2"), - Contains("CI * commit 05"), + Contains("CI commit 05"), Contains("CI commit 04").IsSelected(), Contains("update-ref").Contains("branch1").IsSelected(), - Contains("CI * commit 03").IsSelected(), + Contains("CI commit 03").IsSelected(), Contains("CI commit 02").IsSelected(), Contains("CI <-- YOU ARE HERE --- commit 01"), ) diff --git a/pkg/integration/tests/interactive_rebase/view_files_of_todo_entries.go b/pkg/integration/tests/interactive_rebase/view_files_of_todo_entries.go index 1b35abaaf..837c418d0 100644 --- a/pkg/integration/tests/interactive_rebase/view_files_of_todo_entries.go +++ b/pkg/integration/tests/interactive_rebase/view_files_of_todo_entries.go @@ -30,7 +30,7 @@ var ViewFilesOfTodoEntries = NewIntegrationTest(NewIntegrationTestArgs{ Lines( Contains("pick").Contains("CI commit 03").IsSelected(), Contains("update-ref").Contains("branch1"), - Contains("pick").Contains("CI * commit 02"), + Contains("pick").Contains("CI commit 02"), Contains("CI <-- YOU ARE HERE --- commit 01"), ). Press(keys.Universal.GoInto)