From 8080326526956d6e67b9f9053edcab7de0eb0e78 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 1 Apr 2023 20:11:42 +0200 Subject: [PATCH] cli/streams: minor refactoring and docs touch-ups - Touch-up GoDoc to better document each method, adding punctuation, and use doc-links where applicable. - SetRawTerminal(): change the order in which we check if a terminal is connected; check the local boolean first before checking if the NORAW env-var is set. - NewOut() / NewIn(); remove intermediate variables - Remove explicit use of the embedded "commonStream" to make the code slightly less verbose, and more "to the point". - Document the intended purpose of SetIsTerminal(), which was added in https://github.com/moby/moby/commit/b2551c619d30f163c3ab8c9ee53cdb09bfbeaa55 to be used in unit-tests. Signed-off-by: Sebastiaan van Stijn --- cli/streams/in.go | 27 ++++++++++++++++----------- cli/streams/out.go | 30 +++++++++++++++++++----------- cli/streams/stream.go | 13 +++++++------ 3 files changed, 42 insertions(+), 28 deletions(-) diff --git a/cli/streams/in.go b/cli/streams/in.go index 44b0de38ba..1a10f7c830 100644 --- a/cli/streams/in.go +++ b/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 } diff --git a/cli/streams/out.go b/cli/streams/out.go index 95e21464a8..2383b08576 100644 --- a/cli/streams/out.go +++ b/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,23 +20,29 @@ 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 } @@ -43,8 +50,9 @@ func (o *Out) GetTtySize() (uint, uint) { 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 } diff --git a/cli/streams/stream.go b/cli/streams/stream.go index 21f0e452ea..54c9cda0b2 100644 --- a/cli/streams/stream.go +++ b/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 }