Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Limit the max line length when parsing git grep output #30418

Merged
merged 2 commits into from
Apr 12, 2024
Merged

Limit the max line length when parsing git grep output #30418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2180467
fix
wxiaoguang Apr 11, 2024
a80e544
Merge branch 'main' into fix-git-grep
wxiaoguang Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions modules/git/grep.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"errors"
"fmt"
"os"
"slices"
"strconv"
"strings"

Expand All @@ -27,6 +28,7 @@ type GrepOptions struct {
MaxResultLimit int
ContextLineNumber int
IsFuzzy bool
MaxLineLength int // the maximum length of a line to parse, exceeding chars will be truncated
}

func GrepSearch(ctx context.Context, repo *Repository, search string, opts GrepOptions) ([]*GrepResult, error) {
Expand Down Expand Up @@ -71,10 +73,20 @@ func GrepSearch(ctx context.Context, repo *Repository, search string, opts GrepO
defer stdoutReader.Close()

isInBlock := false
scanner := bufio.NewScanner(stdoutReader)
rd := bufio.NewReaderSize(stdoutReader, util.IfZero(opts.MaxLineLength, 16*1024))
var res *GrepResult
for scanner.Scan() {
line := scanner.Text()
for {
lineBytes, isPrefix, err := rd.ReadLine()
if isPrefix {
lineBytes = slices.Clone(lineBytes)
for isPrefix && err == nil {
_, isPrefix, err = rd.ReadLine()
}
}
if len(lineBytes) == 0 && err != nil {
break
}
Comment on lines +86 to +88
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pity that we now have to treat err != nil as a successful case.
I mean, it does somewhat make sense, but at the same time it probably leads to false negative results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Actually I have thought about this change and I think it should be good enough.

The real cases for returning "err" in this "grep" functions:

  1. Command fails, the error is returned by "cmd.Run"
  2. Pipe fails (this case), I think it would seldom happen in real life. Even if happens, returning partially parsed results is still better than showing a 500 error page.

line := string(lineBytes) // the memory of lineBytes is mutable
if !isInBlock {
if _ /* ref */, filename, ok := strings.Cut(line, ":"); ok {
isInBlock = true
Expand All @@ -100,7 +112,7 @@ func GrepSearch(ctx context.Context, repo *Repository, search string, opts GrepO
res.LineCodes = append(res.LineCodes, lineCode)
}
}
return scanner.Err()
return nil
},
})
// git grep exits by cancel (killed), usually it is caused by the limit of results
Expand Down
10 changes: 10 additions & 0 deletions modules/git/grep_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ func TestGrepSearch(t *testing.T) {
},
}, res)

res, err = GrepSearch(context.Background(), repo, "void", GrepOptions{MaxResultLimit: 1, MaxLineLength: 39})
assert.NoError(t, err)
assert.Equal(t, []*GrepResult{
{
Filename: "java-hello/main.java",
LineNumbers: []int{3},
LineCodes: []string{" public static void main(String[] arg"},
},
}, res)

res, err = GrepSearch(context.Background(), repo, "no-such-content", GrepOptions{})
assert.NoError(t, err)
assert.Len(t, res, 0)
Expand Down