From 93d19db158556cad6b0051317b3d042f5bebd429 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 22 Aug 2023 14:06:29 +0200 Subject: [PATCH 1/4] Add assertion to show the problem --- .../interactive_rebase/drop_todo_commit_with_update_ref.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3fa221d72..b1eb20ba4 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 05"), Contains("update-ref").Contains("branch1").DoesNotContain("*"), Contains("pick").Contains("CI * commit 04"), - Contains("pick").Contains("CI commit 03"), + Contains("pick").Contains("CI commit 03").IsSelected(), // wrong line selected Contains("<-- YOU ARE HERE --- commit 02"), Contains("CI commit 01"), ). From d74c817fd88144278118cf86465b1e58ad06633a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 22 Aug 2023 13:55:05 +0200 Subject: [PATCH 2/4] Panic when trying to use RefreshOptions.Then with mode ASYNC This doesn't work, and since it took me a while of debugging to figure this out, alert other developers earlier when they try to do it. --- pkg/gui/controllers/helpers/refresh_helper.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 377319250..9e1489da4 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -51,6 +51,10 @@ func NewRefreshHelper( } func (self *RefreshHelper) Refresh(options types.RefreshOptions) error { + if options.Mode == types.ASYNC && options.Then != nil { + panic("RefreshOptions.Then doesn't work with mode ASYNC") + } + t := time.Now() defer func() { self.c.Log.Infof(fmt.Sprintf("Refresh took %s", time.Since(t))) From c718a73d0a248c3d62227d065dfe4d3f2da83b3c Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 22 Aug 2023 12:08:41 +0200 Subject: [PATCH 3/4] Call Then function only after everything is done I'm actually surprised how this could even have worked before. --- pkg/gui/controllers/helpers/refresh_helper.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 9e1489da4..0fe8884c1 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -178,11 +178,11 @@ func (self *RefreshHelper) Refresh(options types.RefreshOptions) error { self.refreshStatus() + wg.Wait() + if options.Then != nil { options.Then() } - - wg.Wait() } if options.Mode == types.BLOCK_UI { From 98e6c119f52fbcef9c3df5ff33247b7322937005 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 3 Aug 2023 19:22:51 +0200 Subject: [PATCH 4/4] Select same commit again after pressing "e" to edit a commit When editing a commit, the index of the current commit can change; for example, when merge commits are involved, or when working with stacked branches where "update-ref" commands may be added above the selected commit. Reselect the current commit after pressing "e"; this requires doing the refresh blocking on the main thread. (Another option that I considered was to use a SYNC refresh, and then select the new line with an OnUIThread inside the Then function. This also works, but results in a very visible lag.) --- .../controllers/helpers/merge_and_rebase_helper.go | 8 ++++++-- pkg/gui/controllers/local_commits_controller.go | 11 ++++++++++- .../drop_todo_commit_with_update_ref.go | 4 ++-- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go index 4c7b087aa..a88615271 100644 --- a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go +++ b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go @@ -125,8 +125,8 @@ func isMergeConflictErr(errStr string) bool { return false } -func (self *MergeAndRebaseHelper) CheckMergeOrRebase(result error) error { - if err := self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC}); err != nil { +func (self *MergeAndRebaseHelper) CheckMergeOrRebaseWithRefreshOptions(result error, refreshOptions types.RefreshOptions) error { + if err := self.c.Refresh(refreshOptions); err != nil { return err } if result == nil { @@ -143,6 +143,10 @@ func (self *MergeAndRebaseHelper) CheckMergeOrRebase(result error) error { } } +func (self *MergeAndRebaseHelper) CheckMergeOrRebase(result error) error { + return self.CheckMergeOrRebaseWithRefreshOptions(result, types.RefreshOptions{Mode: types.ASYNC}) +} + func (self *MergeAndRebaseHelper) CheckForConflicts(result error) error { if result == nil { return nil diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 38ddaa515..b3ef598d2 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -392,7 +392,16 @@ func (self *LocalCommitsController) edit(commit *models.Commit) error { return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(gocui.Task) error { self.c.LogAction(self.c.Tr.Actions.EditCommit) err := self.c.Git().Rebase.EditRebase(commit.Sha) - return self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err) + return self.c.Helpers().MergeAndRebase.CheckMergeOrRebaseWithRefreshOptions( + err, + types.RefreshOptions{Mode: types.BLOCK_UI, Then: func() { + _, index, ok := lo.FindIndexOf(self.c.Model().Commits, func(c *models.Commit) bool { + return c.Sha == commit.Sha + }) + if ok { + self.context().SetSelectedLineIdx(index) + } + }}) }) } 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 b1eb20ba4..de085710a 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,8 +44,8 @@ var DropTodoCommitWithUpdateRef = NewIntegrationTest(NewIntegrationTestArgs{ Contains("pick").Contains("CI commit 05"), Contains("update-ref").Contains("branch1").DoesNotContain("*"), Contains("pick").Contains("CI * commit 04"), - Contains("pick").Contains("CI commit 03").IsSelected(), // wrong line selected - Contains("<-- YOU ARE HERE --- commit 02"), + Contains("pick").Contains("CI commit 03"), + Contains("<-- YOU ARE HERE --- commit 02").IsSelected(), Contains("CI commit 01"), ). NavigateToLine(Contains("commit 06")).