~emersion/hut-dev

hut: Ensure pager shutdown on error v1 PROPOSED

Conrad Hoffmann: 1
 Ensure pager shutdown on error

 15 files changed, 44 insertions(+), 15 deletions(-)
#1038538 .build.yml success
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?



          
          
          
        
      

      
      
      
      
      
      

      

      
      
      
      
    
Export patchset (mbox)
How do I use this?

Copy & paste the following snippet into your terminal to import this patchset into git:

curl -s https://lists.sr.ht/~emersion/hut-dev/patches/43467/mbox | git am -3
Learn more about email & git

[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
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
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.