Fix showing diffs for renamed file when filtering by path

When filtering for a file path we use the --follow option for "git log", so it
will follow renames of the file, which is great. However, if you then selected
one of the commits before a rename, you didn't see a diff, because we would pass
the original filter path to the "git show" call.

To fix this, call git log with the --name-status option when filtering by path,
so that each commit reports which file paths are touched in this commit;
remember these in the commit object, so that we can pass them to the "git show"
call later.

Be careful not to store too many such paths unnecessarily. When filtering by
folder rather than file, all these paths will necessarily be inside the original
filter path, so detect this and don't store them in this case.

There is some unfortunate code duplication between loading commits and loading
reflog commits, which I am too lazy to clean up right now.

Author: stefanhaller

pkg/commands/git_commands/commit.go

@@ -253,7 +253,7 @@ func (self *CommitCommands) AmendHeadCmdObj() *oscommands.CmdObj {
 	return self.cmd.New(cmdArgs)
 }
 
-func (self *CommitCommands) ShowCmdObj(hash string, filterPath string) *oscommands.CmdObj {
+func (self *CommitCommands) ShowCmdObj(hash string, filterPaths []string) *oscommands.CmdObj {
 	contextSize := self.UserConfig().Git.DiffContextSize
 
 	extDiffCmd := self.UserConfig().Git.Paging.ExternalDiffCommand
@@ -271,7 +271,7 @@ func (self *CommitCommands) ShowCmdObj(hash string, filterPath string) *oscomman
 		ArgIf(self.UserConfig().Git.IgnoreWhitespaceInDiffView, "--ignore-all-space").
 		Arg(fmt.Sprintf("--find-renames=%d%%", self.UserConfig().Git.RenameSimilarityThreshold)).
 		Arg("--").
-		ArgIf(filterPath != "", filterPath).
+		Arg(filterPaths...).
 		Dir(self.repoPaths.worktreePath).
 		ToArgv()
 

pkg/commands/git_commands/commit_loader.go

@@ -90,7 +90,7 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit,
 		defer wg.Done()
 
 		var realCommits []*models.Commit
-		realCommits, logErr = loadCommits(self.getLogCmd(opts), func(line string) (*models.Commit, bool) {
+		realCommits, logErr = loadCommits(self.getLogCmd(opts), opts.FilterPath, func(line string) (*models.Commit, bool) {
 			return self.extractCommitFromLine(opts.HashPool, line, opts.RefToShowDivergenceFrom != ""), false
 		})
 		if logErr == nil {
@@ -294,7 +294,10 @@ func (self *CommitLoader) getHydratedTodoCommits(hashPool *utils.StringPool, tod
 
 	fullCommits := map[string]*models.Commit{}
 	err := cmdObj.RunAndProcessLines(func(line string) (bool, error) {
-		commit := self.extractCommitFromLine(hashPool, line, false)
+		if line == "" || line[0] != '+' {
+			return false, nil
+		}
+		commit := self.extractCommitFromLine(hashPool, line[1:], false)
 		fullCommits[commit.Hash()] = commit
 		return false, nil
 	})
@@ -601,7 +604,7 @@ func (self *CommitLoader) getLogCmd(opts GetCommitsOptions) *oscommands.CmdObj {
 		Arg("--abbrev=40").
 		ArgIf(opts.FilterAuthor != "", "--author="+opts.FilterAuthor).
 		ArgIf(opts.Limit, "-300").
-		ArgIf(opts.FilterPath != "", "--follow").
+		ArgIf(opts.FilterPath != "", "--follow", "--name-status").
 		Arg("--no-show-signature").
 		ArgIf(opts.RefToShowDivergenceFrom != "", "--left-right").
 		Arg("--").
@@ -611,4 +614,4 @@ func (self *CommitLoader) getLogCmd(opts GetCommitsOptions) *oscommands.CmdObj {
 	return self.cmd.New(cmdArgs).DontLog()
 }
 
-const prettyFormat = `--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s`
+const prettyFormat = `--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s`

pkg/commands/git_commands/commit_loader_test.go

@@ -15,16 +15,16 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-var commitsOutput = strings.ReplaceAll(`0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|[email protected]|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode
-b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164|1640824515|Jesse Duffield|[email protected]|origin/better-tests|e94e8fc5b6fab4cb755f|>|fix logging
-e94e8fc5b6fab4cb755f29f1bdb3ee5e001df35c|1640823749|Jesse Duffield|[email protected]|tag: 123, tag: 456|d8084cd558925eb7c9c3|>|refactor
-d8084cd558925eb7c9c38afeed5725c21653ab90|1640821426|Jesse Duffield|[email protected]||65f910ebd85283b5cce9|>|WIP
-65f910ebd85283b5cce9bf67d03d3f1a9ea3813a|1640821275|Jesse Duffield|[email protected]||26c07b1ab33860a1a759|>|WIP
-26c07b1ab33860a1a7591a0638f9925ccf497ffa|1640750752|Jesse Duffield|[email protected]||3d4470a6c072208722e5|>|WIP
-3d4470a6c072208722e5ae9a54bcb9634959a1c5|1640748818|Jesse Duffield|[email protected]||053a66a7be3da43aacdc|>|WIP
-053a66a7be3da43aacdc7aa78e1fe757b82c4dd2|1640739815|Jesse Duffield|[email protected]||985fe482e806b172aea4|>|refactoring the config struct`, "|", "\x00")
+var commitsOutput = strings.ReplaceAll(`+0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|[email protected]|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode
++b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164|1640824515|Jesse Duffield|[email protected]|origin/better-tests|e94e8fc5b6fab4cb755f|>|fix logging
++e94e8fc5b6fab4cb755f29f1bdb3ee5e001df35c|1640823749|Jesse Duffield|[email protected]|tag: 123, tag: 456|d8084cd558925eb7c9c3|>|refactor
++d8084cd558925eb7c9c38afeed5725c21653ab90|1640821426|Jesse Duffield|[email protected]||65f910ebd85283b5cce9|>|WIP
++65f910ebd85283b5cce9bf67d03d3f1a9ea3813a|1640821275|Jesse Duffield|[email protected]||26c07b1ab33860a1a759|>|WIP
++26c07b1ab33860a1a7591a0638f9925ccf497ffa|1640750752|Jesse Duffield|[email protected]||3d4470a6c072208722e5|>|WIP
++3d4470a6c072208722e5ae9a54bcb9634959a1c5|1640748818|Jesse Duffield|[email protected]||053a66a7be3da43aacdc|>|WIP
++053a66a7be3da43aacdc7aa78e1fe757b82c4dd2|1640739815|Jesse Duffield|[email protected]||985fe482e806b172aea4|>|refactoring the config struct`, "|", "\x00")
 
-var singleCommitOutput = strings.ReplaceAll(`0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|[email protected]|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode`, "|", "\x00")
+var singleCommitOutput = strings.ReplaceAll(`+0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|[email protected]|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode`, "|", "\x00")
 
 func TestGetCommits(t *testing.T) {
 	type scenario struct {
@@ -44,7 +44,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%m%x00%s", "--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),
 
 			expectedCommitOpts: []models.NewCommitOpts{},
 			expectedError:      nil,
@@ -55,7 +55,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%m%x00%s", "--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),
 
 			expectedCommitOpts: []models.NewCommitOpts{},
 			expectedError:      nil,
@@ -69,7 +69,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%m%x00%s", "--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
@@ -205,7 +205,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%m%x00%s", "--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")).
@@ -241,7 +241,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%m%x00%s", "--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")).
@@ -276,7 +276,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%m%x00%s", "--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),
 
 			expectedCommitOpts: []models.NewCommitOpts{},
 			expectedError:      nil,
@@ -287,7 +287,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%m%x00%s", "--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", "--name-status", "--no-show-signature", "--", "src"}, "", nil),
 
 			expectedCommitOpts: []models.NewCommitOpts{},
 			expectedError:      nil,

pkg/commands/git_commands/commit_loading_shared.go

@@ -1,23 +1,74 @@
 package git_commands
 
 import (
+	"strings"
+
 	"github.com/jesseduffield/lazygit/pkg/commands/models"
 	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
+	"github.com/samber/lo"
 )
 
 func loadCommits(
 	cmd *oscommands.CmdObj,
+	filterPath string,
 	parseLogLine func(string) (*models.Commit, bool),
 ) ([]*models.Commit, error) {
 	commits := []*models.Commit{}
 
+	var commit *models.Commit
+	var filterPaths []string
+	// A string pool that stores interned strings to reduce memory usage
+	pool := make(map[string]string)
+
+	finishLastCommit := func() {
+		if commit != nil {
+			// Only set the filter paths if we have one that is not contained in the original
+			// filter path. When filtering on a directory, all file paths will start with that
+			// directory, so we needn't bother storing the individual paths. Likewise, if we
+			// filter on a file and the file path hasn't changed, we needn't store it either.
+			// Only if a file has been moved or renamed do we need to store the paths, but then
+			// we need them all so that we can properly render a diff for the rename.
+			if lo.SomeBy(filterPaths, func(path string) bool {
+				return !strings.HasPrefix(path, filterPath)
+			}) {
+				commit.FilterPaths = lo.Map(filterPaths, func(path string, _ int) string {
+					if v, ok := pool[path]; ok {
+						return v
+					}
+					pool[path] = path
+					return path
+				})
+			}
+			commits = append(commits, commit)
+			commit = nil
+			filterPaths = nil
+		}
+	}
 	err := cmd.RunAndProcessLines(func(line string) (bool, error) {
-		commit, stop := parseLogLine(line)
-		if stop {
-			return true, nil
+		if line == "" {
+			return false, nil
+		}
+
+		if line[0] == '+' {
+			finishLastCommit()
+			var stop bool
+			commit, stop = parseLogLine(line[1:])
+			if stop {
+				commit = nil
+				return true, nil
+			}
+		} else if commit != nil && filterPath != "" {
+			// We are filtering by path, and this line is the output of the --name-status flag
+			fields := strings.Split(line, "\t")
+			// We don't bother looking at the first field (it will be 'A', 'M', 'R072' or a bunch of others).
+			// All we care about is the path(s), and there will be one for 'M' and 'A', and two for 'R' or 'C',
+			// in which case we want them both so that we can show the diff between the two.
+			if len(fields) > 1 {
+				filterPaths = append(filterPaths, fields[1:]...)
+			}
 		}
-		commits = append(commits, commit)
 		return false, nil
 	})
+	finishLastCommit()
 	return commits, err
 }

pkg/commands/git_commands/commit_test.go

@@ -251,7 +251,7 @@ func TestCommitCreateAmendCommit(t *testing.T) {
 func TestCommitShowCmdObj(t *testing.T) {
 	type scenario struct {
 		testName            string
-		filterPath          string
+		filterPaths         []string
 		contextSize         uint64
 		similarityThreshold int
 		ignoreWhitespace    bool
@@ -262,7 +262,7 @@ func TestCommitShowCmdObj(t *testing.T) {
 	scenarios := []scenario{
 		{
 			testName:            "Default case without filter path",
-			filterPath:          "",
+			filterPaths:         []string{},
 			contextSize:         3,
 			similarityThreshold: 50,
 			ignoreWhitespace:    false,
@@ -271,7 +271,7 @@ func TestCommitShowCmdObj(t *testing.T) {
 		},
 		{
 			testName:            "Default case with filter path",
-			filterPath:          "file.txt",
+			filterPaths:         []string{"file.txt"},
 			contextSize:         3,
 			similarityThreshold: 50,
 			ignoreWhitespace:    false,
@@ -280,7 +280,7 @@ func TestCommitShowCmdObj(t *testing.T) {
 		},
 		{
 			testName:            "Show diff with custom context size",
-			filterPath:          "",
+			filterPaths:         []string{},
 			contextSize:         77,
 			similarityThreshold: 50,
 			ignoreWhitespace:    false,
@@ -289,7 +289,7 @@ func TestCommitShowCmdObj(t *testing.T) {
 		},
 		{
 			testName:            "Show diff with custom similarity threshold",
-			filterPath:          "",
+			filterPaths:         []string{},
 			contextSize:         3,
 			similarityThreshold: 33,
 			ignoreWhitespace:    false,
@@ -298,7 +298,7 @@ func TestCommitShowCmdObj(t *testing.T) {
 		},
 		{
 			testName:            "Show diff, ignoring whitespace",
-			filterPath:          "",
+			filterPaths:         []string{},
 			contextSize:         77,
 			similarityThreshold: 50,
 			ignoreWhitespace:    true,
@@ -307,7 +307,7 @@ func TestCommitShowCmdObj(t *testing.T) {
 		},
 		{
 			testName:            "Show diff with external diff command",
-			filterPath:          "",
+			filterPaths:         []string{},
 			contextSize:         3,
 			similarityThreshold: 50,
 			ignoreWhitespace:    false,
@@ -330,7 +330,7 @@ func TestCommitShowCmdObj(t *testing.T) {
 			}
 			instance := buildCommitCommands(commonDeps{userConfig: userConfig, appState: &config.AppState{}, runner: runner, repoPaths: &repoPaths})
 
-			assert.NoError(t, instance.ShowCmdObj("1234567890", s.filterPath).Run())
+			assert.NoError(t, instance.ShowCmdObj("1234567890", s.filterPaths).Run())
 			runner.CheckForMissingCalls()
 		})
 	}

pkg/commands/git_commands/reflog_commit_loader.go

@@ -28,16 +28,16 @@ func (self *ReflogCommitLoader) GetReflogCommits(hashPool *utils.StringPool, las
 	cmdArgs := NewGitCmd("log").
 		Config("log.showSignature=false").
 		Arg("-g").
-		Arg("--format=%H%x00%ct%x00%gs%x00%P").
+		Arg("--format=+%H%x00%ct%x00%gs%x00%P").
 		ArgIf(filterAuthor != "", "--author="+filterAuthor).
-		ArgIf(filterPath != "", "--follow", "--", filterPath).
+		ArgIf(filterPath != "", "--follow", "--name-status", "--", filterPath).
 		ToArgv()
 
 	cmdObj := self.cmd.New(cmdArgs).DontLog()
 
 	onlyObtainedNewReflogCommits := false
 
-	commits, err := loadCommits(cmdObj, func(line string) (*models.Commit, bool) {
+	commits, err := loadCommits(cmdObj, filterPath, func(line string) (*models.Commit, bool) {
 		commit, ok := self.parseLine(hashPool, line)
 		if !ok {
 			return nil, false