Repository: bitfield/script
Pull request #89 (2021-12-05 00:43:52)

Bug fixes and improvements for corner cases
When working on #88 , I noticed some defects in the current library, so I'd like to submit this separate patch, which does the following: 

**Fix the defect that Column() crashes on zero or a negative input**
The library panics when the following code is run
```go
script.Echo("1").Column(-1)
```
After the change, the program will return an empty pipe.

**Fix the defect that Last() crashes on a negative input**
The library panics when the following code is run
```go
script.Echo("1").Last(-1)
```
After the change, the program will return an empty pipe.

**Fix the defect that Regex filters crash on a nil regex expression**
The library panics when the following code is run
```go
script.Echo("1").MatchRegexp(nil)
```
After the change, the program will return a pipe with the error "nil regular expression"

**Add unit test for WithError(); close the pipe when a non-nil error is set**
When fixing the regex defect, I just wrote
```go
if re == nil { // to prevent SIGSEGV
	return p.WithError(errors.New("nil regular expression"))
}
```
Then, I realized that `p` will not be automatically closed, since the subsequent filters and sinks will catch the error and return immediately. To fix this problem and prevent contributors from making the same mistake in the future, I think the best approach is to just close the pipe reader when a non-nil error is set.

**Add ExecForEach, Reject(), and RejectRegexp to doMethodsOnPipe tests**
Just some jobs that previous contributors forgot.

**Improve coverage by adding equal counts in the test case of Freq**
To test that lines are sorted in alphabetical order when there is a tie.

**Use io.Copy() in Stdout() to improve efficiency**
It seems unnecessary to write the pipe's reader into a string and then print it.

By the way, after all the changes, the total line coverage has risen from 92.7% to 93.8%.

gty929: @bitfield Hi, would you please take a look at this pull request? Many thanks.

Patch:
diff --git a/filters.go b/filters.go
index a912114..a9154da 100644
--- a/filters.go
+++ b/filters.go
@@ -6,6 +6,7 @@ import (
 	"container/ring"
 	"crypto/sha256"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -41,7 +42,7 @@ func (p *Pipe) Basename() *Pipe {
 func (p *Pipe) Column(col int) *Pipe {
 	return p.EachLine(func(line string, out *strings.Builder) {
 		columns := strings.Fields(line)
-		if col <= len(columns) {
+		if col > 0 && col <= len(columns) {
 			out.WriteString(columns[col-1])
 			out.WriteRune('\n')
 		}
@@ -176,7 +177,7 @@ func (p *Pipe) First(lines int) *Pipe {
 		return p
 	}
 	defer p.Close()
-	if lines == 0 {
+	if lines <= 0 {
 		return NewPipe()
 	}
 	scanner := bufio.NewScanner(p.Reader)
@@ -263,7 +264,7 @@ func (p *Pipe) Last(lines int) *Pipe {
 		return p
 	}
 	defer p.Close()
-	if lines == 0 {
+	if lines <= 0 {
 		return NewPipe()
 	}
 	scanner := bufio.NewScanner(p.Reader)
@@ -303,6 +304,9 @@ func (p *Pipe) Match(s string) *Pipe {
 // that match the specified compiled regular expression. If there is an error
 // reading the pipe, the pipe's error status is also set.
 func (p *Pipe) MatchRegexp(re *regexp.Regexp) *Pipe {
+	if re == nil { // to prevent SIGSEGV
+		return p.WithError(errors.New("nil regular expression"))
+	}
 	return p.EachLine(func(line string, out *strings.Builder) {
 		if re.MatchString(line) {
 			out.WriteString(line)
@@ -327,6 +331,9 @@ func (p *Pipe) Reject(s string) *Pipe {
 // lines that don't match the specified compiled regular expression. If there
 // is an error reading the pipe, the pipe's error status is also set.
 func (p *Pipe) RejectRegexp(re *regexp.Regexp) *Pipe {
+	if re == nil { // to prevent SIGSEGV
+		return p.WithError(errors.New("nil regular expression"))
+	}
 	return p.EachLine(func(line string, out *strings.Builder) {
 		if !re.MatchString(line) {
 			out.WriteString(line)
@@ -351,6 +358,9 @@ func (p *Pipe) Replace(search, replace string) *Pipe {
 // represents the text of the first submatch. If there is an error reading the
 // pipe, the pipe's error status is also set.
 func (p *Pipe) ReplaceRegexp(re *regexp.Regexp, replace string) *Pipe {
+	if re == nil { // to prevent SIGSEGV
+		return p.WithError(errors.New("nil regular expression"))
+	}
 	return p.EachLine(func(line string, out *strings.Builder) {
 		out.WriteString(re.ReplaceAllString(line, replace))
 		out.WriteRune('\n')
diff --git a/pipes.go b/pipes.go
index ae56294..a442f7c 100644
--- a/pipes.go
+++ b/pipes.go
@@ -75,6 +75,9 @@ func (p *Pipe) Read(b []byte) (int, error) {
 // SetError sets the pipe's error status to the specified error.
 func (p *Pipe) SetError(err error) {
 	if p != nil {
+		if err != nil {
+			p.Close()
+		}
 		p.err = err
 	}
 }


Test patch:
diff --git a/filters_test.go b/filters_test.go
index 462b617..4ff83d0 100644
--- a/filters_test.go
+++ b/filters_test.go
@@ -51,6 +51,27 @@ func TestColumn(t *testing.T) {
 	if !bytes.Equal(got, want) {
 		t.Errorf("want %q, got %q", want, got)
 	}
+	got, err = script.File("testdata/column.input.txt").Column(0).Bytes()
+	if err != nil {
+		t.Error()
+	}
+	if !bytes.Equal(got, []byte{}) {
+		t.Errorf("want %q, got %q", want, got)
+	}
+	got, err = script.File("testdata/column.input.txt").Column(-1).Bytes()
+	if err != nil {
+		t.Error()
+	}
+	if !bytes.Equal(got, []byte{}) {
+		t.Errorf("want %q, got %q", want, got)
+	}
+	got, err = script.File("testdata/column.input.txt").Column(10).Bytes()
+	if err != nil {
+		t.Error()
+	}
+	if !bytes.Equal(got, []byte{}) {
+		t.Errorf("want %q, got %q", want, got)
+	}
 }
 
 func TestConcat(t *testing.T) {
@@ -232,6 +253,18 @@ func TestFirst(t *testing.T) {
 	if err == nil {
 		t.Error("input not closed after reading")
 	}
+	input = script.File("testdata/first.input.txt")
+	gotZero, err = input.First(-1).CountLines()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if gotZero != 0 {
+		t.Errorf("want 0 lines, got %d lines", gotZero)
+	}
+	_, err = ioutil.ReadAll(input.Reader)
+	if err == nil {
+		t.Error("input not closed after reading")
+	}
 	want, err = script.File("testdata/first.input.txt").Bytes()
 	if err != nil {
 		t.Fatal(err)
@@ -312,6 +345,18 @@ func TestLast(t *testing.T) {
 	if err == nil {
 		t.Error("input not closed after reading")
 	}
+	input = script.File("testdata/first.input.txt")
+	gotZero, err = input.Last(-1).CountLines()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if gotZero != 0 {
+		t.Errorf("want 0 lines, got %d lines", gotZero)
+	}
+	_, err = ioutil.ReadAll(input.Reader)
+	if err == nil {
+		t.Error("input not closed after reading")
+	}
 	want, err = script.File("testdata/first.input.txt").Bytes()
 	if err != nil {
 		t.Fatal(err)
@@ -350,6 +395,16 @@ func TestMatch(t *testing.T) {
 
 func TestMatchRegexp(t *testing.T) {
 	t.Parallel()
+	p := script.File("testdata/hello.txt")
+	_, err := p.MatchRegexp(nil).String()
+	if err == nil {
+		t.Error("nil regex should cause an error")
+	}
+	_, err = ioutil.ReadAll(p.Reader)
+	if err == nil {
+		t.Error("input not closed after reading")
+	}
+
 	testCases := []struct {
 		testFileName string
 		match        string
@@ -397,6 +452,16 @@ func TestReplace(t *testing.T) {
 
 func TestReplaceRegexp(t *testing.T) {
 	t.Parallel()
+	p := script.File("testdata/hello.txt")
+	_, err := p.ReplaceRegexp(nil, "").String()
+	if err == nil {
+		t.Error("nil regex should cause an error")
+	}
+	_, err = ioutil.ReadAll(p.Reader)
+	if err == nil {
+		t.Error("input not closed after reading")
+	}
+
 	testCases := []struct {
 		testFileName string
 		regexp       string
@@ -444,6 +509,16 @@ func TestReject(t *testing.T) {
 
 func TestRejectRegexp(t *testing.T) {
 	t.Parallel()
+	p := script.File("testdata/hello.txt")
+	_, err := p.RejectRegexp(nil).String()
+	if err == nil {
+		t.Error("nil regex should cause an error")
+	}
+	_, err = ioutil.ReadAll(p.Reader)
+	if err == nil {
+		t.Error("input not closed after reading")
+	}
+
 	testCases := []struct {
 		testFileName string
 		reject       string
diff --git a/pipes_test.go b/pipes_test.go
index 5e9d2e6..2947c42 100644
--- a/pipes_test.go
+++ b/pipes_test.go
@@ -26,6 +26,25 @@ func TestWithReader(t *testing.T) {
 	}
 }
 
+func TestWithError(t *testing.T) {
+	t.Parallel()
+	p := script.File("testdata/empty.txt")
+	want := "fake error"
+	_, gotErr := p.WithError(errors.New(want)).String()
+	if gotErr.Error() != "fake error" {
+		t.Errorf("want %q, got %q", want, gotErr.Error())
+	}
+	_, err := ioutil.ReadAll(p.Reader)
+	if err == nil {
+		t.Error("input not closed after reading")
+	}
+	p = script.File("testdata/empty.txt")
+	_, gotErr = p.WithError(nil).String()
+	if gotErr != nil {
+		t.Errorf("got unexpected error: %q", gotErr.Error())
+	}
+}
+
 func TestWithStdout(t *testing.T) {
 	t.Parallel()
 	buf := &bytes.Buffer{}
@@ -126,6 +145,8 @@ func doMethodsOnPipe(t *testing.T, p *script.Pipe, kind string) {
 	p.Error()
 	action = "Exec()"
 	p.Exec("bogus")
+	action = "ExecForEach()"
+	p.ExecForEach("bogus")
 	action = "ExitStatus()"
 	p.ExitStatus()
 	action = "First()"
@@ -142,6 +163,10 @@ func doMethodsOnPipe(t *testing.T, p *script.Pipe, kind string) {
 	p.MatchRegexp(regexp.MustCompile(".*"))
 	action = "Read()"
 	p.Read([]byte{})
+	action = "Reject()"
+	p.Reject("")
+	action = "RejectRegexp"
+	p.RejectRegexp(regexp.MustCompile(".*"))
 	action = "Replace()"
 	p.Replace("old", "new")
 	action = "ReplaceRegexp()"
diff --git a/testdata/freq.golden.txt b/testdata/freq.golden.txt
index ae81bc0..81fb31b 100644
--- a/testdata/freq.golden.txt
+++ b/testdata/freq.golden.txt
@@ -1,4 +1,4 @@
 10 apple
  4 banana
- 2 orange
+ 4 orange
  1 kumquat
diff --git a/testdata/freq.input.txt b/testdata/freq.input.txt
index 67762b1..ff80519 100644
--- a/testdata/freq.input.txt
+++ b/testdata/freq.input.txt
@@ -6,11 +6,13 @@ apple
 orange
 kumquat
 apple
+orange
 apple
 banana
 banana
 apple
 apple
+orange
 apple
 apple
 apple