Merge pull request #2327 from ActiveState/mitchell/dx-1527-2

Do not use terminal escape sequences in non-interactive terminals or the Windows command prompt.

Author: mitchell-as

cmd/state/main.go

@@ -82,7 +82,8 @@ func main() {
 
 	// Set up our output formatter/writer
 	outFlags := parseOutputFlags(os.Args)
-	out, err := initOutput(outFlags, "")
+	shellName, _ := subshell.DetectShell(cfg)
+	out, err := initOutput(outFlags, "", shellName)
 	if err != nil {
 		multilog.Critical("Could not initialize outputer: %s", errs.JoinMessage(err))
 		os.Stderr.WriteString(locale.Tr("err_main_outputer", err.Error()))

cmd/state/main_test.go

@@ -15,37 +15,37 @@ type MainTestSuite struct {
 
 func (suite *MainTestSuite) TestOutputer() {
 	{
-		outputer, err := initOutput(outputFlags{"", false, false, false}, "")
+		outputer, err := initOutput(outputFlags{"", false, false, false}, "", "")
 		suite.Require().NoError(err, errs.Join(err, "\n").Error())
 		suite.Equal(output.PlainFormatName, outputer.Type(), "Returns Plain outputer")
 	}
 
 	{
-		outputer, err := initOutput(outputFlags{string(output.PlainFormatName), false, false, false}, "")
+		outputer, err := initOutput(outputFlags{string(output.PlainFormatName), false, false, false}, "", "")
 		suite.Require().NoError(err)
 		suite.Equal(output.PlainFormatName, outputer.Type(), "Returns Plain outputer")
 	}
 
 	{
-		outputer, err := initOutput(outputFlags{string(output.JSONFormatName), false, false, false}, "")
+		outputer, err := initOutput(outputFlags{string(output.JSONFormatName), false, false, false}, "", "")
 		suite.Require().NoError(err)
 		suite.Equal(output.JSONFormatName, outputer.Type(), "Returns JSON outputer")
 	}
 
 	{
-		outputer, err := initOutput(outputFlags{"", false, false, false}, string(output.JSONFormatName))
+		outputer, err := initOutput(outputFlags{"", false, false, false}, string(output.JSONFormatName), "")
 		suite.Require().NoError(err)
 		suite.Equal(output.JSONFormatName, outputer.Type(), "Returns JSON outputer")
 	}
 
 	{
-		outputer, err := initOutput(outputFlags{"", false, false, false}, string(output.EditorFormatName))
+		outputer, err := initOutput(outputFlags{"", false, false, false}, string(output.EditorFormatName), "")
 		suite.Require().NoError(err)
 		suite.Equal(output.EditorFormatName, outputer.Type(), "Returns JSON outputer")
 	}
 
 	{
-		outputer, err := initOutput(outputFlags{"", false, false, false}, string(output.EditorV0FormatName))
+		outputer, err := initOutput(outputFlags{"", false, false, false}, string(output.EditorV0FormatName), "")
 		suite.Require().NoError(err)
 		suite.Equal(output.EditorV0FormatName, outputer.Type(), "Returns JSON outputer")
 	}

cmd/state/output.go

@@ -4,15 +4,14 @@ import (
 	"errors"
 	"os"
 
-	"github.com/jessevdk/go-flags"
-
 	"github.com/ActiveState/cli/internal/errs"
 	"github.com/ActiveState/cli/internal/logging"
 	"github.com/ActiveState/cli/internal/multilog"
 	"github.com/ActiveState/cli/internal/output"
 	"github.com/ActiveState/cli/internal/rollbar"
 	"github.com/ActiveState/cli/internal/terminal"
-
+	"github.com/jessevdk/go-flags"
+	"golang.org/x/term"
 	survey "gopkg.in/AlecAivazis/survey.v1/core"
 )
 
@@ -47,7 +46,7 @@ func parseOutputFlags(args []string) outputFlags {
 	return flagSet
 }
 
-func initOutput(flags outputFlags, formatName string) (output.Outputer, error) {
+func initOutput(flags outputFlags, formatName string, shellName string) (output.Outputer, error) {
 	if formatName == "" {
 		formatName = flags.Output
 	}
@@ -56,13 +55,14 @@ func initOutput(flags outputFlags, formatName string) (output.Outputer, error) {
 		OutWriter:   os.Stdout,
 		ErrWriter:   os.Stderr,
 		Colored:     !flags.DisableColor(),
-		Interactive: true,
+		Interactive: term.IsTerminal(int(os.Stdin.Fd())),
+		ShellName:   shellName,
 	})
 	if err != nil {
 		if errors.Is(err, output.ErrNotRecognized) {
 			// The formatter might still be registered, so default to plain for now
 			logging.Warning("Output format not recognized: %s, defaulting to plain output instead", formatName)
-			return initOutput(flags, string(output.PlainFormatName))
+			return initOutput(flags, string(output.PlainFormatName), shellName)
 		}
 		multilog.Log(logging.ErrorNoStacktrace, rollbar.Error)("Could not create outputer, name: %s, error: %s", formatName, err.Error())
 		return nil, errs.Wrap(err, "output.New %s failed", formatName)

internal/output/output.go

@@ -90,4 +90,5 @@ type Config struct {
 	ErrWriter   io.Writer
 	Colored     bool
 	Interactive bool
+	ShellName   string
 }

internal/output/progress.go

@@ -6,14 +6,15 @@ import (
 	"time"
 )
 
-const moveCaretBack = "\x1b[%dD" // %d is the number of characters to move back
+const moveCaretBackEscapeSequence = "\x1b[%dD" // %d is the number of characters to move back
 
 type Spinner struct {
-	frame    int
-	frames   []string
-	out      Outputer
-	stop     chan struct{}
-	interval time.Duration
+	frame         int
+	frames        []string
+	out           Outputer
+	stop          chan struct{}
+	interval      time.Duration
+	reportedError bool
 }
 
 var _ Marshaller = &Spinner{}
@@ -27,7 +28,7 @@ func StartSpinner(out Outputer, msg string, interval time.Duration) *Spinner {
 	if out.Config().Interactive {
 		frames = []string{`|`, `/`, `-`, `\`}
 	}
-	d := &Spinner{0, frames, out, make(chan struct{}, 1), interval}
+	d := &Spinner{0, frames, out, make(chan struct{}, 1), interval, false}
 
 	if msg != "" {
 		d.out.Fprint(d.out.Config().ErrWriter, strings.TrimSuffix(msg, " ")+" ")
@@ -50,7 +51,11 @@ func (d *Spinner) moveCaretBack() int {
 		prevPos = len(d.frames) - 1
 	}
 	prevFrame := d.frames[prevPos]
-	d.out.Fprint(d.out.Config().ErrWriter, fmt.Sprintf(moveCaretBack, len(prevFrame)))
+	if d.out.Config().ShellName != "cmd" { // cannot use subshell/cmd.Name due to import cycle
+		d.moveCaretBackInTerminal(len(prevFrame))
+	} else {
+		d.moveCaretBackInCommandPrompt(len(prevFrame))
+	}
 
 	return len(prevFrame)
 }
@@ -96,3 +101,7 @@ func (d *Spinner) Stop(msg string) {
 
 	d.out.Fprint(d.out.Config().ErrWriter, "\n")
 }
+
+func (d *Spinner) moveCaretBackInTerminal(n int) {
+	d.out.Fprint(d.out.Config().ErrWriter, fmt.Sprintf(moveCaretBackEscapeSequence, n))
+}

internal/output/progress_unix.go

@@ -0,0 +1,15 @@
+//go:build linux || darwin
+// +build linux darwin
+
+package output
+
+import (
+	"github.com/ActiveState/cli/internal/rollbar"
+)
+
+func (d *Spinner) moveCaretBackInCommandPrompt(n int) {
+	if !d.reportedError {
+		rollbar.Error("Incorrectly detected Windows command prompt in Unix environment")
+		d.reportedError = true
+	}
+}

internal/output/progress_win.go

@@ -0,0 +1,59 @@
+//go:build windows
+// +build windows
+
+package output
+
+import (
+	"os"
+	"syscall"
+	"unsafe"
+
+	"github.com/ActiveState/cli/internal/rollbar"
+)
+
+var kernel32 = syscall.NewLazyDLL("kernel32.dll")
+var procGetConsoleScreenBufferInfo = kernel32.NewProc("GetConsoleScreenBufferInfo")
+var procSetConsoleCursorPosition = kernel32.NewProc("SetConsoleCursorPosition")
+
+type coord struct {
+	x short
+	y short
+}
+
+type short int16
+type word uint16
+
+type smallRect struct {
+	bottom short
+	left   short
+	right  short
+	top    short
+}
+
+type consoleScreenBufferInfo struct {
+	size              coord
+	cursorPosition    coord
+	attributes        word
+	window            smallRect
+	maximumWindowSize coord
+}
+
+func (d *Spinner) moveCaretBackInCommandPrompt(n int) {
+	handle := syscall.Handle(os.Stdout.Fd())
+
+	var csbi consoleScreenBufferInfo
+	if _, _, err := procGetConsoleScreenBufferInfo.Call(uintptr(handle), uintptr(unsafe.Pointer(&csbi))); err != nil {
+		var cursor coord
+		cursor.x = csbi.cursorPosition.x + short(-n)
+		cursor.y = csbi.cursorPosition.y
+
+		_, _, err2 := procSetConsoleCursorPosition.Call(uintptr(handle), uintptr(*(*int32)(unsafe.Pointer(&cursor))))
+		if err2 != nil && !d.reportedError {
+			rollbar.Error("Error calling SetConsoleCursorPosition: %v", err2)
+			d.reportedError = true
+		}
+	} else if !d.reportedError {
+		rollbar.Error("Error calling GetConsoleScreenBufferInfo: %v", err)
+		d.reportedError = true
+	}
+}

internal/subshell/subshell.go

@@ -88,49 +88,34 @@ type SubShell interface {
 
 // New returns the subshell relevant to the current process, but does not activate it
 func New(cfg sscommon.Configurable) SubShell {
-	binary := resolveBinaryPath(DetectShellBinary(cfg))
-
-	name := filepath.Base(binary)
-	name = strings.TrimSuffix(name, filepath.Ext(name))
-	logging.Debug("Detected SHELL: %s", name)
-
-	if runtime.GOOS == "windows" {
-		// For some reason Go or MSYS doesn't translate paths with spaces correctly, so we have to strip out the
-		// invalid escape characters for spaces
-		binary = strings.ReplaceAll(binary, `\ `, ` `)
-	}
+	name, path := DetectShell(cfg)
 
 	var subs SubShell
 	switch name {
-	case "bash":
+	case bash.Name:
 		subs = &bash.SubShell{}
-	case "zsh":
+	case zsh.Name:
 		subs = &zsh.SubShell{}
-	case "tcsh":
+	case tcsh.Name:
 		subs = &tcsh.SubShell{}
-	case "fish":
+	case fish.Name:
 		subs = &fish.SubShell{}
-	case "cmd":
+	case cmd.Name:
 		subs = &cmd.SubShell{}
 	default:
-		logging.Debug("Unsupported shell: %s, defaulting to OS default.", name)
-		rollbar.Error("Unsupported shell: %s", name) // we just want to know what this person is using
+		rollbar.Error("subshell.DetectShell did not return a known name: %s", name)
 		switch runtime.GOOS {
 		case "windows":
-			binary = "cmd.exe"
 			subs = &cmd.SubShell{}
 		case "darwin":
-			binary = "zsh"
 			subs = &zsh.SubShell{}
 		default:
-			binary = "bash"
 			subs = &bash.SubShell{}
 		}
-		binary = resolveBinaryPath(binary)
 	}
 
-	logging.Debug("Using binary: %s", binary)
-	subs.SetBinary(binary)
+	logging.Debug("Using binary: %s", path)
+	subs.SetBinary(path)
 
 	env := funk.FilterString(os.Environ(), func(s string) bool {
 		return !strings.HasPrefix(s, constants.ProjectEnvVarName)
@@ -182,8 +167,10 @@ func ConfigureAvailableShells(shell SubShell, cfg sscommon.Configurable, env map
 	return nil
 }
 
-func DetectShellBinary(cfg sscommon.Configurable) (binary string) {
+// DetectShell detects the shell relevant to the current process and returns its name and path.
+func DetectShell(cfg sscommon.Configurable) (string, string) {
 	configured := cfg.GetString(ConfigKeyShell)
+	var binary string
 	defer func() {
 		// do not re-write shell binary to config, if the value did not change.
 		if configured == binary {
@@ -196,25 +183,56 @@ func DetectShellBinary(cfg sscommon.Configurable) (binary string) {
 		}
 	}()
 
-	if binary := os.Getenv("SHELL"); binary != "" {
-		return binary
-	}
-
-	if runtime.GOOS == "windows" {
+	binary = os.Getenv("SHELL")
+	if binary == "" && runtime.GOOS == "windows" {
 		binary = os.Getenv("ComSpec")
-		if binary != "" {
-			return binary
-		}
 	}
 
-	fallback := configured
-	if fallback == "" {
+	if binary == "" {
+		binary = configured
+	}
+	if binary == "" {
 		if runtime.GOOS == "windows" {
-			fallback = "cmd.exe"
+			binary = "cmd.exe"
 		} else {
-			fallback = "bash"
+			binary = "bash"
+		}
+	}
+
+	path := resolveBinaryPath(binary)
+
+	name := filepath.Base(path)
+	name = strings.TrimSuffix(name, filepath.Ext(name))
+	logging.Debug("Detected SHELL: %s", name)
+
+	if runtime.GOOS == "windows" {
+		// For some reason Go or MSYS doesn't translate paths with spaces correctly, so we have to strip out the
+		// invalid escape characters for spaces
+		path = strings.ReplaceAll(path, `\ `, ` `)
+	}
+
+	isKnownShell := false
+	for _, ssName := range []string{bash.Name, cmd.Name, fish.Name, tcsh.Name, zsh.Name} {
+		if name == ssName {
+			isKnownShell = true
+			break
+		}
+	}
+	if !isKnownShell {
+		logging.Debug("Unsupported shell: %s, defaulting to OS default.", name)
+		rollbar.Error("Unsupported shell: %s", name) // we just want to know what this person is using
+		switch runtime.GOOS {
+		case "windows":
+			name = cmd.Name
+			path = resolveBinaryPath("cmd.exe")
+		case "darwin":
+			name = zsh.Name
+			path = resolveBinaryPath("zsh")
+		default:
+			name = bash.Name
+			path = resolveBinaryPath("bash")
 		}
 	}
 
-	return fallback
+	return name, path
 }