Skip to content

Commit

Permalink
Use first class task objects instead of global counter
Browse files Browse the repository at this point in the history
The global counter approach is easy to understand but it's brittle and depends on implicit behaviour that is not very discoverable.

With a global counter, if any goroutine accidentally decrements the counter twice, we'll think lazygit is idle when it's actually busy.
Likewise if a goroutine accidentally increments the counter twice we'll think lazygit is busy when it's actually idle.
With the new approach we have a map of tasks where each task can either be busy or not. We create a new task and add it to the map
when we spawn a worker goroutine (among other things) and we remove it once the task is done.

The task can also be paused and continued for situations where we switch back and forth between running a program and asking for user
input.

In order for this to work with `git push` (and other commands that require credentials) we need to obtain the task from gocui when
we create the worker goroutine, and then pass it along to the commands package to pause/continue the task as required. This is
MUCH more discoverable than the old approach which just decremented and incremented the global counter from within the commands package,
but it's at the cost of expanding some function signatures (arguably a good thing).

Likewise, whenever you want to call WithWaitingStatus or WithLoaderPanel the callback will now have access to the task for pausing/
continuing. We only need to actually make use of this functionality in a couple of places so it's a high price to pay, but I don't
know if I want to introduce a WithWaitingStatusTask and WithLoaderPanelTask function (open to suggestions).
  • Loading branch information
jesseduffield committed Jul 9, 2023
1 parent ef7c5e3 commit f6d49b5
Show file tree
Hide file tree
Showing 39 changed files with 302 additions and 219 deletions.
6 changes: 4 additions & 2 deletions pkg/commands/git_commands/remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package git_commands

import (
"fmt"

"github.com/jesseduffield/gocui"
)

type RemoteCommands struct {
Expand Down Expand Up @@ -46,12 +48,12 @@ func (self *RemoteCommands) UpdateRemoteUrl(remoteName string, updatedUrl string
return self.cmd.New(cmdArgs).Run()
}

func (self *RemoteCommands) DeleteRemoteBranch(remoteName string, branchName string) error {
func (self *RemoteCommands) DeleteRemoteBranch(task *gocui.Task, remoteName string, branchName string) error {
cmdArgs := NewGitCmd("push").
Arg(remoteName, "--delete", branchName).
ToArgv()

return self.cmd.New(cmdArgs).PromptOnCredentialRequest().WithMutex(self.syncMutex).Run()
return self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).WithMutex(self.syncMutex).Run()
}

// CheckRemoteBranchExists Returns remote branch
Expand Down
47 changes: 22 additions & 25 deletions pkg/commands/git_commands/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package git_commands

import (
"github.com/go-errors/errors"
"github.com/jesseduffield/gocui"
"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
)

Expand All @@ -23,7 +24,7 @@ type PushOpts struct {
SetUpstream bool
}

func (self *SyncCommands) PushCmdObj(opts PushOpts) (oscommands.ICmdObj, error) {
func (self *SyncCommands) PushCmdObj(task *gocui.Task, opts PushOpts) (oscommands.ICmdObj, error) {
if opts.UpstreamBranch != "" && opts.UpstreamRemote == "" {
return nil, errors.New(self.Tr.MustSpecifyOriginError)
}
Expand All @@ -35,41 +36,37 @@ func (self *SyncCommands) PushCmdObj(opts PushOpts) (oscommands.ICmdObj, error)
ArgIf(opts.UpstreamBranch != "", opts.UpstreamBranch).
ToArgv()

cmdObj := self.cmd.New(cmdArgs).PromptOnCredentialRequest().WithMutex(self.syncMutex)
cmdObj := self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).WithMutex(self.syncMutex)
return cmdObj, nil
}

func (self *SyncCommands) Push(opts PushOpts) error {
cmdObj, err := self.PushCmdObj(opts)
func (self *SyncCommands) Push(task *gocui.Task, opts PushOpts) error {
cmdObj, err := self.PushCmdObj(task, opts)
if err != nil {
return err
}

return cmdObj.Run()
}

type FetchOptions struct {
Background bool
}

// Fetch fetch git repo
func (self *SyncCommands) FetchCmdObj(opts FetchOptions) oscommands.ICmdObj {
func (self *SyncCommands) Fetch(task *gocui.Task) error {
cmdArgs := NewGitCmd("fetch").
ArgIf(self.UserConfig.Git.FetchAll, "--all").
ToArgv()

cmdObj := self.cmd.New(cmdArgs)
if opts.Background {
cmdObj.DontLog().FailOnCredentialRequest()
} else {
cmdObj.PromptOnCredentialRequest()
}
return cmdObj.WithMutex(self.syncMutex)
cmdObj.PromptOnCredentialRequest(task)
return cmdObj.WithMutex(self.syncMutex).Run()
}

func (self *SyncCommands) Fetch(opts FetchOptions) error {
cmdObj := self.FetchCmdObj(opts)
return cmdObj.Run()
func (self *SyncCommands) FetchBackground() error {
cmdArgs := NewGitCmd("fetch").
ArgIf(self.UserConfig.Git.FetchAll, "--all").
ToArgv()

cmdObj := self.cmd.New(cmdArgs)
cmdObj.DontLog().FailOnCredentialRequest()
return cmdObj.WithMutex(self.syncMutex).Run()
}

type PullOptions struct {
Expand All @@ -78,7 +75,7 @@ type PullOptions struct {
FastForwardOnly bool
}

func (self *SyncCommands) Pull(opts PullOptions) error {
func (self *SyncCommands) Pull(task *gocui.Task, opts PullOptions) error {
cmdArgs := NewGitCmd("pull").
Arg("--no-edit").
ArgIf(opts.FastForwardOnly, "--ff-only").
Expand All @@ -88,22 +85,22 @@ func (self *SyncCommands) Pull(opts PullOptions) error {

// setting GIT_SEQUENCE_EDITOR to ':' as a way of skipping it, in case the user
// has 'pull.rebase = interactive' configured.
return self.cmd.New(cmdArgs).AddEnvVars("GIT_SEQUENCE_EDITOR=:").PromptOnCredentialRequest().WithMutex(self.syncMutex).Run()
return self.cmd.New(cmdArgs).AddEnvVars("GIT_SEQUENCE_EDITOR=:").PromptOnCredentialRequest(task).WithMutex(self.syncMutex).Run()
}

func (self *SyncCommands) FastForward(branchName string, remoteName string, remoteBranchName string) error {
func (self *SyncCommands) FastForward(task *gocui.Task, branchName string, remoteName string, remoteBranchName string) error {
cmdArgs := NewGitCmd("fetch").
Arg(remoteName).
Arg(remoteBranchName + ":" + branchName).
ToArgv()

return self.cmd.New(cmdArgs).PromptOnCredentialRequest().WithMutex(self.syncMutex).Run()
return self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).WithMutex(self.syncMutex).Run()
}

func (self *SyncCommands) FetchRemote(remoteName string) error {
func (self *SyncCommands) FetchRemote(task *gocui.Task, remoteName string) error {
cmdArgs := NewGitCmd("fetch").
Arg(remoteName).
ToArgv()

return self.cmd.New(cmdArgs).PromptOnCredentialRequest().WithMutex(self.syncMutex).Run()
return self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).WithMutex(self.syncMutex).Run()
}
6 changes: 4 additions & 2 deletions pkg/commands/git_commands/tag.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package git_commands

import "github.com/jesseduffield/gocui"

type TagCommands struct {
*GitCommon
}
Expand Down Expand Up @@ -34,9 +36,9 @@ func (self *TagCommands) Delete(tagName string) error {
return self.cmd.New(cmdArgs).Run()
}

func (self *TagCommands) Push(remoteName string, tagName string) error {
func (self *TagCommands) Push(task *gocui.Task, remoteName string, tagName string) error {
cmdArgs := NewGitCmd("push").Arg(remoteName, "tag", tagName).
ToArgv()

return self.cmd.New(cmdArgs).PromptOnCredentialRequest().WithMutex(self.syncMutex).Run()
return self.cmd.New(cmdArgs).PromptOnCredentialRequest(task).WithMutex(self.syncMutex).Run()
}
12 changes: 10 additions & 2 deletions pkg/commands/oscommands/cmd_obj.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"os/exec"
"strings"

"github.com/jesseduffield/gocui"
"github.com/samber/lo"
"github.com/sasha-s/go-deadlock"
)
Expand Down Expand Up @@ -56,13 +57,14 @@ type ICmdObj interface {
// returns true if IgnoreEmptyError() was called
ShouldIgnoreEmptyError() bool

PromptOnCredentialRequest() ICmdObj
PromptOnCredentialRequest(task *gocui.Task) ICmdObj
FailOnCredentialRequest() ICmdObj

WithMutex(mutex *deadlock.Mutex) ICmdObj
Mutex() *deadlock.Mutex

GetCredentialStrategy() CredentialStrategy
GetTask() *gocui.Task
}

type CmdObj struct {
Expand All @@ -85,6 +87,7 @@ type CmdObj struct {

// if set to true, it means we might be asked to enter a username/password by this command.
credentialStrategy CredentialStrategy
task *gocui.Task

// can be set so that we don't run certain commands simultaneously
mutex *deadlock.Mutex
Expand Down Expand Up @@ -192,8 +195,9 @@ func (self *CmdObj) RunAndProcessLines(onLine func(line string) (bool, error)) e
return self.runner.RunAndProcessLines(self, onLine)
}

func (self *CmdObj) PromptOnCredentialRequest() ICmdObj {
func (self *CmdObj) PromptOnCredentialRequest(task *gocui.Task) ICmdObj {
self.credentialStrategy = PROMPT
self.task = task

return self
}
Expand All @@ -207,3 +211,7 @@ func (self *CmdObj) FailOnCredentialRequest() ICmdObj {
func (self *CmdObj) GetCredentialStrategy() CredentialStrategy {
return self.credentialStrategy
}

func (self *CmdObj) GetTask() *gocui.Task {
return self.task
}
12 changes: 5 additions & 7 deletions pkg/commands/oscommands/cmd_obj_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strings"

"github.com/go-errors/errors"
"github.com/jesseduffield/gocui"
"github.com/jesseduffield/lazygit/pkg/utils"
"github.com/sirupsen/logrus"
)
Expand Down Expand Up @@ -308,7 +309,7 @@ func (self *cmdObjRunner) runAndDetectCredentialRequest(
tr := io.TeeReader(handler.stdoutPipe, cmdWriter)

go utils.Safe(func() {
self.processOutput(tr, handler.stdinPipe, promptUserForCredential)
self.processOutput(tr, handler.stdinPipe, promptUserForCredential, cmdObj.GetTask())
})
})
}
Expand All @@ -317,6 +318,7 @@ func (self *cmdObjRunner) processOutput(
reader io.Reader,
writer io.Writer,
promptUserForCredential func(CredentialType) <-chan string,
task *gocui.Task,
) {
checkForCredentialRequest := self.getCheckForCredentialRequestFunc()

Expand All @@ -327,13 +329,9 @@ func (self *cmdObjRunner) processOutput(
askFor, ok := checkForCredentialRequest(newBytes)
if ok {
responseChan := promptUserForCredential(askFor)
// We assume that the busy count is greater than zero here because we're
// in the middle of a command. We decrement it so that The user can be prompted
// without lazygit thinking it's still doing its own processing. This helps
// integration tests know how long to wait before typing in a response.
self.guiIO.DecrementBusyCount()
task.Pause()
toInput := <-responseChan
self.guiIO.IncrementBusyCount()
task.Continue()
// If the return data is empty we don't write anything to stdin
if toInput != "" {
_, _ = writer.Write([]byte(toInput))
Expand Down
9 changes: 0 additions & 9 deletions pkg/commands/oscommands/gui_io.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,19 @@ type guiIO struct {
// that a command requests it.
// the 'credential' arg is something like 'username' or 'password'
promptForCredentialFn func(credential CredentialType) <-chan string

IncrementBusyCount func()
DecrementBusyCount func()
}

func NewGuiIO(
log *logrus.Entry,
logCommandFn func(string, bool),
newCmdWriterFn func() io.Writer,
promptForCredentialFn func(CredentialType) <-chan string,
IncrementBusyCount func(),
DecrementBusyCount func(),
) *guiIO {
return &guiIO{
log: log,
logCommandFn: logCommandFn,
newCmdWriterFn: newCmdWriterFn,
promptForCredentialFn: promptForCredentialFn,
IncrementBusyCount: IncrementBusyCount,
DecrementBusyCount: DecrementBusyCount,
}
}

Expand All @@ -58,7 +51,5 @@ func NewNullGuiIO(log *logrus.Entry) *guiIO {
logCommandFn: func(string, bool) {},
newCmdWriterFn: func() io.Writer { return io.Discard },
promptForCredentialFn: failPromptFn,
IncrementBusyCount: func() {},
DecrementBusyCount: func() {},
}
}
6 changes: 3 additions & 3 deletions pkg/gui/background.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"strings"
"time"

"github.com/jesseduffield/lazygit/pkg/commands/git_commands"
"github.com/jesseduffield/gocui"
"github.com/jesseduffield/lazygit/pkg/gui/types"
"github.com/jesseduffield/lazygit/pkg/utils"
)
Expand Down Expand Up @@ -86,7 +86,7 @@ func (self *BackgroundRoutineMgr) goEvery(interval time.Duration, stop chan stru
if self.pauseBackgroundRefreshes {
continue
}
self.gui.c.OnWorker(func() { _ = function() })
self.gui.c.OnWorker(func(*gocui.Task) { _ = function() })
case <-stop:
return
}
Expand All @@ -95,7 +95,7 @@ func (self *BackgroundRoutineMgr) goEvery(interval time.Duration, stop chan stru
}

func (self *BackgroundRoutineMgr) backgroundFetch() (err error) {
err = self.gui.git.Sync.Fetch(git_commands.FetchOptions{Background: true})
err = self.gui.git.Sync.FetchBackground()

_ = self.gui.c.Refresh(types.RefreshOptions{Scope: []types.RefreshableView{types.BRANCHES, types.COMMITS, types.REMOTES, types.TAGS}, Mode: types.ASYNC})

Expand Down
6 changes: 4 additions & 2 deletions pkg/gui/controllers/branches_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"strings"

"github.com/jesseduffield/gocui"
"github.com/jesseduffield/lazygit/pkg/commands/git_commands"
"github.com/jesseduffield/lazygit/pkg/commands/models"
"github.com/jesseduffield/lazygit/pkg/gui/context"
Expand Down Expand Up @@ -363,11 +364,12 @@ func (self *BranchesController) fastForward(branch *models.Branch) error {
},
)

return self.c.WithLoaderPanel(message, func() error {
return self.c.WithLoaderPanel(message, func(task *gocui.Task) error {
if branch == self.c.Helpers().Refs.GetCheckedOutRef() {
self.c.LogAction(action)

err := self.c.Git().Sync.Pull(
task,
git_commands.PullOptions{
RemoteName: branch.UpstreamRemote,
BranchName: branch.UpstreamBranch,
Expand All @@ -381,7 +383,7 @@ func (self *BranchesController) fastForward(branch *models.Branch) error {
return self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC})
} else {
self.c.LogAction(action)
err := self.c.Git().Sync.FastForward(branch.Name, branch.UpstreamRemote, branch.UpstreamBranch)
err := self.c.Git().Sync.FastForward(task, branch.Name, branch.UpstreamRemote, branch.UpstreamBranch)
if err != nil {
_ = self.c.Error(err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/gui/controllers/commits_files_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ func (self *CommitFilesController) discard(node *filetree.CommitFileNode) error
Title: self.c.Tr.DiscardFileChangesTitle,
Prompt: prompt,
HandleConfirm: func() error {
return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func() error {
return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(*gocui.Task) error {
self.c.LogAction(self.c.Tr.Actions.DiscardOldFileChange)
if err := self.c.Git().Rebase.DiscardOldFileChanges(self.c.Model().Commits, self.c.Contexts().LocalCommits.GetSelectedLineIdx(), node.GetPath()); err != nil {
if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err); err != nil {
Expand Down Expand Up @@ -205,7 +205,7 @@ func (self *CommitFilesController) edit(node *filetree.CommitFileNode) error {

func (self *CommitFilesController) toggleForPatch(node *filetree.CommitFileNode) error {
toggle := func() error {
return self.c.WithWaitingStatus(self.c.Tr.UpdatingPatch, func() error {
return self.c.WithWaitingStatus(self.c.Tr.UpdatingPatch, func(*gocui.Task) error {
if !self.c.Git().Patch.PatchBuilder.Active() {
if err := self.startPatchBuilder(); err != nil {
return err
Expand Down
Loading

0 comments on commit f6d49b5

Please sign in to comment.