diff options
author | Jesse Duffield <jessedduffield@gmail.com> | 2023-07-08 14:17:54 +1000 |
---|---|---|
committer | Jesse Duffield <jessedduffield@gmail.com> | 2023-07-08 22:54:52 +1000 |
commit | 26ca41a40e3ee79a3e84542ff6cf3fd3f2745679 (patch) | |
tree | 42df8e770a7a10147e5716d802ad90ba2e01f987 /pkg | |
parent | 6c4e7ee9729ccfd65ac03073a37bd110a61be432 (diff) |
Handle pending actions properly in git commands that require credentials
I don't know if this is a hack or not: we run a git command and increment the pending action
count to 1 but at some point the command requests a username or password, so we need to prompt
the user to enter that. At that point we don't want to say that there is a pending action,
so we decrement the action count before prompting the user and then re-increment it again afterward.
Given that we panic when the counter goes below zero, it's important that it's not zero
when we run the git command (should be impossible anyway).
I toyed with a different approach using channels and a long-running goroutine that
handles all commands that request credentials but it feels over-engineered compared to this
commit's approach.
Diffstat (limited to 'pkg')
-rw-r--r-- | pkg/commands/oscommands/cmd_obj_runner.go | 122 | ||||
-rw-r--r-- | pkg/commands/oscommands/cmd_obj_runner_test.go | 14 | ||||
-rw-r--r-- | pkg/commands/oscommands/gui_io.go | 18 | ||||
-rw-r--r-- | pkg/gui/controllers/helpers/credentials_helper.go | 23 | ||||
-rw-r--r-- | pkg/gui/gui.go | 2 |
5 files changed, 111 insertions, 68 deletions
diff --git a/pkg/commands/oscommands/cmd_obj_runner.go b/pkg/commands/oscommands/cmd_obj_runner.go index 3df1e2916..55cf1e1ce 100644 --- a/pkg/commands/oscommands/cmd_obj_runner.go +++ b/pkg/commands/oscommands/cmd_obj_runner.go @@ -19,15 +19,6 @@ type ICmdObjRunner interface { RunAndProcessLines(cmdObj ICmdObj, onLine func(line string) (bool, error)) error } -type CredentialType int - -const ( - Password CredentialType = iota - Username - Passphrase - PIN -) - type cmdObjRunner struct { log *logrus.Entry guiIO *guiIO @@ -182,26 +173,6 @@ func (self *cmdObjRunner) RunAndProcessLines(cmdObj ICmdObj, onLine func(line st return nil } -// Whenever we're asked for a password we just enter a newline, which will -// eventually cause the command to fail. -var failPromptFn = func(CredentialType) string { return "\n" } - -func (self *cmdObjRunner) runWithCredentialHandling(cmdObj ICmdObj) error { - var promptFn func(CredentialType) string - - switch cmdObj.GetCredentialStrategy() { - case PROMPT: - promptFn = self.guiIO.promptForCredentialFn - case FAIL: - promptFn = failPromptFn - case NONE: - // we should never land here - return errors.New("runWithCredentialHandling called but cmdObj does not have a credential strategy") - } - - return self.runAndDetectCredentialRequest(cmdObj, promptFn) -} - func (self *cmdObjRunner) logCmdObj(cmdObj ICmdObj) { self.guiIO.logCommandFn(cmdObj.ToString(), true) } @@ -233,25 +204,6 @@ func (self *cmdObjRunner) runAndStream(cmdObj ICmdObj) error { }) } -// runAndDetectCredentialRequest detect a username / password / passphrase question in a command -// promptUserForCredential is a function that gets executed when this function detect you need to fillin a password or passphrase -// The promptUserForCredential argument will be "username", "password" or "passphrase" and expects the user's password/passphrase or username back -func (self *cmdObjRunner) runAndDetectCredentialRequest( - cmdObj ICmdObj, - promptUserForCredential func(CredentialType) string, -) error { - // setting the output to english so we can parse it for a username/password request - cmdObj.AddEnvVars("LANG=en_US.UTF-8", "LC_ALL=en_US.UTF-8") - - return self.runAndStreamAux(cmdObj, func(handler *cmdHandler, cmdWriter io.Writer) { - tr := io.TeeReader(handler.stdoutPipe, cmdWriter) - - go utils.Safe(func() { - self.processOutput(tr, handler.stdinPipe, promptUserForCredential) - }) - }) -} - func (self *cmdObjRunner) runAndStreamAux( cmdObj ICmdObj, onRun func(*cmdHandler, io.Writer), @@ -302,7 +254,70 @@ func (self *cmdObjRunner) runAndStreamAux( return nil } -func (self *cmdObjRunner) processOutput(reader io.Reader, writer io.Writer, promptUserForCredential func(CredentialType) string) { +type CredentialType int + +const ( + Password CredentialType = iota + Username + Passphrase + PIN +) + +// Whenever we're asked for a password we just enter a newline, which will +// eventually cause the command to fail. +var failPromptFn = func(CredentialType) <-chan string { + ch := make(chan string) + go func() { + ch <- "\n" + }() + return ch +} + +func (self *cmdObjRunner) runWithCredentialHandling(cmdObj ICmdObj) error { + promptFn, err := self.getCredentialPromptFn(cmdObj) + if err != nil { + return err + } + + return self.runAndDetectCredentialRequest(cmdObj, promptFn) +} + +func (self *cmdObjRunner) getCredentialPromptFn(cmdObj ICmdObj) (func(CredentialType) <-chan string, error) { + switch cmdObj.GetCredentialStrategy() { + case PROMPT: + return self.guiIO.promptForCredentialFn, nil + case FAIL: + return failPromptFn, nil + default: + // we should never land here + return nil, errors.New("runWithCredentialHandling called but cmdObj does not have a credential strategy") + } +} + +// runAndDetectCredentialRequest detect a username / password / passphrase question in a command +// promptUserForCredential is a function that gets executed when this function detect you need to fillin a password or passphrase +// The promptUserForCredential argument will be "username", "password" or "passphrase" and expects the user's password/passphrase or username back +func (self *cmdObjRunner) runAndDetectCredentialRequest( + cmdObj ICmdObj, + promptUserForCredential func(CredentialType) <-chan string, +) error { + // setting the output to english so we can parse it for a username/password request + cmdObj.AddEnvVars("LANG=en_US.UTF-8", "LC_ALL=en_US.UTF-8") + + return self.runAndStreamAux(cmdObj, func(handler *cmdHandler, cmdWriter io.Writer) { + tr := io.TeeReader(handler.stdoutPipe, cmdWriter) + + go utils.Safe(func() { + self.processOutput(tr, handler.stdinPipe, promptUserForCredential) + }) + }) +} + +func (self *cmdObjRunner) processOutput( + reader io.Reader, + writer io.Writer, + promptUserForCredential func(CredentialType) <-chan string, +) { checkForCredentialRequest := self.getCheckForCredentialRequestFunc() scanner := bufio.NewScanner(reader) @@ -311,7 +326,14 @@ func (self *cmdObjRunner) processOutput(reader io.Reader, writer io.Writer, prom newBytes := scanner.Bytes() askFor, ok := checkForCredentialRequest(newBytes) if ok { - toInput := promptUserForCredential(askFor) + 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() + toInput := <-responseChan + self.guiIO.IncrementBusyCount() // If the return data is empty we don't write anything to stdin if toInput != "" { _, _ = writer.Write([]byte(toInput)) diff --git a/pkg/commands/oscommands/cmd_obj_runner_test.go b/pkg/commands/oscommands/cmd_obj_runner_test.go index ab26b9827..3b2340649 100644 --- a/pkg/commands/oscommands/cmd_obj_runner_test.go +++ b/pkg/commands/oscommands/cmd_obj_runner_test.go @@ -15,6 +15,18 @@ func getRunner() *cmdObjRunner { } } +func toChanFn(f func(ct CredentialType) string) func(CredentialType) <-chan string { + return func(ct CredentialType) <-chan string { + ch := make(chan string) + + go func() { + ch <- f(ct) + }() + + return ch + } +} + func TestProcessOutput(t *testing.T) { defaultPromptUserForCredential := func(ct CredentialType) string { switch ct { @@ -99,7 +111,7 @@ func TestProcessOutput(t *testing.T) { reader := strings.NewReader(scenario.output) writer := &strings.Builder{} - runner.processOutput(reader, writer, scenario.promptUserForCredential) + runner.processOutput(reader, writer, toChanFn(scenario.promptUserForCredential)) if writer.String() != scenario.expectedToWrite { t.Errorf("expected to write '%s' but got '%s'", scenario.expectedToWrite, writer.String()) diff --git a/pkg/commands/oscommands/gui_io.go b/pkg/commands/oscommands/gui_io.go index 10a8b2678..1ff090052 100644 --- a/pkg/commands/oscommands/gui_io.go +++ b/pkg/commands/oscommands/gui_io.go @@ -26,15 +26,27 @@ type guiIO struct { // this allows us to request info from the user like username/password, in the event // that a command requests it. // the 'credential' arg is something like 'username' or 'password' - promptForCredentialFn func(credential CredentialType) string + 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) string) *guiIO { +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, } } @@ -46,5 +58,7 @@ func NewNullGuiIO(log *logrus.Entry) *guiIO { logCommandFn: func(string, bool) {}, newCmdWriterFn: func() io.Writer { return io.Discard }, promptForCredentialFn: failPromptFn, + IncrementBusyCount: func() {}, + DecrementBusyCount: func() {}, } } diff --git a/pkg/gui/controllers/helpers/credentials_helper.go b/pkg/gui/controllers/helpers/credentials_helper.go index 0aed34110..20fb59052 100644 --- a/pkg/gui/controllers/helpers/credentials_helper.go +++ b/pkg/gui/controllers/helpers/credentials_helper.go @@ -1,8 +1,6 @@ package helpers import ( - "sync" - "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/gui/types" ) @@ -20,11 +18,11 @@ func NewCredentialsHelper( } // promptUserForCredential wait for a username, password or passphrase input from the credentials popup -func (self *CredentialsHelper) PromptUserForCredential(passOrUname oscommands.CredentialType) string { - waitGroup := sync.WaitGroup{} - waitGroup.Add(1) - - userInput := "" +// We return a channel rather than returning the string directly so that the calling function knows +// when the prompt has been created (before the user has entered anything) so that it can +// note that we're now waiting on user input and lazygit isn't processing anything. +func (self *CredentialsHelper) PromptUserForCredential(passOrUname oscommands.CredentialType) <-chan string { + ch := make(chan string) self.c.OnUIThread(func() error { title, mask := self.getTitleAndMask(passOrUname) @@ -33,24 +31,19 @@ func (self *CredentialsHelper) PromptUserForCredential(passOrUname oscommands.Cr Title: title, Mask: mask, HandleConfirm: func(input string) error { - userInput = input - - waitGroup.Done() + ch <- input + "\n" return self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC}) }, HandleClose: func() error { - waitGroup.Done() + ch <- "\n" return nil }, }) }) - // wait for username/passwords/passphrase input - waitGroup.Wait() - - return userInput + "\n" + return ch } func (self *CredentialsHelper) getTitleAndMask(passOrUname oscommands.CredentialType) (string, bool) { diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go index 92deee1a0..5bba6b967 100644 --- a/pkg/gui/gui.go +++ b/pkg/gui/gui.go @@ -487,6 +487,8 @@ func NewGui( gui.LogCommand, gui.getCmdWriter, credentialsHelper.PromptUserForCredential, + func() { gui.g.IncrementBusyCount() }, + func() { gui.g.DecrementBusyCount() }, ) osCommand := oscommands.NewOSCommand(cmn, config, oscommands.GetPlatform(), guiIO) |