Merge d22af869d0 into 37cf840879

Reported here:
https://github.com/walles/moar/issues/236#issuecomment-2282677792

.github/workflows/windows-ci.yml

@@ -18,4 +18,4 @@ jobs:
           go-version-file: "go.mod"
 
       - run: go build
-      - run: go test -timeout 30s ./...
+      - run: go test -timeout 60s ./...

go.mod

@@ -4,12 +4,13 @@ go 1.20
 
 require (
 	github.com/alecthomas/chroma/v2 v2.12.0
+	github.com/fsnotify/fsnotify v1.7.0
 	github.com/google/go-cmp v0.5.9
 	github.com/klauspost/compress v1.17.4
 	github.com/sirupsen/logrus v1.8.1
 	github.com/ulikunitz/xz v0.5.11
 	golang.org/x/exp v0.0.0-20240103183307-be819d1f06fc
-	golang.org/x/sys v0.1.0
+	golang.org/x/sys v0.4.0
 	golang.org/x/term v0.0.0-20210503060354-a79de5458b56
 	gotest.tools/v3 v3.3.0
 )

go.sum

@@ -7,6 +7,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/dlclark/regexp2 v1.10.0 h1:+/GIL799phkJqYW+3YbOd8LCcbHzT0Pbo8zl70MHsq0=
 github.com/dlclark/regexp2 v1.10.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8=
+github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA=
+github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM=
 github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
 github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
@@ -41,8 +43,8 @@ golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
-golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U=
-golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.4.0 h1:Zr2JFtRQNX3BCZ8YtxRE9hNJYC8J6I1MVbMg6owUp18=
+golang.org/x/sys v0.4.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20210503060354-a79de5458b56 h1:b8jxX3zqjpqb2LklXPzKSGJhzyxCOZSz8ncv8Nv+y7w=
 golang.org/x/term v0.0.0-20210503060354-a79de5458b56/go.mod h1:tfny5GFUkzUvx4ps4ajbZsCe5lw1metzhBm9T3x7oIY=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=

m/editor.go

@@ -0,0 +1,182 @@
+package m
+
+import (
+	"fmt"
+	"math"
+	"os"
+	"os/exec"
+	"runtime"
+	"strings"
+
+	log "github.com/sirupsen/logrus"
+	"github.com/walles/moar/m/linenumbers"
+)
+
+// Dump the reader lines into a read-only temp file and return the absolute file
+// name.
+func dumpToTempFile(reader *Reader) (string, error) {
+	tempFile, err := os.CreateTemp("", "moar-contents-")
+	if err != nil {
+		return "", err
+	}
+	defer tempFile.Close()
+
+	log.Debug("Dumping contents into: ", tempFile.Name())
+
+	lines, _ := reader.GetLines(linenumbers.LineNumber{}, math.MaxInt)
+	for _, line := range lines.lines {
+		_, err := tempFile.WriteString(line.raw + "\n")
+		if err != nil {
+			return "", err
+		}
+	}
+
+	// Ref: https://pkg.go.dev/os#Chmod
+	err = os.Chmod(tempFile.Name(), 0400)
+	if err != nil {
+		// Doesn't matter that much, but if it fails we should at least log it
+		log.Debug("Failed to make temp file ", tempFile.Name(), " read-only: ", err)
+	}
+
+	return tempFile.Name(), nil
+}
+
+// Check that the editor is executable
+func errUnlessExecutable(file string) error {
+	stat, err := os.Stat(file)
+	if err != nil {
+		return fmt.Errorf("Failed to stat %s: %w", file, err)
+	}
+
+	if runtime.GOOS == "windows" && strings.HasSuffix(strings.ToLower(file), ".exe") {
+		log.Debug(".exe file on Windows, assuming executable: ", file)
+		return nil
+	}
+
+	if stat.Mode()&0111 != 0 {
+		// Note that this check isn't perfect, it could still be executable but
+		// not by us. Corner case, let's just fail later in that case.
+		return nil
+	}
+
+	return fmt.Errorf("Not executable: %s", file)
+}
+
+func pickAnEditor() (string, string, error) {
+	// Get an editor setting from either VISUAL or EDITOR
+	editorEnv := "VISUAL"
+	editor := strings.TrimSpace(os.Getenv(editorEnv))
+	if editor == "" {
+		editorEnv := "EDITOR"
+		editor = strings.TrimSpace(os.Getenv(editorEnv))
+	}
+
+	if editor != "" {
+		return editor, editorEnv, nil
+	}
+
+	candidates := []string{
+		"vim", // This is a sucky default, but let's have it for compatibility with less
+		"nano",
+		"vi",
+	}
+
+	for _, candidate := range candidates {
+		fullPath, err := exec.LookPath(candidate)
+		log.Trace("Problem finding ", candidate, ": ", err)
+		if err != nil {
+			continue
+		}
+
+		err = errUnlessExecutable(fullPath)
+		log.Trace("Problem with executability of ", fullPath, ": ", err)
+		if err != nil {
+			continue
+		}
+
+		return candidate, "fallback list", nil
+	}
+
+	return "", "", fmt.Errorf("No editor found, tried: $VISUAL, $EDITOR, %s", strings.Join(candidates, ", "))
+}
+
+func handleEditingRequest(p *Pager) {
+	editor, editorEnv, err := pickAnEditor()
+	if err != nil {
+		log.Warn("Failed to find an editor: ", err)
+		return
+	}
+
+	// Tyre kicking check that we can find the editor either in the PATH or as
+	// an absolute path
+	firstWord := strings.Fields(editor)[0]
+	editorPath, err := exec.LookPath(firstWord)
+	if err != nil {
+		// FIXME: Show a message in the status bar instead? Nothing wrong with
+		// moar here.
+		log.Warn("Failed to find editor "+firstWord+" from $"+editorEnv+": ", err)
+		return
+	}
+
+	// Check that the editor is executable
+	err = errUnlessExecutable(editorPath)
+	if err != nil {
+		// FIXME: Show a message in the status bar instead? Nothing wrong with
+		// moar here.
+		log.Warn("Editor from {} not executable: {}", editorEnv, err)
+		return
+	}
+
+	canOpenFile := p.reader.fileName != nil
+	if p.reader.fileName != nil {
+		// Verify that the file exists and is readable
+		err = tryOpen(*p.reader.fileName)
+		if err != nil {
+			canOpenFile = false
+			log.Info("File to edit is not readable: ", err)
+		}
+	}
+
+	var fileToEdit string
+	if canOpenFile {
+		fileToEdit = *p.reader.fileName
+	} else {
+		// NOTE: Let's not wait for the stream to finish, just dump whatever we
+		// have and open the editor on that. The user just asked for it, if they
+		// wanted to wait, they should have done that themselves.
+
+		// Create a temp file based on reader contents
+		fileToEdit, err = dumpToTempFile(p.reader)
+		if err != nil {
+			log.Warn("Failed to create temp file to edit: ", err)
+			return
+		}
+	}
+
+	p.AfterExit = func() error {
+		// NOTE: If you do any changes here, make sure they work with both "nano"
+		// and "code -w" (VSCode).
+		commandWithArgs := strings.Fields(editor)
+		commandWithArgs = append(commandWithArgs, fileToEdit)
+
+		log.Info("'v' pressed, launching editor: ", commandWithArgs)
+		command := exec.Command(commandWithArgs[0], commandWithArgs[1:]...)
+
+		// Since os.Stdin might come from a pipe, we can't trust that. Instead,
+		// we tell the editor to read from os.Stdout, which points to the
+		// terminal as well.
+		//
+		// Tested on macOS and Linux, works like a charm.
+		command.Stdin = os.Stdout // <- YES, WE SHOULD ASSIGN STDOUT TO STDIN
+
+		command.Stdout = os.Stdout
+		command.Stderr = os.Stderr
+
+		err := command.Run()
+		if err == nil {
+			log.Info("Editor exited successfully: ", commandWithArgs)
+		}
+		return err
+	}
+	p.Quit()
+}

m/highlight.go

@@ -28,10 +28,14 @@ func highlight(text string, style chroma.Style, formatter chroma.Formatter, lexe
 		return nil, nil
 	}
 
-	// See: https://github.com/alecthomas/chroma#identifying-the-language
-	// FIXME: Do we actually need this? We should profile our reader performance
-	// with and without.
-	lexer = chroma.Coalesce(lexer)
+	// NOTE: We used to do...
+	//
+	//   lexer = chroma.Coalesce(lexer)
+	//
+	// ... here, but with Chroma 2.12.0 that resulted in this problem:
+	// https://github.com/walles/moar/issues/236#issuecomment-2282677792
+	//
+	// So let's not do that anymore.
 
 	iterator, err := lexer.Tokenise(nil, text)
 	if err != nil {

m/pager.go

@@ -266,10 +266,11 @@ func (p *Pager) handleScrolledUp() {
 
 func (p *Pager) handleScrolledDown() {
 	if p.isScrolledToEnd() {
+		// Follow output
 		reallyHigh := linenumbers.LineNumberMax()
 		p.TargetLineNumber = &reallyHigh
 	} else {
-		p.TargetLineNumber = &linenumbers.LineNumber{}
+		p.TargetLineNumber = nil
 	}
 }
 
@@ -321,6 +322,10 @@ func (p *Pager) StartPaging(screen twin.Screen, chromaStyle *chroma.Style, chrom
 	p.marks = make(map[rune]scrollPosition)
 
 	go func() {
+		defer func() {
+			panicHandler("StartPaging()/moreLinesAvailable", recover())
+		}()
+
 		for range p.reader.moreLinesAdded {
 			// Notify the main loop about the new lines so it can show them
 			screen.Events() <- eventMoreLinesAvailable{}
@@ -336,6 +341,10 @@ func (p *Pager) StartPaging(screen twin.Screen, chromaStyle *chroma.Style, chrom
 	}()
 
 	go func() {
+		defer func() {
+			panicHandler("StartPaging()/spinner", recover())
+		}()
+
 		// Spin the spinner as long as contents is still loading
 		spinnerFrames := [...]string{"/.\\", "-o-", "\\O/", "| |"}
 		spinnerIndex := 0
@@ -358,6 +367,10 @@ func (p *Pager) StartPaging(screen twin.Screen, chromaStyle *chroma.Style, chrom
 	}()
 
 	go func() {
+		defer func() {
+			panicHandler("StartPaging()/maybeDone", recover())
+		}()
+
 		for range p.reader.maybeDone {
 			screen.Events() <- eventMaybeDone{}
 		}

m/pagermode-viewing.go

@@ -1,15 +1,7 @@
 package m
 
 import (
-	"fmt"
-	"math"
-	"os"
-	"os/exec"
-	"runtime"
-	"strings"
-
 	log "github.com/sirupsen/logrus"
-	"github.com/walles/moar/m/linenumbers"
 	"github.com/walles/moar/twin"
 )
 
@@ -77,145 +69,6 @@ func (m PagerModeViewing) onKey(keyCode twin.KeyCode) {
 	}
 }
 
-// Dump the reader lines into a read-only temp file and return the absolute file
-// name.
-func dumpToTempFile(reader *Reader) (string, error) {
-	tempFile, err := os.CreateTemp("", "moar-contents-")
-	if err != nil {
-		return "", err
-	}
-	defer tempFile.Close()
-
-	log.Debug("Dumping contents into: ", tempFile.Name())
-
-	lines, _ := reader.GetLines(linenumbers.LineNumber{}, math.MaxInt)
-	for _, line := range lines.lines {
-		_, err := tempFile.WriteString(line.raw + "\n")
-		if err != nil {
-			return "", err
-		}
-	}
-
-	// Ref: https://pkg.go.dev/os#Chmod
-	err = os.Chmod(tempFile.Name(), 0400)
-	if err != nil {
-		// Doesn't matter that much, but if it fails we should at least log it
-		log.Debug("Failed to make temp file ", tempFile.Name(), " read-only: ", err)
-	}
-
-	return tempFile.Name(), nil
-}
-
-// Check that the editor is executable
-func errUnlessExecutable(file string) error {
-	stat, err := os.Stat(file)
-	if err != nil {
-		return fmt.Errorf("Failed to stat %s: %w", file, err)
-	}
-
-	if runtime.GOOS == "windows" && strings.HasSuffix(strings.ToLower(file), ".exe") {
-		log.Debug(".exe file on Windows, assuming executable: ", file)
-		return nil
-	}
-
-	if stat.Mode()&0111 != 0 {
-		// Note that this check isn't perfect, it could still be executable but
-		// not by us. Corner case, let's just fail later in that case.
-		return nil
-	}
-
-	return fmt.Errorf("Not executable: %s", file)
-}
-
-func handleEditingRequest(p *Pager) {
-	// Get an editor setting from either VISUAL or EDITOR
-	editorEnv := "VISUAL"
-	editor := strings.TrimSpace(os.Getenv(editorEnv))
-	if editor == "" {
-		editorEnv := "EDITOR"
-		editor = strings.TrimSpace(os.Getenv(editorEnv))
-	}
-	if editor == "" {
-		// FIXME: Show a message in the status bar instead? Nothing wrong with
-		// moar here.
-		log.Warn("Neither $VISUAL nor $EDITOR are set, can't launch any editor")
-		return
-	}
-
-	// Tyre kicking check that we can find the editor either in the PATH or as
-	// an absolute path
-	firstWord := strings.Fields(editor)[0]
-	editorPath, err := exec.LookPath(firstWord)
-	if err != nil {
-		// FIXME: Show a message in the status bar instead? Nothing wrong with
-		// moar here.
-		log.Warn("Failed to find editor "+firstWord+" from $"+editorEnv+": ", err)
-		return
-	}
-
-	// Check that the editor is executable
-	err = errUnlessExecutable(editorPath)
-	if err != nil {
-		// FIXME: Show a message in the status bar instead? Nothing wrong with
-		// moar here.
-		log.Warn("Editor not executable: {}", err)
-		return
-	}
-
-	canOpenFile := p.reader.fileName != nil
-	if p.reader.fileName != nil {
-		// Verify that the file exists and is readable
-		err = tryOpen(*p.reader.fileName)
-		if err != nil {
-			canOpenFile = false
-			log.Info("File to edit is not readable: ", err)
-		}
-	}
-
-	var fileToEdit string
-	if canOpenFile {
-		fileToEdit = *p.reader.fileName
-	} else {
-		// NOTE: Let's not wait for the stream to finish, just dump whatever we
-		// have and open the editor on that. The user just asked for it, if they
-		// wanted to wait, they should have done that themselves.
-
-		// Create a temp file based on reader contents
-		fileToEdit, err = dumpToTempFile(p.reader)
-		if err != nil {
-			log.Warn("Failed to create temp file to edit: ", err)
-			return
-		}
-	}
-
-	p.AfterExit = func() error {
-		// NOTE: If you do any changes here, make sure they work with both "nano"
-		// and "code -w" (VSCode).
-		commandWithArgs := strings.Fields(editor)
-		commandWithArgs = append(commandWithArgs, fileToEdit)
-
-		log.Info("'v' pressed, launching editor: ", commandWithArgs)
-		command := exec.Command(commandWithArgs[0], commandWithArgs[1:]...)
-
-		// Since os.Stdin might come from a pipe, we can't trust that. Instead,
-		// we tell the editor to read from os.Stdout, which points to the
-		// terminal as well.
-		//
-		// Tested on macOS and Linux, works like a charm.
-		command.Stdin = os.Stdout // <- YES, WE SHOULD ASSIGN STDOUT TO STDIN
-
-		command.Stdout = os.Stdout
-		command.Stderr = os.Stderr
-
-		err := command.Run()
-		if err == nil {
-			log.Info("Editor exited successfully: ", commandWithArgs)
-		}
-		return err
-	}
-	p.Quit()
-}
-
 func (m PagerModeViewing) onRune(char rune) {
 	p := m.pager
 

m/panicHandler.go

@@ -0,0 +1,17 @@
+package m
+
+// NOTE: This file should be identical to twin/panicHandler.go
+
+import (
+	log "github.com/sirupsen/logrus"
+)
+
+func panicHandler(goroutineName string, recoverResult any) {
+	if recoverResult == nil {
+		return
+	}
+
+	log.WithFields(log.Fields{
+		"recoverResult": recoverResult,
+	}).Error("Goroutine panicked: " + goroutineName)
+}

m/reader.go

@@ -12,6 +12,7 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/fsnotify/fsnotify"
 	"github.com/walles/moar/m/linenumbers"
 
 	"github.com/alecthomas/chroma/v2"
@@ -239,11 +240,39 @@ func (reader *Reader) tailFile() error {
 
 	log.Debugf("Tailing file %s", *fileName)
 
+	watcher, err := fsnotify.NewWatcher()
+	if err != nil {
+		log.Debugf("Failed to create watcher for %s, giving up: %s", *fileName, err.Error())
+		return nil
+	}
+	err = watcher.Add(*fileName)
+	if err != nil {
+		log.Debugf("Failed to add %s to watcher for tailing, giving up: %s", *fileName, err.Error())
+		return nil
+	}
+
+	// Make sure to not miss any events while we were setting up the watcher
+	firstLap := true
+
 	for {
-		// NOTE: We could use something like
-		// https://github.com/fsnotify/fsnotify instead of sleeping and polling
-		// here.
-		time.Sleep(1 * time.Second)
+		if !firstLap {
+			select {
+			case event, ok := <-watcher.Events:
+				if !ok {
+					log.Debug("Watcher closed, giving up on tailing")
+					return nil
+				}
+				log.Trace("File event received: ", event.String())
+			case err, ok := <-watcher.Errors:
+				if !ok {
+					log.Debug("Watcher closed, giving up on tailing")
+					return nil
+				}
+				log.Debugf("Watcher error, giving up on tailing: %s", err.Error())
+				return nil
+			}
+		}
+		firstLap = false
 
 		fileStats, err := os.Stat(*fileName)
 		if err != nil {
@@ -342,7 +371,7 @@ func NewReaderFromStream(name string, reader io.Reader, style chroma.Style, form
 // then used for pre-allocating the lines slice, which improves large file
 // loading performance.
 //
-// If lexer is not nil, the file will be highlighted after being fully read.
+// If lexer is set, the file will be highlighted after being fully read.
 //
 // Note that you must call reader.SetStyleForHighlighting() after this to get
 // highlighting.
@@ -364,9 +393,13 @@ func newReaderFromStream(reader io.Reader, originalFileName *string, formatter c
 		done:                    &done,
 	}
 
-	// FIXME: Make sure that if we panic somewhere inside of this goroutine,
-	// the main program terminates and prints our panic stack trace.
-	go returnMe.readStream(reader, formatter, lexer)
+	go func() {
+		defer func() {
+			panicHandler("newReaderFromStream()/readStream()", recover())
+		}()
+
+		returnMe.readStream(reader, formatter, lexer)
+	}()
 
 	return &returnMe
 }

m/search.go

@@ -124,6 +124,10 @@ func (p *Pager) findFirstHit(startPosition linenumbers.LineNumber, beforePositio
 		}
 
 		go func(i int, searchStart linenumbers.LineNumber, chunkBefore *linenumbers.LineNumber) {
+			defer func() {
+				panicHandler("findFirstHit()/chunkSearch", recover())
+			}()
+
 			findings[i] <- p._findFirstHit(searchStart, chunkBefore, backwards)
 		}(i, searchStart, chunkBefore)
 	}

twin/panicHandler.go

@@ -0,0 +1,17 @@
+package twin
+
+// NOTE: This file should be identical to m/panicHandler.go
+
+import (
+	log "github.com/sirupsen/logrus"
+)
+
+func panicHandler(goroutineName string, recoverResult any) {
+	if recoverResult == nil {
+		return
+	}
+
+	log.WithFields(log.Fields{
+		"recoverResult": recoverResult,
+	}).Error("Goroutine panicked: " + goroutineName)
+}

twin/screen-setup.go

@@ -117,6 +117,10 @@ func (screen *UnixScreen) setupSigwinchNotification() {
 	sigwinch := make(chan os.Signal, 1)
 	signal.Notify(sigwinch, syscall.SIGWINCH)
 	go func() {
+		defer func() {
+			panicHandler("setupSigwinchNotification()/SIGWINCH", recover())
+		}()
+
 		for {
 			// Await window resize signal
 			<-sigwinch

twin/screen-setup_test.go

@@ -31,6 +31,10 @@ func TestInterruptableReader_blockedOnReadImmediate(t *testing.T) {
 	}
 	readResultChan := make(chan readResult)
 	go func() {
+		defer func() {
+			panicHandler("TestInterruptableReader_blockedOnReadImmediate()", recover())
+		}()
+
 		buffer := make([]byte, 1)
 		n, err := testMe.Read(buffer)
 		readResultChan <- readResult{n, err}

twin/screen.go

@@ -179,7 +179,13 @@ func NewScreenWithMouseModeAndColorType(mouseMode MouseMode, terminalColorCount
 
 	screen.hideCursor(true)
 
-	go screen.mainLoop()
+	go func() {
+		defer func() {
+			panicHandler("NewScreenWithMouseModeAndColorType()/mainLoop()", recover())
+		}()
+
+		screen.mainLoop()
+	}()
 
 	return &screen, nil
 }

twin/screen_test.go

@@ -269,6 +269,10 @@ func TestInterruptableReader_blockedOnRead(t *testing.T) {
 	}
 	readResultChan := make(chan readResult)
 	go func() {
+		defer func() {
+			panicHandler("TestInterruptableReader_blockedOnRead()", recover())
+		}()
+
 		buffer := make([]byte, 1)
 		n, err := testMe.Read(buffer)
 		readResultChan <- readResult{n, err}