Fix loading commits with very long subjects (#3533)

- **PR Description** This PR fixes a problem with lazygit locking up completely when there's a commit whose subject line is longer than approximately 65500 characters. Fixes #3529.
2025-05-11 12:25:47 +02:00 · 2024-05-15 13:29:28 +02:00 · 2024-05-15 13:29:28 +02:00 · d8b3c0e568
commit d8b3c0e568
parent 5558d873af f69eb6dc48
7 changed files with 149 additions and 23 deletions
--- 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`
--- 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,
--- 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
 	}
@ -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))
--- 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()

--- 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{})
--- 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
+	}
+}
--- 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)
+	}
+}