Merge pull request #4153 from thaJeztah/streams_cleanup

cli/streams: minor refactoring and docs touch-ups

Author: thaJeztah

cli/streams/in.go

@@ -9,38 +9,42 @@ import (
 	"github.com/moby/term"
 )
 
-// In is an input stream used by the DockerCli to read user input
+// In is an input stream to read user input. It implements [io.ReadCloser]
+// with additional utilities, such as putting the terminal in raw mode.
 type In struct {
 	commonStream
 	in io.ReadCloser
 }
 
+// Read implements the [io.Reader] interface.
 func (i *In) Read(p []byte) (int, error) {
 	return i.in.Read(p)
 }
 
-// Close implements the Closer interface
+// Close implements the [io.Closer] interface.
 func (i *In) Close() error {
 	return i.in.Close()
 }
 
-// SetRawTerminal sets raw mode on the input terminal
+// SetRawTerminal sets raw mode on the input terminal. It is a no-op if In
+// is not a TTY, or if the "NORAW" environment variable is set to a non-empty
+// value.
 func (i *In) SetRawTerminal() (err error) {
-	if os.Getenv("NORAW") != "" || !i.commonStream.isTerminal {
+	if !i.isTerminal || os.Getenv("NORAW") != "" {
 		return nil
 	}
-	i.commonStream.state, err = term.SetRawTerminal(i.commonStream.fd)
+	i.state, err = term.SetRawTerminal(i.fd)
 	return err
 }
 
-// CheckTty checks if we are trying to attach to a container tty
-// from a non-tty client input stream, and if so, returns an error.
+// CheckTty checks if we are trying to attach to a container TTY
+// from a non-TTY client input stream, and if so, returns an error.
 func (i *In) CheckTty(attachStdin, ttyMode bool) error {
 	// In order to attach to a container tty, input stream for the client must
 	// be a tty itself: redirecting or piping the client standard input is
 	// incompatible with `docker run -t`, `docker exec -t` or `docker attach`.
 	if ttyMode && attachStdin && !i.isTerminal {
-		eText := "the input device is not a TTY"
+		const eText = "the input device is not a TTY"
 		if runtime.GOOS == "windows" {
 			return errors.New(eText + ".  If you are using mintty, try prefixing the command with 'winpty'")
 		}
@@ -49,8 +53,9 @@ func (i *In) CheckTty(attachStdin, ttyMode bool) error {
 	return nil
 }
 
-// NewIn returns a new In object from a ReadCloser
+// NewIn returns a new [In] from an [io.ReadCloser].
 func NewIn(in io.ReadCloser) *In {
-	fd, isTerminal := term.GetFdInfo(in)
-	return &In{commonStream: commonStream{fd: fd, isTerminal: isTerminal}, in: in}
+	i := &In{in: in}
+	i.fd, i.isTerminal = term.GetFdInfo(in)
+	return i
 }

cli/streams/out.go

@@ -8,8 +8,9 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// Out is an output stream used by the DockerCli to write normal program
-// output.
+// Out is an output stream to write normal program output. It implements
+// an [io.Writer], with additional utilities for detecting whether a terminal
+// is connected, getting the TTY size, and putting the terminal in raw mode.
 type Out struct {
 	commonStream
 	out io.Writer
@@ -19,32 +20,39 @@ func (o *Out) Write(p []byte) (int, error) {
 	return o.out.Write(p)
 }
 
-// SetRawTerminal sets raw mode on the input terminal
+// SetRawTerminal puts the output of the terminal connected to the stream
+// into raw mode.
+//
+// On UNIX, this does nothing. On Windows, it disables LF -> CRLF/ translation.
+// It is a no-op if Out is not a TTY, or if the "NORAW" environment variable is
+// set to a non-empty value.
 func (o *Out) SetRawTerminal() (err error) {
-	if os.Getenv("NORAW") != "" || !o.commonStream.isTerminal {
+	if !o.isTerminal || os.Getenv("NORAW") != "" {
 		return nil
 	}
-	o.commonStream.state, err = term.SetRawTerminalOutput(o.commonStream.fd)
+	o.state, err = term.SetRawTerminalOutput(o.fd)
 	return err
 }
 
-// GetTtySize returns the height and width in characters of the tty
-func (o *Out) GetTtySize() (uint, uint) {
+// GetTtySize returns the height and width in characters of the TTY, or
+// zero for both if no TTY is connected.
+func (o *Out) GetTtySize() (height uint, width uint) {
 	if !o.isTerminal {
 		return 0, 0
 	}
 	ws, err := term.GetWinsize(o.fd)
 	if err != nil {
-		logrus.Debugf("Error getting size: %s", err)
+		logrus.WithError(err).Debug("Error getting TTY size")
 		if ws == nil {
 			return 0, 0
 		}
 	}
 	return uint(ws.Height), uint(ws.Width)
 }
 
-// NewOut returns a new Out object from a Writer
+// NewOut returns a new [Out] from an [io.Writer].
 func NewOut(out io.Writer) *Out {
-	fd, isTerminal := term.GetFdInfo(out)
-	return &Out{commonStream: commonStream{fd: fd, isTerminal: isTerminal}, out: out}
+	o := &Out{out: out}
+	o.fd, o.isTerminal = term.GetFdInfo(out)
+	return o
 }

cli/streams/stream.go

@@ -4,31 +4,32 @@ import (
 	"github.com/moby/term"
 )
 
-// commonStream is an input stream used by the DockerCli to read user input
 type commonStream struct {
 	fd         uintptr
 	isTerminal bool
 	state      *term.State
 }
 
-// FD returns the file descriptor number for this stream
+// FD returns the file descriptor number for this stream.
 func (s *commonStream) FD() uintptr {
 	return s.fd
 }
 
-// IsTerminal returns true if this stream is connected to a terminal
+// IsTerminal returns true if this stream is connected to a terminal.
 func (s *commonStream) IsTerminal() bool {
 	return s.isTerminal
 }
 
-// RestoreTerminal restores normal mode to the terminal
+// RestoreTerminal restores normal mode to the terminal.
 func (s *commonStream) RestoreTerminal() {
 	if s.state != nil {
-		term.RestoreTerminal(s.fd, s.state)
+		_ = term.RestoreTerminal(s.fd, s.state)
 	}
 }
 
-// SetIsTerminal sets the boolean used for isTerminal
+// SetIsTerminal overrides whether a terminal is connected. It is used to
+// override this property in unit-tests, and should not be depended on for
+// other purposes.
 func (s *commonStream) SetIsTerminal(isTerminal bool) {
 	s.isTerminal = isTerminal
 }