From 8d8bdb948b089250c22f3ac4f549152a209dcdb2 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 15 Jan 2022 13:29:28 +1100 Subject: [PATCH] avoid deadlock in merge panel --- pkg/gui/diff_context_size.go | 17 +++++- pkg/gui/diff_context_size_test.go | 8 ++- pkg/gui/files_panel.go | 12 ++-- pkg/gui/menu_panel.go | 2 +- pkg/gui/merge_panel.go | 94 +++++++++++++++++-------------- pkg/i18n/chinese.go | 1 - pkg/i18n/dutch.go | 1 - pkg/i18n/english.go | 2 - pkg/i18n/polish.go | 1 - 9 files changed, 79 insertions(+), 59 deletions(-) diff --git a/pkg/gui/diff_context_size.go b/pkg/gui/diff_context_size.go index 68a7bf14b..ff7786c8a 100644 --- a/pkg/gui/diff_context_size.go +++ b/pkg/gui/diff_context_size.go @@ -4,10 +4,25 @@ import ( "errors" ) +var CONTEXT_KEYS_SHOWING_DIFFS = []ContextKey{ + FILES_CONTEXT_KEY, + COMMIT_FILES_CONTEXT_KEY, + STASH_CONTEXT_KEY, + BRANCH_COMMITS_CONTEXT_KEY, + SUB_COMMITS_CONTEXT_KEY, + MAIN_STAGING_CONTEXT_KEY, + MAIN_PATCH_BUILDING_CONTEXT_KEY, +} + func isShowingDiff(gui *Gui) bool { key := gui.currentStaticContext().GetKey() - return key == FILES_CONTEXT_KEY || key == COMMIT_FILES_CONTEXT_KEY || key == STASH_CONTEXT_KEY || key == BRANCH_COMMITS_CONTEXT_KEY || key == SUB_COMMITS_CONTEXT_KEY || key == MAIN_STAGING_CONTEXT_KEY || key == MAIN_PATCH_BUILDING_CONTEXT_KEY + for _, contextKey := range CONTEXT_KEYS_SHOWING_DIFFS { + if key == contextKey { + return true + } + } + return false } func (gui *Gui) IncreaseContextInDiffView() error { diff --git a/pkg/gui/diff_context_size_test.go b/pkg/gui/diff_context_size_test.go index 11bd0712b..b459e40d0 100644 --- a/pkg/gui/diff_context_size_test.go +++ b/pkg/gui/diff_context_size_test.go @@ -65,7 +65,9 @@ func TestDoesntIncreaseContextInDiffViewInContextWithoutDiff(t *testing.T) { func(gui *Gui) Context { return gui.State.Contexts.ReflogCommits }, func(gui *Gui) Context { return gui.State.Contexts.RemoteBranches }, func(gui *Gui) Context { return gui.State.Contexts.Tags }, - func(gui *Gui) Context { return gui.State.Contexts.Merging }, + // not testing this because it will kick straight back to the files context + // upon pushing the context + // func(gui *Gui) Context { return gui.State.Contexts.Merging }, func(gui *Gui) Context { return gui.State.Contexts.CommandLog }, } @@ -115,7 +117,9 @@ func TestDoesntDecreaseContextInDiffViewInContextWithoutDiff(t *testing.T) { func(gui *Gui) Context { return gui.State.Contexts.ReflogCommits }, func(gui *Gui) Context { return gui.State.Contexts.RemoteBranches }, func(gui *Gui) Context { return gui.State.Contexts.Tags }, - func(gui *Gui) Context { return gui.State.Contexts.Merging }, + // not testing this because it will kick straight back to the files context + // upon pushing the context + // func(gui *Gui) Context { return gui.State.Contexts.Merging }, func(gui *Gui) Context { return gui.State.Contexts.CommandLog }, } diff --git a/pkg/gui/files_panel.go b/pkg/gui/files_panel.go index 05bf8d4ed..d121c8275 100644 --- a/pkg/gui/files_panel.go +++ b/pkg/gui/files_panel.go @@ -54,7 +54,7 @@ func (gui *Gui) filesRenderToMain() error { } if node.File != nil && node.File.HasInlineMergeConflicts { - return gui.refreshMergePanelWithLock() + return gui.renderConflictsFromFilesPanel() } cmdObj := gui.Git.WorkingTree.WorktreeFileDiffCmdObj(node, false, !node.GetHasUnstagedChanges() && node.GetHasStagedChanges(), gui.State.IgnoreWhitespaceInDiffView) @@ -182,7 +182,7 @@ func (gui *Gui) enterFile(opts OnFocusOpts) error { } if file.HasInlineMergeConflicts { - return gui.handleSwitchToMerge() + return gui.switchToMerge() } if file.HasMergeConflicts { return gui.createErrorPanel(gui.Tr.FileStagingRequirements) @@ -201,7 +201,7 @@ func (gui *Gui) handleFilePress() error { file := node.File if file.HasInlineMergeConflicts { - return gui.handleSwitchToMerge() + return gui.switchToMerge() } if file.HasUnstagedChanges { @@ -880,16 +880,12 @@ func (gui *Gui) upstreamForBranchInConfig(branchName string) (string, string, er return "", "", nil } -func (gui *Gui) handleSwitchToMerge() error { +func (gui *Gui) switchToMerge() error { file := gui.getSelectedFile() if file == nil { return nil } - if !file.HasInlineMergeConflicts { - return gui.createErrorPanel(gui.Tr.FileNoMergeCons) - } - return gui.pushContext(gui.State.Contexts.Merging) } diff --git a/pkg/gui/menu_panel.go b/pkg/gui/menu_panel.go index be17d352d..b32b3bf44 100644 --- a/pkg/gui/menu_panel.go +++ b/pkg/gui/menu_panel.go @@ -39,7 +39,7 @@ func (gui *Gui) getMenuOptions() map[string]string { } func (gui *Gui) handleMenuClose() error { - return gui.returnFromContextSync() + return gui.returnFromContext() } type createMenuOptions struct { diff --git a/pkg/gui/merge_panel.go b/pkg/gui/merge_panel.go index 5b7502f64..942340286 100644 --- a/pkg/gui/merge_panel.go +++ b/pkg/gui/merge_panel.go @@ -90,6 +90,7 @@ func (gui *Gui) handlePickHunk() error { if err := gui.handleCompleteMerge(); err != nil { return err } + return nil } return gui.refreshMergePanel() }) @@ -151,11 +152,19 @@ func (gui *Gui) refreshMergePanelWithLock() error { return gui.withMergeConflictLock(gui.refreshMergePanel) } -func (gui *Gui) refreshMergePanel() error { - panelState := gui.State.Panels.Merging +// not re-using state here because we can run into issues with mutexes when +// doing that. +func (gui *Gui) renderConflictsFromFilesPanel() error { + state := mergeconflicts.NewState() + _, err := gui.renderConflicts(state, false) + + return err +} + +func (gui *Gui) renderConflicts(state *mergeconflicts.State, hasFocus bool) (bool, error) { cat, err := gui.catSelectedFile() if err != nil { - return gui.refreshMainViews(refreshMainOpts{ + return false, gui.refreshMainViews(refreshMainOpts{ main: &viewUpdateOpts{ title: "", task: NewRenderStringTask(err.Error()), @@ -163,20 +172,20 @@ func (gui *Gui) refreshMergePanel() error { }) } - panelState.SetConflictsFromCat(cat) + state.SetConflictsFromCat(cat) - if panelState.NoConflicts() { - return gui.handleCompleteMerge() + if state.NoConflicts() { + // we shouldn't end up here + return false, nil } - hasFocus := gui.currentViewName() == "main" - content := mergeconflicts.ColoredConflictFile(cat, panelState.State, hasFocus) + content := mergeconflicts.ColoredConflictFile(cat, state, hasFocus) - if err := gui.scrollToConflict(); err != nil { - return err + if !gui.State.Panels.Merging.UserVerticalScrolling { + gui.centerYPos(gui.Views.Main, state.GetConflictMiddle()) } - return gui.refreshMainViews(refreshMainOpts{ + return true, gui.refreshMainViews(refreshMainOpts{ main: &viewUpdateOpts{ title: gui.Tr.MergeConflictsTitle, task: NewRenderStringWithoutScrollTask(content), @@ -185,6 +194,19 @@ func (gui *Gui) refreshMergePanel() error { }) } +func (gui *Gui) refreshMergePanel() error { + conflictsFound, err := gui.renderConflicts(gui.State.Panels.Merging.State, true) + if err != nil { + return err + } + + if !conflictsFound { + return gui.handleCompleteMerge() + } + + return nil +} + func (gui *Gui) catSelectedFile() (string, error) { item := gui.getSelectedFile() if item == nil { @@ -203,21 +225,6 @@ func (gui *Gui) catSelectedFile() (string, error) { return cat, nil } -func (gui *Gui) scrollToConflict() error { - if gui.State.Panels.Merging.UserVerticalScrolling { - return nil - } - - panelState := gui.State.Panels.Merging - if panelState.NoConflicts() { - return nil - } - - gui.centerYPos(gui.Views.Main, panelState.GetConflictMiddle()) - - return nil -} - func (gui *Gui) centerYPos(view *gocui.View, y int) { ox, _ := view.Origin() _, height := view.Size() @@ -238,18 +245,11 @@ func (gui *Gui) getMergingOptions() map[string]string { } func (gui *Gui) handleEscapeMerge() error { - gui.takeOverMergeConflictScrolling() - - gui.State.Panels.Merging.Reset() if err := gui.refreshSidePanels(refreshOptions{scope: []RefreshableView{FILES}}); err != nil { return err } - // it's possible this method won't be called from the merging view so we need to - // ensure we only 'return' focus if we already have it - if gui.g.CurrentView() == gui.Views.Main { - return gui.pushContext(gui.State.Contexts.Files) - } - return nil + + return gui.escapeMerge() } func (gui *Gui) handleCompleteMerge() error { @@ -259,16 +259,26 @@ func (gui *Gui) handleCompleteMerge() error { if err := gui.refreshSidePanels(refreshOptions{scope: []RefreshableView{FILES}}); err != nil { return err } - // if we got conflicts after unstashing, we don't want to call any git - // commands to continue rebasing/merging here - if gui.Git.Status.WorkingTreeState() == enums.REBASE_MODE_NONE { - return gui.handleEscapeMerge() - } + // if there are no more files with merge conflicts, we should ask whether the user wants to continue - if !gui.anyFilesWithMergeConflicts() { + if gui.Git.Status.WorkingTreeState() != enums.REBASE_MODE_NONE && !gui.anyFilesWithMergeConflicts() { return gui.promptToContinueRebase() } - return gui.handleEscapeMerge() + + return gui.escapeMerge() +} + +func (gui *Gui) escapeMerge() error { + gui.takeOverMergeConflictScrolling() + + gui.State.Panels.Merging.Reset() + + // it's possible this method won't be called from the merging view so we need to + // ensure we only 'return' focus if we already have it + if gui.g.CurrentView() == gui.Views.Main { + return gui.pushContext(gui.State.Contexts.Files) + } + return nil } // promptToContinueRebase asks the user if they want to continue the rebase/merge that's in progress diff --git a/pkg/i18n/chinese.go b/pkg/i18n/chinese.go index 3167e0bf8..585d77b38 100644 --- a/pkg/i18n/chinese.go +++ b/pkg/i18n/chinese.go @@ -73,7 +73,6 @@ func chineseTranslationSet() TranslationSet { PullWait: "拉取中……", PushWait: "推送中……", FetchWait: "正在抓取……", - FileNoMergeCons: "该文件没有合并冲突", LcSoftReset: "软重置", AlreadyCheckedOutBranch: "您已经检出了这个分支", SureForceCheckout: "您确定要强制检出吗？您将丢失所有本地更改", diff --git a/pkg/i18n/dutch.go b/pkg/i18n/dutch.go index 914d558fe..7e8b2dcec 100644 --- a/pkg/i18n/dutch.go +++ b/pkg/i18n/dutch.go @@ -44,7 +44,6 @@ func dutchTranslationSet() TranslationSet { PullWait: "Pullen...", PushWait: "Pushen...", FetchWait: "Fetchen...", - FileNoMergeCons: "Dit bestand heeft geen merge conflicten", LcSoftReset: "zacht reset", AlreadyCheckedOutBranch: "Je hebt deze branch al uitgecheckt", SureForceCheckout: "Weet je zeker dat je het uitchecken wil forceren? Al je lokale verandering zullen worden verwijdert", diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 2defb4ae6..4ff6c3555 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -58,7 +58,6 @@ type TranslationSet struct { PullWait string PushWait string FetchWait string - FileNoMergeCons string LcSoftReset string AlreadyCheckedOutBranch string SureForceCheckout string @@ -608,7 +607,6 @@ func EnglishTranslationSet() TranslationSet { PullWait: "Pulling...", PushWait: "Pushing...", FetchWait: "Fetching...", - FileNoMergeCons: "This file has no inline merge conflicts", LcSoftReset: "soft reset", AlreadyCheckedOutBranch: "You have already checked out this branch", SureForceCheckout: "Are you sure you want force checkout? You will lose all local changes", diff --git a/pkg/i18n/polish.go b/pkg/i18n/polish.go index 5994a59e9..f378fa08b 100644 --- a/pkg/i18n/polish.go +++ b/pkg/i18n/polish.go @@ -39,7 +39,6 @@ func polishTranslationSet() TranslationSet { PullWait: "Pobieranie zmian...", PushWait: "Wysyłanie zmian...", FetchWait: "Pobieram...", - FileNoMergeCons: "Brak konfliktów scalania w pliku", AlreadyCheckedOutBranch: "Już przęłączono na tą gałąź", SureForceCheckout: "Jesteś pewny, że chcesz wymusić przełączenie? Stracisz wszystkie lokalne zmiany", ForceCheckoutBranch: "Wymuś przełączenie gałęzi",