From 6bb8c180b2c44911775cd045b8c90a916de738d9 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 27 Apr 2024 15:55:30 +0200 Subject: [PATCH 1/4] Handle scanner error in RunAndProcessLines Scanners can return errors (e.g. ErrTooLong), and if we don't handle it, the cmd.Wait() call below will block forever because nobody drains the command's output. This happens for CommitLoader.GetCommits when there's a commit whose subject line is longer than approx. 65500 characters; in that case, lazygit would lock up completely. With this fix it remains usable, but the commit list is truncated before the bad commit, which is not good enough. We'll improve that in the remaining commits of this branch. --- pkg/commands/oscommands/cmd_obj_runner.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/commands/oscommands/cmd_obj_runner.go b/pkg/commands/oscommands/cmd_obj_runner.go index 296850e32..5f28196e3 100644 --- a/pkg/commands/oscommands/cmd_obj_runner.go +++ b/pkg/commands/oscommands/cmd_obj_runner.go @@ -178,6 +178,11 @@ func (self *cmdObjRunner) RunAndProcessLines(cmdObj ICmdObj, onLine func(line st } } + if scanner.Err() != nil { + _ = Kill(cmd) + return scanner.Err() + } + _ = cmd.Wait() self.log.Infof("%s (%s)", cmdObj.ToString(), time.Since(t)) From 66d0ce841c9d89bbca3663e4c51b59120838d074 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 27 Apr 2024 17:16:06 +0200 Subject: [PATCH 2/4] Implement ScanLinesAndTruncateWhenLongerThanBuffer --- pkg/utils/lines.go | 59 ++++++++++++++++++++++++++++++++++++- pkg/utils/lines_test.go | 64 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 1 deletion(-) diff --git a/pkg/utils/lines.go b/pkg/utils/lines.go index 662ba2f9b..c70d02ffc 100644 --- a/pkg/utils/lines.go +++ b/pkg/utils/lines.go @@ -1,6 +1,9 @@ package utils -import "strings" +import ( + "bytes" + "strings" +) // SplitLines takes a multiline string and splits it on newlines // currently we are also stripping \r's which may have adverse effects for @@ -43,3 +46,57 @@ func EscapeSpecialChars(str string) string { "\v", "\\v", ).Replace(str) } + +func dropCR(data []byte) []byte { + if len(data) > 0 && data[len(data)-1] == '\r' { + return data[0 : len(data)-1] + } + return data +} + +// ScanLinesAndTruncateWhenLongerThanBuffer returns a split function that can be +// used with bufio.Scanner.Split(). It is very similar to bufio.ScanLines, +// except that it will truncate lines that are longer than the scanner's read +// buffer (whereas bufio.ScanLines will return an error in that case, which is +// often difficult to handle). +// +// If you are using your own buffer for the scanner, you must set maxBufferSize +// to the same value as the max parameter that you passed to scanner.Buffer(). +// Otherwise, maxBufferSize must be set to bufio.MaxScanTokenSize. +func ScanLinesAndTruncateWhenLongerThanBuffer(maxBufferSize int) func(data []byte, atEOF bool) (int, []byte, error) { + skipOverRemainderOfLongLine := false + + return func(data []byte, atEOF bool) (int, []byte, error) { + if atEOF && len(data) == 0 { + // Done + return 0, nil, nil + } + if i := bytes.IndexByte(data, '\n'); i >= 0 { + if skipOverRemainderOfLongLine { + skipOverRemainderOfLongLine = false + return i + 1, nil, nil + } + return i + 1, dropCR(data[0:i]), nil + } + if atEOF { + if skipOverRemainderOfLongLine { + return len(data), nil, nil + } + + return len(data), dropCR(data), nil + } + + // Buffer is full, so we can't get more data + if len(data) >= maxBufferSize { + if skipOverRemainderOfLongLine { + return len(data), nil, nil + } + + skipOverRemainderOfLongLine = true + return len(data), data, nil + } + + // Request more data. + return 0, nil, nil + } +} diff --git a/pkg/utils/lines_test.go b/pkg/utils/lines_test.go index e7171022b..2192a3780 100644 --- a/pkg/utils/lines_test.go +++ b/pkg/utils/lines_test.go @@ -1,6 +1,8 @@ package utils import ( + "bufio" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -100,3 +102,65 @@ func TestNormalizeLinefeeds(t *testing.T) { assert.EqualValues(t, string(s.expected), NormalizeLinefeeds(string(s.byteArray))) } } + +func TestScanLinesAndTruncateWhenLongerThanBuffer(t *testing.T) { + type scenario struct { + input string + expectedLines []string + } + + scenarios := []scenario{ + { + "", + []string{}, + }, + { + "\n", + []string{""}, + }, + { + "abc", + []string{"abc"}, + }, + { + "abc\ndef", + []string{"abc", "def"}, + }, + { + "abc\n\ndef", + []string{"abc", "", "def"}, + }, + { + "abc\r\ndef\r", + []string{"abc", "def"}, + }, + { + "abcdef", + []string{"abcde"}, + }, + { + "abcdef\n", + []string{"abcde"}, + }, + { + "abcdef\nghijkl\nx", + []string{"abcde", "ghijk", "x"}, + }, + { + "abc\ndefghijklmnopqrstuvw\nx", + []string{"abc", "defgh", "x"}, + }, + } + + for _, s := range scenarios { + scanner := bufio.NewScanner(strings.NewReader(s.input)) + scanner.Buffer(make([]byte, 5), 5) + scanner.Split(ScanLinesAndTruncateWhenLongerThanBuffer(5)) + result := []string{} + for scanner.Scan() { + result = append(result, scanner.Text()) + } + assert.NoError(t, scanner.Err()) + assert.EqualValues(t, s.expectedLines, result) + } +} From e0a2d97f0f5fc1afd6796883ad5f848bf2e1ff87 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 27 Apr 2024 17:22:36 +0200 Subject: [PATCH 3/4] Put subject last in git log's prettyFormat We are going to truncate overly long lines returned from git log, and the most likely field that is going to make the line too long is the subject; so we must put it last, otherwise we'd end up with not enough fields to split when it's too long. It might not be obvious from the diff what's happening to the mock command output in the test: it didn't have the divergence field (">") at all, which was kind of a bug. It didn't matter for these tests though, because we are not testing the divergence here, and our production code happens to be resilient against it missing. But now we must add the ">" field before the subject. --- pkg/commands/git_commands/commit_loader.go | 6 ++-- .../git_commands/commit_loader_test.go | 32 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 48545edb9..3c01344d9 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -211,11 +211,11 @@ func (self *CommitLoader) extractCommitFromLine(line string, showDivergence bool authorEmail := split[3] extraInfo := strings.TrimSpace(split[4]) parentHashes := split[5] - message := split[6] divergence := models.DivergenceNone if showDivergence { - divergence = lo.Ternary(split[7] == "<", models.DivergenceLeft, models.DivergenceRight) + divergence = lo.Ternary(split[6] == "<", models.DivergenceLeft, models.DivergenceRight) } + message := split[7] tags := []string{} @@ -605,4 +605,4 @@ func (self *CommitLoader) getLogCmd(opts GetCommitsOptions) oscommands.ICmdObj { return self.cmd.New(cmdArgs).DontLog() } -const prettyFormat = `--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%s%x00%m` +const prettyFormat = `--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%m%x00%s` diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index 7f60209a4..d9aa8456c 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -15,16 +15,16 @@ import ( "github.com/stretchr/testify/assert" ) -var commitsOutput = strings.Replace(`0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|jessedduffield@gmail.com|HEAD -> better-tests|b21997d6b4cbdf84b149|better typing for rebase mode -b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164|1640824515|Jesse Duffield|jessedduffield@gmail.com|origin/better-tests|e94e8fc5b6fab4cb755f|fix logging -e94e8fc5b6fab4cb755f29f1bdb3ee5e001df35c|1640823749|Jesse Duffield|jessedduffield@gmail.com|tag: 123, tag: 456|d8084cd558925eb7c9c3|refactor -d8084cd558925eb7c9c38afeed5725c21653ab90|1640821426|Jesse Duffield|jessedduffield@gmail.com||65f910ebd85283b5cce9|WIP -65f910ebd85283b5cce9bf67d03d3f1a9ea3813a|1640821275|Jesse Duffield|jessedduffield@gmail.com||26c07b1ab33860a1a759|WIP -26c07b1ab33860a1a7591a0638f9925ccf497ffa|1640750752|Jesse Duffield|jessedduffield@gmail.com||3d4470a6c072208722e5|WIP -3d4470a6c072208722e5ae9a54bcb9634959a1c5|1640748818|Jesse Duffield|jessedduffield@gmail.com||053a66a7be3da43aacdc|WIP -053a66a7be3da43aacdc7aa78e1fe757b82c4dd2|1640739815|Jesse Duffield|jessedduffield@gmail.com||985fe482e806b172aea4|refactoring the config struct`, "|", "\x00", -1) +var commitsOutput = strings.Replace(`0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|jessedduffield@gmail.com|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode +b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164|1640824515|Jesse Duffield|jessedduffield@gmail.com|origin/better-tests|e94e8fc5b6fab4cb755f|>|fix logging +e94e8fc5b6fab4cb755f29f1bdb3ee5e001df35c|1640823749|Jesse Duffield|jessedduffield@gmail.com|tag: 123, tag: 456|d8084cd558925eb7c9c3|>|refactor +d8084cd558925eb7c9c38afeed5725c21653ab90|1640821426|Jesse Duffield|jessedduffield@gmail.com||65f910ebd85283b5cce9|>|WIP +65f910ebd85283b5cce9bf67d03d3f1a9ea3813a|1640821275|Jesse Duffield|jessedduffield@gmail.com||26c07b1ab33860a1a759|>|WIP +26c07b1ab33860a1a7591a0638f9925ccf497ffa|1640750752|Jesse Duffield|jessedduffield@gmail.com||3d4470a6c072208722e5|>|WIP +3d4470a6c072208722e5ae9a54bcb9634959a1c5|1640748818|Jesse Duffield|jessedduffield@gmail.com||053a66a7be3da43aacdc|>|WIP +053a66a7be3da43aacdc7aa78e1fe757b82c4dd2|1640739815|Jesse Duffield|jessedduffield@gmail.com||985fe482e806b172aea4|>|refactoring the config struct`, "|", "\x00", -1) -var singleCommitOutput = strings.Replace(`0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|jessedduffield@gmail.com|HEAD -> better-tests|b21997d6b4cbdf84b149|better typing for rebase mode`, "|", "\x00", -1) +var singleCommitOutput = strings.Replace(`0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|jessedduffield@gmail.com|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode`, "|", "\x00", -1) func TestGetCommits(t *testing.T) { type scenario struct { @@ -46,7 +46,7 @@ func TestGetCommits(t *testing.T) { opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false}, runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). - ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%s%x00%m", "--abbrev=40", "--no-show-signature", "--"}, "", nil), + ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), expectedCommits: []*models.Commit{}, expectedError: nil, @@ -58,7 +58,7 @@ func TestGetCommits(t *testing.T) { opts: GetCommitsOptions{RefName: "refs/heads/mybranch", RefForPushedStatus: "refs/heads/mybranch", IncludeRebaseCommits: false}, runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). - ExpectGitArgs([]string{"log", "refs/heads/mybranch", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%s%x00%m", "--abbrev=40", "--no-show-signature", "--"}, "", nil), + ExpectGitArgs([]string{"log", "refs/heads/mybranch", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), expectedCommits: []*models.Commit{}, expectedError: nil, @@ -73,7 +73,7 @@ func TestGetCommits(t *testing.T) { // here it's seeing which commits are yet to be pushed ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). // here it's actually getting all the commits in a formatted form, one per line - ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%s%x00%m", "--abbrev=40", "--no-show-signature", "--"}, commitsOutput, nil). + ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, commitsOutput, nil). // here it's testing which of the configured main branches have an upstream ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "refs/remotes/origin/master", nil). // this one does ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "main@{u}"}, "", errors.New("error")). // this one doesn't, so it checks origin instead @@ -210,7 +210,7 @@ func TestGetCommits(t *testing.T) { // here it's seeing which commits are yet to be pushed ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). // here it's actually getting all the commits in a formatted form, one per line - ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%s%x00%m", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). + ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). // here it's testing which of the configured main branches exist; neither does ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "", errors.New("error")). ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/remotes/origin/master"}, "", errors.New("error")). @@ -247,7 +247,7 @@ func TestGetCommits(t *testing.T) { // here it's seeing which commits are yet to be pushed ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). // here it's actually getting all the commits in a formatted form, one per line - ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%s%x00%m", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). + ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). // here it's testing which of the configured main branches exist ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "refs/remotes/origin/master", nil). ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "main@{u}"}, "", errors.New("error")). @@ -283,7 +283,7 @@ func TestGetCommits(t *testing.T) { opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false}, runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). - ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%s%x00%m", "--abbrev=40", "--no-show-signature", "--"}, "", nil), + ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), expectedCommits: []*models.Commit{}, expectedError: nil, @@ -295,7 +295,7 @@ func TestGetCommits(t *testing.T) { opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", FilterPath: "src"}, runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). - ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%s%x00%m", "--abbrev=40", "--follow", "--no-show-signature", "--", "src"}, "", nil), + ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%m%x00%s", "--abbrev=40", "--follow", "--no-show-signature", "--", "src"}, "", nil), expectedCommits: []*models.Commit{}, expectedError: nil, From f69eb6dc4875e662cfc95abf1600bba57b52ff54 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 27 Apr 2024 17:19:08 +0200 Subject: [PATCH 4/4] Use ScanLinesAndTruncateWhenLongerThanBuffer instead of bufio.ScanLines --- pkg/commands/oscommands/cmd_obj_runner.go | 2 +- pkg/gui/mergeconflicts/find_conflicts.go | 2 +- pkg/tasks/tasks.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/commands/oscommands/cmd_obj_runner.go b/pkg/commands/oscommands/cmd_obj_runner.go index 5f28196e3..16257158e 100644 --- a/pkg/commands/oscommands/cmd_obj_runner.go +++ b/pkg/commands/oscommands/cmd_obj_runner.go @@ -161,7 +161,7 @@ func (self *cmdObjRunner) RunAndProcessLines(cmdObj ICmdObj, onLine func(line st } scanner := bufio.NewScanner(stdoutPipe) - scanner.Split(bufio.ScanLines) + scanner.Split(utils.ScanLinesAndTruncateWhenLongerThanBuffer(bufio.MaxScanTokenSize)) if err := cmd.Start(); err != nil { return err } diff --git a/pkg/gui/mergeconflicts/find_conflicts.go b/pkg/gui/mergeconflicts/find_conflicts.go index 3802a66b7..c4d3a51a8 100644 --- a/pkg/gui/mergeconflicts/find_conflicts.go +++ b/pkg/gui/mergeconflicts/find_conflicts.go @@ -99,7 +99,7 @@ func FileHasConflictMarkers(path string) (bool, error) { // Efficiently scans through a file looking for merge conflict markers. Returns true if it does func fileHasConflictMarkersAux(file io.Reader) bool { scanner := bufio.NewScanner(file) - scanner.Split(bufio.ScanLines) + scanner.Split(utils.ScanLinesAndTruncateWhenLongerThanBuffer(bufio.MaxScanTokenSize)) for scanner.Scan() { line := scanner.Bytes() diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 202c3f23a..e80b63a2a 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -162,7 +162,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p done := make(chan struct{}) scanner := bufio.NewScanner(r) - scanner.Split(bufio.ScanLines) + scanner.Split(utils.ScanLinesAndTruncateWhenLongerThanBuffer(bufio.MaxScanTokenSize)) lineChan := make(chan []byte) lineWrittenChan := make(chan struct{})