hut: Ensure pager shutdown on error v1 PROPOSED

Conrad Hoffmann: 1
 Ensure pager shutdown on error

 15 files changed, 44 insertions(+), 15 deletions(-)

I do understand now, thanks. I do wonder, though: wouldn't it be easier 
and lot cleaner if we just change the pagerifyFn signature to return 
(bool, error) and go through every caller once to update it accordingly?

Simon Ser <contact@emersion.fr>

3 months ago

I wonder. It sounds cleaner indeed, however if we keep using log.Fatal
anywhere down the call stack we'll still hit the same issue. I don't
know if we do, and it's not trivial to figure out and prevent... I
suppose we mainly log.Fatal when parsing commands, which should be
outside of the pagerify func. I'd be fine with that solution.

Nit: instead of (bool, error), we could return just an error and use a
special guard value to stop, like filepath.Walk does.

[RFC PATCH hut] Ensure pager shutdown on error Export this patch

Less messes with the TTY settings, and if it does not get shut down
properly, the terminal might be messed up after a call to hut. This
happens on any error inside a call to `pagerify` that is handled by
`log.Fatal*()`, as deferred methods are not executed in this case.

Some shells (fish, zsh) seem unaffected, potentially because they are
more aggressive about resetting terminal properties after a command
executed. Both bash and dash are affected by the issue.

The solution is to properly close the pager in case of an error. Given
that `log.Fatal*()` is used all over the place, simply create a `log`
object in the main package that can be used instead and that forwards
any calls to the log package, after closing the pager if required.
---
I am well aware that this might not be the prettiest solution, it's
mostly meant as a way to start a discussion about this, and as a PoC
that it fixes the issue (it does). I am open to suggestions, I could
imagine e.g. a custom log package. But you may also have much better
ideas, so I'd like to solicit your feedback right away.

Note: had to bump to go 1.18 because "predeclared any requires go1.18 or
later".


 builds.go  |  1 -
 client.go  |  1 -
 config.go  |  1 -
 export.go  |  1 -
 git.go     |  1 -
 go.mod     |  2 +-
 graphql.go |  1 -
 hg.go      |  1 -
 lists.go   |  1 -
 main.go    |  1 -
 meta.go    |  1 -
 pager.go   | 44 +++++++++++++++++++++++++++++++++++++++++++-
 pages.go   |  1 -
 paste.go   |  1 -
 todo.go    |  1 -
 15 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/builds.go b/builds.go
index 687e211..a650d02 100644
--- a/builds.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"log"
 	"net/http"
 	"os"
 	"os/exec"
diff --git a/client.go b/client.go
index f4f7dec..883dbfa 100644
--- a/client.go
+++ b/client.go
@@ -2,7 +2,6 @@ package main
 
 import (
 	"fmt"
-	"log"
 	"net"
 	"net/http"
 	"os/exec"
diff --git a/config.go b/config.go
index 51023ac..28c0221 100644
--- a/config.go
+++ b/config.go
@@ -5,7 +5,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"log"
 	"os"
 	"path/filepath"
 	"strings"
diff --git a/export.go b/export.go
index 1c29b28..0eb8a87 100644
--- a/export.go
+++ b/export.go
@@ -2,7 +2,6 @@ package main
 
 import (
 	"encoding/json"
-	"log"
 	"os"
 	"path"
 	"time"
diff --git a/git.go b/git.go
index 744b1b9..1e5d34d 100644
--- a/git.go
+++ b/git.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"log"
 	"net/url"
 	"os"
 	"os/exec"
diff --git a/go.mod b/go.mod
index 217f1fe..7c90cb9 100644
--- a/go.mod
+++ b/go.mod
@@ -1,6 +1,6 @@
 module git.sr.ht/~emersion/hut
 
-go 1.17
+go 1.18
 
 require (
 	git.sr.ht/~emersion/go-scfg v0.0.0-20211215104734-c2c7a15d6c99
diff --git a/graphql.go b/graphql.go
index 7bd86f6..35f98bb 100644
--- a/graphql.go
+++ b/graphql.go
@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"log"
 	"os"
 	"strings"
 
diff --git a/hg.go b/hg.go
index 1112232..c13329c 100644
--- a/hg.go
+++ b/hg.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"log"
 	"strings"
 
 	"git.sr.ht/~emersion/hut/srht/hgsrht"
diff --git a/lists.go b/lists.go
index 26b891c..79146e2 100644
--- a/lists.go
+++ b/lists.go
@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"log"
 	"net/http"
 	"net/mail"
 	"os"
diff --git a/main.go b/main.go
index 33d9422..226315d 100644
--- a/main.go
+++ b/main.go
@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"log"
 	"os"
 	"os/exec"
 	"strconv"
diff --git a/meta.go b/meta.go
index 472f7e3..54e2636 100644
--- a/meta.go
+++ b/meta.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"log"
 	"os"
 	"os/exec"
 	"path/filepath"
diff --git a/pager.go b/pager.go
index 7b8b6d0..8bb752a 100644
--- a/pager.go
+++ b/pager.go
@@ -2,7 +2,7 @@ package main
 
 import (
 	"io"
-	"log"
+	golog "log"
 	"os"
 	"os/exec"
 
@@ -14,6 +14,45 @@ type pager interface {
 	Running() bool
 }
 
+type logger struct {
+	pager pager
+}
+
+var log logger
+
+func (l logger) Fatal(v ...any) {
+	if l.pager != nil {
+		l.pager.Close()
+	}
+	golog.Fatal(v...)
+}
+
+func (l logger) Fatalln(v ...any) {
+	if l.pager != nil {
+		l.pager.Close()
+	}
+	golog.Fatalln(v...)
+}
+
+func (l logger) Fatalf(format string, v ...any) {
+	if l.pager != nil {
+		l.pager.Close()
+	}
+	golog.Fatalf(format, v...)
+}
+
+func (l logger) Println(v ...any) {
+	golog.Println(v...)
+}
+
+func (l logger) Printf(format string, v ...any) {
+	golog.Printf(format, v...)
+}
+
+func (l logger) SetFlags(flag int) {
+	golog.SetFlags(flag)
+}
+
 func newPager() pager {
 	if !termfmt.IsTerminal() {
 		return &singleWritePager{os.Stdout, true}
@@ -50,6 +89,8 @@ func pagerify(fn pagerifyFn) {
 	pager := newPager()
 	defer pager.Close()
 
+	log.pager = pager
+
 	for pager.Running() {
 		shouldStop := fn(pager)
 		if shouldStop {
@@ -82,6 +123,7 @@ func (p *cmdPager) Close() error {
 		return err
 	}
 	<-p.done
+	log.pager = nil
 	return nil
 }
 
diff --git a/pages.go b/pages.go
index d2b01da..bc2789b 100644
--- a/pages.go
+++ b/pages.go
@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"io"
 	"io/fs"
-	"log"
 	"os"
 	"path/filepath"
 	"strings"
diff --git a/paste.go b/paste.go
index 74b95c5..ad66e01 100644
--- a/paste.go
+++ b/paste.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"log"
 	"mime"
 	"net/http"
 	"os"
diff --git a/todo.go b/todo.go
index 0c1f568..b9072d1 100644
--- a/todo.go
+++ b/todo.go
@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"log"
 	"math"
 	"os"
 	"strings"
-- 
2.41.0

builds.sr.ht <builds@sr.ht>

3 months ago

hut/patches/.build.yml: SUCCESS in 1m1s

[Ensure pager shutdown on error][0] from [Conrad Hoffmann][1]

[0]: https://lists.sr.ht/~emersion/hut-dev/patches/43467
[1]: mailto:ch@bitfehler.net

✓ #1038538 SUCCESS hut/patches/.build.yml https://builds.sr.ht/~emersion/job/1038538

Simon Ser <contact@emersion.fr>

3 months ago

I think we'll need a custom logger as done in this patch.

It should be possible to simplify this patch by embedding a logger:

    type logger struct {
        *golog.Logger
    }

using golog.Default, and overriding the Fatal family of methods.

An alternative would be to panic with, say, a errCommandFailed value,
and then recover in the main function and os.Exit(1) there. This would
have the upside of running all defer statements before exit, without
any pager-specific logic.