From 5e6af77b7ad7a0e76e08134e12faf30d9d1eab55 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Mar 2021 22:50:52 +0100 Subject: [PATCH 1/4] Unify interactive terminal detection code Previously the progress bar / status update interval used stdoutIsTerminal to determine whether it is possible to update the progress bar or not. However, its implementation differed from the detection within the backup command which included additional checks to detect the presence of mintty on Windows. mintty behaves like a terminal but uses pipes for communication. This adds stdoutCanUpdateStatus() which calls the same terminal detection code used by backup. This ensures that all commands consistently switch between interactive and non-interactive terminal mode. stdoutIsTerminal() now also returns true whenever stdoutCanUpdateStatus() does so. This is required to properly handle the special case of mintty. --- cmd/restic/global.go | 15 +++++++++++---- cmd/restic/progress.go | 2 +- internal/ui/termstatus/status.go | 2 +- internal/ui/termstatus/terminal_unix.go | 4 ++-- internal/ui/termstatus/terminal_windows.go | 4 ++-- 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/cmd/restic/global.go b/cmd/restic/global.go index f5255e22b..8d02b228e 100644 --- a/cmd/restic/global.go +++ b/cmd/restic/global.go @@ -31,6 +31,7 @@ import ( "github.com/restic/restic/internal/repository" "github.com/restic/restic/internal/restic" "github.com/restic/restic/internal/textfile" + "github.com/restic/restic/internal/ui/termstatus" "github.com/restic/restic/internal/errors" @@ -142,7 +143,13 @@ func stdinIsTerminal() bool { } func stdoutIsTerminal() bool { - return terminal.IsTerminal(int(os.Stdout.Fd())) + // mintty on windows can use pipes which behave like a posix terminal, + // but which are not a terminal handle + return terminal.IsTerminal(int(os.Stdout.Fd())) || stdoutCanUpdateStatus() +} + +func stdoutCanUpdateStatus() bool { + return termstatus.CanUpdateStatus(os.Stdout.Fd()) } func stdoutTerminalWidth() int { @@ -159,7 +166,7 @@ func stdoutTerminalWidth() int { // program execution must revert changes to the terminal configuration itself. // The terminal configuration is only restored while reading a password. func restoreTerminal() { - if !stdoutIsTerminal() { + if !terminal.IsTerminal(int(os.Stdout.Fd())) { return } @@ -248,7 +255,7 @@ func PrintProgress(format string, args ...interface{}) { message = fmt.Sprintf(format, args...) if !(strings.HasSuffix(message, "\r") || strings.HasSuffix(message, "\n")) { - if stdoutIsTerminal() { + if stdoutCanUpdateStatus() { carriageControl = "\r" } else { carriageControl = "\n" @@ -256,7 +263,7 @@ func PrintProgress(format string, args ...interface{}) { message = fmt.Sprintf("%s%s", message, carriageControl) } - if stdoutIsTerminal() { + if stdoutCanUpdateStatus() { message = fmt.Sprintf("%s%s", ClearLine(), message) } diff --git a/cmd/restic/progress.go b/cmd/restic/progress.go index 62f2e6396..0c2a24271 100644 --- a/cmd/restic/progress.go +++ b/cmd/restic/progress.go @@ -20,7 +20,7 @@ func calculateProgressInterval(show bool) time.Duration { fps = 60 } interval = time.Duration(float64(time.Second) / fps) - } else if !stdoutIsTerminal() || !show { + } else if !stdoutCanUpdateStatus() || !show { interval = 0 } return interval diff --git a/internal/ui/termstatus/status.go b/internal/ui/termstatus/status.go index 6b50effbb..e275f5b7d 100644 --- a/internal/ui/termstatus/status.go +++ b/internal/ui/termstatus/status.go @@ -67,7 +67,7 @@ func New(wr io.Writer, errWriter io.Writer, disableStatus bool) *Terminal { return t } - if d, ok := wr.(fder); ok && canUpdateStatus(d.Fd()) { + if d, ok := wr.(fder); ok && CanUpdateStatus(d.Fd()) { // only use the fancy status code when we're running on a real terminal. t.canUpdateStatus = true t.fd = d.Fd() diff --git a/internal/ui/termstatus/terminal_unix.go b/internal/ui/termstatus/terminal_unix.go index 3f0061c01..67ce06b0b 100644 --- a/internal/ui/termstatus/terminal_unix.go +++ b/internal/ui/termstatus/terminal_unix.go @@ -20,9 +20,9 @@ func moveCursorUp(wr io.Writer, fd uintptr) func(io.Writer, uintptr, int) { return posixMoveCursorUp } -// canUpdateStatus returns true if status lines can be printed, the process +// CanUpdateStatus returns true if status lines can be printed, the process // output is not redirected to a file or pipe. -func canUpdateStatus(fd uintptr) bool { +func CanUpdateStatus(fd uintptr) bool { if !terminal.IsTerminal(int(fd)) { return false } diff --git a/internal/ui/termstatus/terminal_windows.go b/internal/ui/termstatus/terminal_windows.go index 723aebdff..478d3a8ce 100644 --- a/internal/ui/termstatus/terminal_windows.go +++ b/internal/ui/termstatus/terminal_windows.go @@ -80,9 +80,9 @@ func isPipe(fd uintptr) bool { return err == nil && typ == windows.FILE_TYPE_PIPE } -// canUpdateStatus returns true if status lines can be printed, the process +// CanUpdateStatus returns true if status lines can be printed, the process // output is not redirected to a file or pipe. -func canUpdateStatus(fd uintptr) bool { +func CanUpdateStatus(fd uintptr) bool { // easy case, the terminal is cmd or psh, without redirection if isWindowsTerminal(fd) { return true From 80564a9bc9905a73e095c44bd6a1f674096878d9 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Mar 2021 23:17:11 +0100 Subject: [PATCH 2/4] Properly detect mintty output redirection mintty on windows always uses pipes to connect stdout between processes and for the terminal output. The previous implementation always assumed that stdout connected to a pipe means that stdout is displayed on a mintty terminal. However, this detection breaks when using pipes to connect processes and for powershell which uses pipes when redirecting to a file. Now the pipe filename is queried and matched against the pattern used by msys / cygwin when connected to the terminal. In all other cases assume that a pipe is just a regular pipe. --- go.mod | 2 +- go.sum | 3 +- internal/ui/termstatus/terminal_windows.go | 35 ++++++++++++++++++++-- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 32c8d19ba..b27b58185 100644 --- a/go.mod +++ b/go.mod @@ -37,7 +37,7 @@ require ( golang.org/x/net v0.0.0-20200904194848-62affa334b73 golang.org/x/oauth2 v0.0.0-20200902213428-5d25da1a8d43 golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208 - golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4 + golang.org/x/sys v0.0.0-20210305230114-8fe3ee5dd75b golang.org/x/text v0.3.4 google.golang.org/api v0.32.0 gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b // indirect diff --git a/go.sum b/go.sum index 114b206b1..d03c1708d 100644 --- a/go.sum +++ b/go.sum @@ -379,8 +379,9 @@ golang.org/x/sys v0.0.0-20200803210538-64077c9b5642/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20200828194041-157a740278f4/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200905004654-be1d3432aa8f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20201101102859-da207088b7d1/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4 h1:myAQVi0cGEoqQVR5POX+8RR2mrocKqNN1hmeMqhX27k= golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20210305230114-8fe3ee5dd75b h1:ggRgirZABFolTmi3sn6Ivd9SipZwLedQ5wR0aAKnFxU= +golang.org/x/sys v0.0.0-20210305230114-8fe3ee5dd75b/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/term v0.0.0-20201117132131-f5c789dd3221 h1:/ZHdbVpdR/jk3g30/d4yUL0JU9kksj8+F/bnQUVLGDM= golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= diff --git a/internal/ui/termstatus/terminal_windows.go b/internal/ui/termstatus/terminal_windows.go index 478d3a8ce..f7217e35d 100644 --- a/internal/ui/termstatus/terminal_windows.go +++ b/internal/ui/termstatus/terminal_windows.go @@ -4,6 +4,7 @@ package termstatus import ( "io" + "strings" "syscall" "unsafe" @@ -80,6 +81,22 @@ func isPipe(fd uintptr) bool { return err == nil && typ == windows.FILE_TYPE_PIPE } +func getFileNameByHandle(fd uintptr) (string, error) { + type FILE_NAME_INFO struct { + FileNameLength int32 + FileName [windows.MAX_LONG_PATH]uint16 + } + + var fi FILE_NAME_INFO + err := windows.GetFileInformationByHandleEx(windows.Handle(fd), windows.FileNameInfo, (*byte)(unsafe.Pointer(&fi)), uint32(unsafe.Sizeof(fi))) + if err != nil { + return "", err + } + + filename := syscall.UTF16ToString(fi.FileName[:]) + return filename, nil +} + // CanUpdateStatus returns true if status lines can be printed, the process // output is not redirected to a file or pipe. func CanUpdateStatus(fd uintptr) bool { @@ -88,11 +105,23 @@ func CanUpdateStatus(fd uintptr) bool { return true } - // check that the output file type is a pipe (0x0003) + // pipes require special handling if !isPipe(fd) { return false } - // assume we're running in mintty/cygwin - return true + fn, err := getFileNameByHandle(fd) + if err != nil { + return false + } + + // inspired by https://github.com/RyanGlScott/mintty/blob/master/src/System/Console/MinTTY/Win32.hsc + // terminal: \msys-dd50a72ab4668b33-pty0-to-master + // pipe to cat: \msys-dd50a72ab4668b33-13244-pipe-0x16 + if (strings.HasPrefix(fn, "\\cygwin-") || strings.HasPrefix(fn, "\\msys-")) && + strings.Contains(fn, "-pty") && strings.HasSuffix(fn, "-master") { + return true + } + + return false } From 7cb8ea69ba386289c75cf7e98dbe894a030f9f39 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Wed, 10 Mar 2021 23:02:32 +0100 Subject: [PATCH 3/4] Add test to mintty pipe detection --- .../ui/termstatus/terminal_windows_test.go | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 internal/ui/termstatus/terminal_windows_test.go diff --git a/internal/ui/termstatus/terminal_windows_test.go b/internal/ui/termstatus/terminal_windows_test.go new file mode 100644 index 000000000..e6eb39dd5 --- /dev/null +++ b/internal/ui/termstatus/terminal_windows_test.go @@ -0,0 +1,30 @@ +package termstatus + +import ( + "syscall" + "testing" + + "golang.org/x/sys/windows" + + rtest "github.com/restic/restic/internal/test" +) + +func TestIsMinTTY(t *testing.T) { + for _, test := range []struct { + path string + result bool + }{ + {`\\.\pipe\msys-dd50a72ab4668b33-pty0-to-master`, true}, + {`\\.\pipe\msys-dd50a72ab4668b33-13244-pipe-0x16`, false}, + } { + filename, err := syscall.UTF16FromString(test.path) + rtest.OK(t, err) + handle, err := windows.CreateNamedPipe(&filename[0], windows.PIPE_ACCESS_DUPLEX, + windows.PIPE_TYPE_BYTE, 1, 1024, 1024, 0, nil) + rtest.OK(t, err) + defer windows.CloseHandle(handle) + + rtest.Assert(t, CanUpdateStatus(uintptr(handle)) == test.result, + "expected CanUpdateStatus(%v) == %v", test.path, test.result) + } +} From f65e1211d9663fd9d0becac25568a6f6379dd796 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 11 Apr 2021 20:01:37 +0200 Subject: [PATCH 4/4] Add changelog entry --- changelog/unreleased/issue-3111 | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changelog/unreleased/issue-3111 diff --git a/changelog/unreleased/issue-3111 b/changelog/unreleased/issue-3111 new file mode 100644 index 000000000..56ccc86b7 --- /dev/null +++ b/changelog/unreleased/issue-3111 @@ -0,0 +1,11 @@ +Bugfix: Fix terminal output redirection for powershell + +When redirecting the output of restic using powershell on Windows, the +output contained terminal escape characters. This has been fixed by +properly detecting the terminal type. + +In addition, the mintty terminal now shows progress output for the backup +command. + +https://github.com/restic/restic/issues/3111 +https://github.com/restic/restic/pull/3325