Repository: gokcehan/lf
Pull request #753 (2022-02-07 21:24:39)

Allow kill to work for rsync when run via %
I'm using rsync for my paste command:

```
cmd paste-rsync %{{
    set -- $(cat ~/.local/share/lf/files)
    mode="$1"; shift
    case "$mode" in
        copy) rsync -rltphv "$@" . ;;
        move) mv -- "$@" .; lf -remote "send clear" ;;
    esac
}}
```

However when running it via `%` I'm not able to kill the process (`cmd-interrupt` will send the kill signal to the shell process but not to its children). This is not an issue for `$` and `!`.

This PR places the process in its own process group on linux and macOS (where I am able to test) and `cmd-interrupt` is then able to send the exit signal to the whole group.

gokcehan: @laktak A patch for such a change should likely address #694 and #734 somehow. Also what's the situation with other unix systems? I don't really want to maintain platform specific patches.
laktak: Yes #694 is also discussing the process group of shell-async in addition to shell-pipe that is handled here. @slavistan do you have any input/would you be willing to test once shell-async is handled by this PR as well?

As for #734 - this is a separate issue IMO. Also there appears to be a simple workaround.

I understand that you don't want to implement platform specific patches. I think that this PR should work on all POSIX systems as it is documented for these systems. This patch would still not support Solaris, Illumos, and Windows - is that OK in this context?
gokcehan: @laktak Having separate windows is ok, that's why we have `os.go` and `os_windows.go` files. I'm not sure if we have any actual users with Solaris and Illumos but Github shows some downloads so we should not break the builds if possible for whatever it's worth. "syscall" in your patch seems to break the build on these systems. I think it is time we should switch to [golang.org/x/sys](https://pkg.go.dev/golang.org/x/sys) package. The old "syscall" package is frozen and deprecated anyway.
laktak: Thanks for pointing out the x/sys package! I was able to move the code into os/os_windows. The PR now also includes shell-async.

Patch:
diff --git a/app.go b/app.go
index 011eddb..751295c 100644
--- a/app.go
+++ b/app.go
@@ -483,6 +483,7 @@ func (app *app) runShell(s string, args []string, prefix string) {
 
 		err = cmd.Run()
 	case "%":
+		shellSetPG(cmd)
 		if app.ui.cmdPrefix == ">" {
 			return
 		}
@@ -499,6 +500,7 @@ func (app *app) runShell(s string, args []string, prefix string) {
 		cmd.Stderr = cmd.Stdout
 		fallthrough
 	case "&":
+		shellSetPG(cmd)
 		err = cmd.Start()
 	}
 
diff --git a/eval.go b/eval.go
index 597f1be..ba828cb 100644
--- a/eval.go
+++ b/eval.go
@@ -1772,7 +1772,12 @@ func (e *callExpr) eval(app *app, args []string) {
 		update(app)
 	case "cmd-interrupt":
 		if app.cmd != nil {
-			app.cmd.Process.Kill()
+			err := shellKill(app.cmd)
+			if err != nil {
+				app.ui.echoerrf("kill: %s", err)
+			} else {
+				app.ui.echoerr("process interrupt")
+			}
 		}
 		normal(app)
 	case "cmd-word":
diff --git a/go.mod b/go.mod
index ec1e3c6..635d7f7 100644
--- a/go.mod
+++ b/go.mod
@@ -5,6 +5,7 @@ go 1.12
 require (
 	github.com/gdamore/tcell/v2 v2.3.1
 	github.com/mattn/go-runewidth v0.0.10
+	golang.org/x/sys v0.0.0-20220209214540-3681064d5158 // indirect
 	golang.org/x/term v0.0.0-20210220032956-6a3ed077a48d
 	gopkg.in/djherbis/times.v1 v1.2.0
 )
diff --git a/go.sum b/go.sum
index 604e1f7..6041a84 100644
--- a/go.sum
+++ b/go.sum
@@ -10,6 +10,8 @@ github.com/rivo/uniseg v0.1.0 h1:+2KBaVoUmb9XzDsrx/Ct0W/EYOSFf/nWTauy++DprtY=
 github.com/rivo/uniseg v0.1.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc=
 golang.org/x/sys v0.0.0-20201119102817-f84b799fce68 h1:nxC68pudNYkKU6jWhgrqdreuFiOQWj1Fs7T3VrH4Pjw=
 golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20220209214540-3681064d5158 h1:rm+CHSpPEEW2IsXUib1ThaHIjuBVZjxNgSKmBLFfD4c=
+golang.org/x/sys v0.0.0-20220209214540-3681064d5158/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201210144234-2321bbc49cbf/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.0.0-20210220032956-6a3ed077a48d h1:SZxvLBoTP5yHO3Frd4z4vrF+DBX9vMVanchswa69toE=
 golang.org/x/term v0.0.0-20210220032956-6a3ed077a48d/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
diff --git a/misc.go b/misc.go
index 4aa8761..0f18f7a 100644
--- a/misc.go
+++ b/misc.go
@@ -2,7 +2,6 @@ package main
 
 import (
 	"bufio"
-	"errors"
 	"fmt"
 	"io"
 	"path/filepath"
@@ -136,23 +135,36 @@ func splitWord(s string) (word, rest string) {
 	return
 }
 
-var reComment = regexp.MustCompile(`#.*$`)
-var reTrailingSpace = regexp.MustCompile(`\s+$`)
-
+// This function reads whitespace separated string pairs at each line. Single
+// or double quotes can be used to escape whitespaces. Hash characters can be
+// used to add a comment until the end of line. Indentation and trailing space
+// is trimmed. Empty lines are skipped.
 func readPairs(r io.Reader) ([][]string, error) {
 	var pairs [][]string
 	s := bufio.NewScanner(r)
 	for s.Scan() {
 		line := s.Text()
 
-		line = reComment.ReplaceAllString(line, "")
-		line = reTrailingSpace.ReplaceAllString(line, "")
+		squote, dquote := false, false
+		for i := 0; i < len(line); i++ {
+			if line[i] == '\'' && !dquote {
+				squote = !squote
+			} else if line[i] == '"' && !squote {
+				dquote = !dquote
+			}
+			if !squote && !dquote && line[i] == '#' {
+				line = line[:i]
+				break
+			}
+		}
+
+		line = strings.TrimSpace(line)
 
 		if line == "" {
 			continue
 		}
 
-		squote, dquote := false, false
+		squote, dquote = false, false
 		pair := strings.FieldsFunc(line, func(r rune) bool {
 			if r == '\'' && !dquote {
 				squote = !squote
@@ -163,12 +175,11 @@ func readPairs(r io.Reader) ([][]string, error) {
 		})
 
 		if len(pair) != 2 {
-			return nil, errors.New(fmt.Sprintf("expected pair but found: %s", s.Text()))
-			continue
+			return nil, fmt.Errorf("expected pair but found: %s", s.Text())
 		}
 
 		for i := 0; i < len(pair); i++ {
-			squote, dquote := false, false
+			squote, dquote = false, false
 			buf := make([]rune, 0, len(pair[i]))
 			for _, r := range pair[i] {
 				if r == '\'' && !dquote {
diff --git a/os.go b/os.go
index 33125b7..ff9bc92 100644
--- a/os.go
+++ b/os.go
@@ -4,6 +4,7 @@ package main
 
 import (
 	"fmt"
+	"golang.org/x/sys/unix"
 	"log"
 	"os"
 	"os/exec"
@@ -11,7 +12,6 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
-	"syscall"
 )
 
 var (
@@ -110,7 +110,7 @@ func init() {
 
 func detachedCommand(name string, arg ...string) *exec.Cmd {
 	cmd := exec.Command(name, arg...)
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
+	cmd.SysProcAttr = &unix.SysProcAttr{Setsid: true}
 	return cmd
 }
 
@@ -126,6 +126,22 @@ func shellCommand(s string, args []string) *exec.Cmd {
 	return exec.Command(gOpts.shell, args...)
 }
 
+func shellSetPG(cmd *exec.Cmd) {
+	cmd.SysProcAttr = &unix.SysProcAttr{Setpgid: true}
+}
+
+func shellKill(cmd *exec.Cmd) error {
+	pgid, err := unix.Getpgid(cmd.Process.Pid)
+	if err == nil && cmd.Process.Pid == pgid {
+		// kill the process group
+		err = unix.Kill(-pgid, 15)
+		if err == nil {
+			return nil
+		}
+	}
+	return cmd.Process.Kill()
+}
+
 func setDefaults() {
 	gOpts.cmds["open"] = &execExpr{"&", `$OPENER "$f"`}
 	gOpts.keys["e"] = &execExpr{"$", `$EDITOR "$f"`}
@@ -137,7 +153,7 @@ func setDefaults() {
 }
 
 func setUserUmask() {
-	syscall.Umask(0077)
+	unix.Umask(0077)
 }
 
 func isExecutable(f os.FileInfo) bool {
@@ -158,7 +174,7 @@ func isHidden(f os.FileInfo, path string, hiddenfiles []string) bool {
 }
 
 func userName(f os.FileInfo) string {
-	if stat, ok := f.Sys().(*syscall.Stat_t); ok {
+	if stat, ok := f.Sys().(*unix.Stat_t); ok {
 		if u, err := user.LookupId(fmt.Sprint(stat.Uid)); err == nil {
 			return fmt.Sprintf("%v ", u.Username)
 		}
@@ -167,7 +183,7 @@ func userName(f os.FileInfo) string {
 }
 
 func groupName(f os.FileInfo) string {
-	if stat, ok := f.Sys().(*syscall.Stat_t); ok {
+	if stat, ok := f.Sys().(*unix.Stat_t); ok {
 		if g, err := user.LookupGroupId(fmt.Sprint(stat.Gid)); err == nil {
 			return fmt.Sprintf("%v ", g.Name)
 		}
@@ -176,14 +192,14 @@ func groupName(f os.FileInfo) string {
 }
 
 func linkCount(f os.FileInfo) string {
-	if stat, ok := f.Sys().(*syscall.Stat_t); ok {
+	if stat, ok := f.Sys().(*unix.Stat_t); ok {
 		return fmt.Sprintf("%v ", stat.Nlink)
 	}
 	return ""
 }
 
 func errCrossDevice(err error) bool {
-	return err.(*os.LinkError).Err.(syscall.Errno) == syscall.EXDEV
+	return err.(*os.LinkError).Err.(unix.Errno) == unix.EXDEV
 }
 
 func exportFiles(f string, fs []string, pwd string) {
diff --git a/os_windows.go b/os_windows.go
index 333c0ec..2a93163 100644
--- a/os_windows.go
+++ b/os_windows.go
@@ -2,13 +2,13 @@ package main
 
 import (
 	"fmt"
+	"golang.org/x/sys/windows"
 	"log"
 	"os"
 	"os/exec"
 	"os/user"
 	"path/filepath"
 	"strings"
-	"syscall"
 )
 
 var (
@@ -87,7 +87,7 @@ func init() {
 
 func detachedCommand(name string, arg ...string) *exec.Cmd {
 	cmd := exec.Command(name, arg...)
-	cmd.SysProcAttr = &syscall.SysProcAttr{CreationFlags: 8}
+	cmd.SysProcAttr = &windows.SysProcAttr{CreationFlags: 8}
 	return cmd
 }
 
@@ -99,6 +99,13 @@ func shellCommand(s string, args []string) *exec.Cmd {
 	return exec.Command(gOpts.shell, args...)
 }
 
+func shellSetPG(cmd *exec.Cmd) {
+}
+
+func shellKill(cmd *exec.Cmd) error {
+	return cmd.Process.Kill()
+}
+
 func setDefaults() {
 	gOpts.cmds["open"] = &execExpr{"&", "%OPENER% %f%"}
 	gOpts.keys["e"] = &execExpr{"$", "%EDITOR% %f%"}
@@ -123,15 +130,15 @@ func isExecutable(f os.FileInfo) bool {
 }
 
 func isHidden(f os.FileInfo, path string, hiddenfiles []string) bool {
-	ptr, err := syscall.UTF16PtrFromString(filepath.Join(path, f.Name()))
+	ptr, err := windows.UTF16PtrFromString(filepath.Join(path, f.Name()))
 	if err != nil {
 		return false
 	}
-	attrs, err := syscall.GetFileAttributes(ptr)
+	attrs, err := windows.GetFileAttributes(ptr)
 	if err != nil {
 		return false
 	}
-	return attrs&syscall.FILE_ATTRIBUTE_HIDDEN != 0
+	return attrs&windows.FILE_ATTRIBUTE_HIDDEN != 0
 }
 
 func userName(f os.FileInfo) string {
@@ -147,7 +154,7 @@ func linkCount(f os.FileInfo) string {
 }
 
 func errCrossDevice(err error) bool {
-	return err.(*os.LinkError).Err.(syscall.Errno) == 17
+	return err.(*os.LinkError).Err.(windows.Errno) == 17
 }
 
 func exportFiles(f string, fs []string, pwd string) {


Test patch:
diff --git a/misc_test.go b/misc_test.go
index 2541e04..6962b3b 100644
--- a/misc_test.go
+++ b/misc_test.go
@@ -3,6 +3,7 @@ package main
 import (
 	"os"
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -165,6 +166,28 @@ func TestSplitWord(t *testing.T) {
 	}
 }
 
+func TestReadPairs(t *testing.T) {
+	tests := []struct {
+		s   string
+		exp [][]string
+	}{
+		{"foo bar", [][]string{[]string{"foo", "bar"}}},
+		{"foo bar ", [][]string{[]string{"foo", "bar"}}},
+		{" foo bar", [][]string{[]string{"foo", "bar"}}},
+		{" foo bar ", [][]string{[]string{"foo", "bar"}}},
+		{"foo bar#baz", [][]string{[]string{"foo", "bar"}}},
+		{"foo bar #baz", [][]string{[]string{"foo", "bar"}}},
+		{`'foo#baz' bar`, [][]string{[]string{"foo#baz", "bar"}}},
+		{`"foo#baz" bar`, [][]string{[]string{"foo#baz", "bar"}}},
+	}
+
+	for _, test := range tests {
+		if got, _ := readPairs(strings.NewReader(test.s)); !reflect.DeepEqual(got, test.exp) {
+			t.Errorf("at input '%v' expected '%v' but got '%v'", test.s, test.exp, got)
+		}
+	}
+}
+
 func TestHumanize(t *testing.T) {
 	tests := []struct {
 		i   int64