Show renames when selecting a directory that a file was moved into or out of

pkg/commands/git_commands/working_tree.go

@@ -385,27 +385,22 @@ func (self *WorkingTreeCommands) Exclude(filename string) error {
 // WorktreeFileDiff returns the diff of a file
 func (self *WorkingTreeCommands) WorktreeFileDiff(file *models.File, plain bool, cached bool) string {
 	// for now we assume an error means the file was deleted
-	s, _ := self.WorktreeFileDiffCmdObj(file, plain, cached, nil).RunWithOutput()
+	s, _ := self.WorktreeFileDiffCmdObj(file, plain, cached, file.Names()).RunWithOutput()
 	return s
 }
 
-// WorktreeFileDiffCmdObj returns a command object for diffing a file or directory
-// in the working tree. When pathOverrides is non-empty, those paths are used instead of
-// the node's path (used to diff only filtered/visible files within a directory).
-func (self *WorkingTreeCommands) WorktreeFileDiffCmdObj(node models.IFile, plain bool, cached bool, pathOverrides []string) *oscommands.CmdObj {
+// WorktreeFileDiffCmdObj returns a command object for diffing the given paths
+// in the working tree. node is the item they belong to; all it decides is
+// whether git has to compare against /dev/null, which is the case for a file
+// that isn't in the index yet.
+func (self *WorkingTreeCommands) WorktreeFileDiffCmdObj(node models.IFile, plain bool, cached bool, paths []string) *oscommands.CmdObj {
 	colorArg := self.diffRendererConfigManager.GetColorArg()
 	if plain {
 		colorArg = "never"
 	}
 
-	prevPath := node.GetPreviousPath()
 	noIndex := !node.GetIsTracked() && !node.GetHasStagedChanges() && !cached && node.GetIsFile()
 
-	paths := pathOverrides
-	if len(paths) == 0 {
-		paths = []string{node.GetPath()}
-	}
-
 	cmdArgs := NewGitCmd("diff").
 		AddCommonDiffArgs(self.diffRendererConfigManager, self.UserConfig(), !plain).
 		Arg("--submodule").
@@ -415,7 +410,6 @@ func (self *WorkingTreeCommands) WorktreeFileDiffCmdObj(node models.IFile, plain
 		Arg("--").
 		ArgIf(noIndex, "/dev/null").
 		Arg(paths...).
-		ArgIf(prevPath != "", prevPath).
 		Dir(self.repoPaths.worktreePath).
 		ToArgv()
 

pkg/gui/controllers/commits_files_controller.go

@@ -616,24 +616,9 @@ func (self *CommitFilesController) GetOnClickFocusedMainView() func(mainViewName
 	}
 }
 
-// pathsForDiff returns the file paths to use for a diff command. When a text
-// filter is active and the node is a directory, only the visible (filtered)
-// file paths are returned so the diff reflects what the user sees.
 func (self *CommitFilesController) pathsForDiff(node *filetree.CommitFileNode) []string {
-	if !node.IsFile() && self.context().IsFiltering() {
-		var paths []string
-		_ = node.ForEachFile(func(file *models.CommitFile) error {
-			// For a rename we need to pass both paths so that git detects it as
-			// a rename rather than an unrelated delete and add.
-			paths = append(paths, file.Names()...)
-			return nil
-		})
-		return paths
-	}
-	if file := node.GetFile(); file != nil {
-		return file.Names()
-	}
-	return []string{node.GetPath()}
+	return diffPathsForNode(
+		node.Raw(), self.context().GetRoot().Raw(), self.c.Model().CommitFiles, self.context().IsFiltering())
 }
 
 // NOTE: these functions are identical to those in files_controller.go (except for types) and

pkg/gui/controllers/diff_paths.go

@@ -0,0 +1,132 @@
+package controllers
+
+import (
+	"path"
+	"strings"
+
+	"github.com/jesseduffield/lazygit/pkg/gui/filetree"
+	"github.com/samber/lo"
+)
+
+// Both models.File and models.CommitFile satisfy this. Names returns the file's
+// path, plus the path it was renamed from if it is a rename.
+type fileWithNames[T any] interface {
+	*T
+	GetPath() string
+	GetPreviousPath() string
+	Names() []string
+}
+
+// diffPathsForNode returns the paths to limit a diff command to for showing the
+// changes of the given node. files are all the files that the diff contains,
+// while root is the root of the tree the node belongs to, which holds only the
+// files matching the text filter when there is one.
+func diffPathsForNode[T any, PT fileWithNames[T]](node *filetree.Node[T], root *filetree.Node[T], files []*T, isFiltering bool) []string {
+	if file := node.GetFile(); file != nil {
+		return PT(file).Names()
+	}
+
+	dir := node.GetPath()
+
+	if isFiltering {
+		// Passing the directory would bring back the files that the filter hides,
+		// so we spell out the ones it leaves.
+		var paths []string
+		for _, file := range filesInDir[T, PT](filesInTree(root), dir) {
+			paths = append(paths, PT(file).Names()...)
+		}
+		return paths
+	}
+
+	// The directory covers everything below it, but git only pairs up the two
+	// ends of a rename if both are in the pathspec, and one end can well be
+	// outside the directory. Without that end we would get an addition or a
+	// deletion where the diff has a rename.
+	var outsidePaths []string
+	for _, f := range filesInDir[T, PT](files, dir) {
+		file := PT(f)
+		if p := file.GetPath(); !isInDir(p, dir) {
+			outsidePaths = append(outsidePaths, p)
+		}
+		if p := file.GetPreviousPath(); p != "" && !isInDir(p, dir) {
+			outsidePaths = append(outsidePaths, p)
+		}
+	}
+
+	return dropContainedPaths(append([]string{dir}, collapseToDirs[T, PT](outsidePaths, files, dir)...))
+}
+
+// dropContainedPaths removes the paths that another one of them contains, since
+// a pathspec that matches a directory matches everything below it anyway.
+func dropContainedPaths(paths []string) []string {
+	return lo.Filter(paths, func(p string, _ int) bool {
+		return !lo.SomeBy(paths, func(other string) bool {
+			return other != p && isInDir(p, other)
+		})
+	})
+}
+
+// collapseToDirs replaces each of the given paths with the highest directory
+// that can stand in for it, so that moving a whole directory elsewhere costs a
+// single pathspec rather than one per file. There is a limit to how long a
+// command line may get, and a commit can move a great many files at once.
+func collapseToDirs[T any, PT fileWithNames[T]](paths []string, files []*T, dir string) []string {
+	if len(paths) == 0 {
+		return nil
+	}
+
+	// A directory can stand in for the paths under it as long as everything it
+	// contains ends up in the diff anyway, which is to say as long as all of it
+	// is in the directory we are diffing too.
+	canStandIn := make(map[string]bool)
+	standsIn := func(candidate string) bool {
+		if result, ok := canStandIn[candidate]; ok {
+			return result
+		}
+
+		result := lo.EveryBy(files, func(file *T) bool {
+			return !fileIsInDir[T, PT](file, candidate) || fileIsInDir[T, PT](file, dir)
+		})
+		canStandIn[candidate] = result
+		return result
+	}
+
+	return lo.Uniq(lo.Map(paths, func(p string, _ int) string {
+		// A directory that can't stand in for the path rules out its parents
+		// too, since they contain everything it contains. We stop short of the
+		// repository root: it would leave the command with nothing to say about
+		// the directory whose diff we are showing.
+		for candidate := path.Dir(p); candidate != "." && standsIn(candidate); candidate = path.Dir(candidate) {
+			p = candidate
+		}
+		return p
+	}))
+}
+
+func filesInTree[T any](root *filetree.Node[T]) []*T {
+	files := []*T{}
+	_ = root.ForEachFile(func(file *T) error {
+		files = append(files, file)
+		return nil
+	})
+	return files
+}
+
+// filesInDir returns the files that the given directory contains, either at
+// their current or at their previous path.
+func filesInDir[T any, PT fileWithNames[T]](files []*T, dir string) []*T {
+	return lo.Filter(files, func(file *T, _ int) bool {
+		return fileIsInDir[T, PT](file, dir)
+	})
+}
+
+func fileIsInDir[T any, PT fileWithNames[T]](f *T, dir string) bool {
+	file := PT(f)
+	previousPath := file.GetPreviousPath()
+	return isInDir(file.GetPath(), dir) || (previousPath != "" && isInDir(previousPath, dir))
+}
+
+func isInDir(path string, dir string) bool {
+	// "." is the root item, which contains every file
+	return dir == "." || strings.HasPrefix(path, dir+"/")
+}

pkg/gui/controllers/diff_paths_test.go

@@ -0,0 +1,113 @@
+package controllers
+
+import (
+	"testing"
+
+	"github.com/jesseduffield/lazygit/pkg/commands/models"
+	"github.com/jesseduffield/lazygit/pkg/gui/filetree"
+	"github.com/samber/lo"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDiffPathsForNode(t *testing.T) {
+	files := []*models.CommitFile{
+		{Path: "dir/file1", PreviousPath: "file1", ChangeStatus: "R"},
+		{Path: "dir/file2-renamed", PreviousPath: "dir/file2", ChangeStatus: "R"},
+		{Path: "dir/sub/file3", ChangeStatus: "M"},
+		{Path: "file4", PreviousPath: "dir/sub/file4", ChangeStatus: "R"},
+		{Path: "file5", ChangeStatus: "M"},
+	}
+
+	scenarios := []struct {
+		testName      string
+		files         []*models.CommitFile // defaults to the files above
+		selectedPath  string
+		isFiltering   bool
+		expectedPaths []string
+	}{
+		{
+			testName:      "file",
+			selectedPath:  "dir/sub/file3",
+			expectedPaths: []string{"dir/sub/file3"},
+		},
+		{
+			testName:      "renamed file",
+			selectedPath:  "dir/file1",
+			expectedPaths: []string{"dir/file1", "file1"},
+		},
+		{
+			testName:     "directory: pass the other end of each rename that crosses its boundary",
+			selectedPath: "dir",
+			// dir/file2-renamed was renamed within the directory, so both of its
+			// paths are covered by it already
+			expectedPaths: []string{"dir", "file1", "file4"},
+		},
+		{
+			testName:      "directory without renames crossing its boundary",
+			selectedPath:  "dir/sub",
+			expectedPaths: []string{"dir/sub", "file4"},
+		},
+		{
+			testName:      "root",
+			selectedPath:  ".",
+			expectedPaths: []string{"."},
+		},
+		{
+			testName: "a whole directory moved into the selected one collapses to that directory",
+			files: []*models.CommitFile{
+				{Path: "dir/a", PreviousPath: "src/a", ChangeStatus: "R"},
+				{Path: "dir/b", PreviousPath: "src/nested/b", ChangeStatus: "R"},
+				{Path: "dir/c", PreviousPath: "src/nested/c", ChangeStatus: "R"},
+				{Path: "unrelated", ChangeStatus: "M"},
+			},
+			selectedPath:  "dir",
+			expectedPaths: []string{"dir", "src"},
+		},
+		{
+			testName: "a directory that stands in for the selected one as well",
+			files: []*models.CommitFile{
+				{Path: "a/b/c", PreviousPath: "a/c", ChangeStatus: "R"},
+				{Path: "a/b/d", ChangeStatus: "M"},
+				{Path: "unrelated", ChangeStatus: "M"},
+			},
+			selectedPath:  "a/b",
+			expectedPaths: []string{"a"},
+		},
+		{
+			testName: "a directory with changes of its own doesn't collapse",
+			files: []*models.CommitFile{
+				{Path: "dir/a", PreviousPath: "src/a", ChangeStatus: "R"},
+				{Path: "dir/b", PreviousPath: "src/nested/b", ChangeStatus: "R"},
+				{Path: "src/nested/c", ChangeStatus: "M"},
+			},
+			selectedPath: "dir",
+			// src/nested is left out of it, so that only src/a stays behind
+			expectedPaths: []string{"dir", "src/a", "src/nested/b"},
+		},
+		{
+			testName:     "directory while filtering",
+			selectedPath: "dir",
+			isFiltering:  true,
+			expectedPaths: []string{
+				"dir/file1", "file1",
+				"dir/file2-renamed", "dir/file2",
+				"dir/sub/file3",
+				"file4", "dir/sub/file4",
+			},
+		},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.testName, func(t *testing.T) {
+			files := lo.Ternary(s.files != nil, s.files, files)
+			cmp := filetree.NodeSortComparator[models.CommitFile]("mixed", true)
+			root := filetree.BuildTreeFromCommitFiles(files, true, cmp)
+			node, found := lo.Find(root.Flatten(filetree.NewCollapsedPaths()), func(node *filetree.Node[models.CommitFile]) bool {
+				return node.GetPath() == s.selectedPath
+			})
+			assert.True(t, found, "no node for path %s", s.selectedPath)
+
+			assert.Equal(t, s.expectedPaths, diffPathsForNode(node, root, files, s.isFiltering))
+		})
+	}
+}

pkg/gui/controllers/files_controller.go

@@ -368,8 +368,8 @@ func (self *FilesController) renderWorkingTreeDiff(node *filetree.FileNode) {
 	split := self.c.UserConfig().Gui.SplitDiff == "always" || (node.GetHasUnstagedChanges() && node.GetHasStagedChanges())
 	mainShowsStaged := !split && node.GetHasStagedChanges()
 
-	pathOverrides := self.pathOverridesForDiff(node)
-	cmdObj := self.c.Git().WorkingTree.WorktreeFileDiffCmdObj(node, false, mainShowsStaged, pathOverrides)
+	paths := self.pathsForDiff(node)
+	cmdObj := self.c.Git().WorkingTree.WorktreeFileDiffCmdObj(node, false, mainShowsStaged, paths)
 	title := self.c.Tr.UnstagedChanges
 	if mainShowsStaged {
 		title = self.c.Tr.StagedChanges
@@ -384,7 +384,7 @@ func (self *FilesController) renderWorkingTreeDiff(node *filetree.FileNode) {
 	}
 
 	if split {
-		cmdObj := self.c.Git().WorkingTree.WorktreeFileDiffCmdObj(node, false, true, pathOverrides)
+		cmdObj := self.c.Git().WorkingTree.WorktreeFileDiffCmdObj(node, false, true, paths)
 
 		title := self.c.Tr.StagedChanges
 		if mainShowsStaged {
@@ -642,19 +642,9 @@ func (self *FilesController) press(nodes []*filetree.FileNode) error {
 	return nil
 }
 
-// pathOverridesForDiff returns file paths to override the node's path in diff
-// commands when a text filter is active and the node is a directory. This
-// ensures the diff only shows filtered/visible files.
-func (self *FilesController) pathOverridesForDiff(node *filetree.FileNode) []string {
-	if !node.IsFile() && self.context().IsFiltering() {
-		var paths []string
-		_ = node.ForEachFile(func(file *models.File) error {
-			paths = append(paths, file.Path)
-			return nil
-		})
-		return paths
-	}
-	return nil
+func (self *FilesController) pathsForDiff(node *filetree.FileNode) []string {
+	return diffPathsForNode(
+		node.Raw(), self.context().GetRoot().Raw(), self.c.Model().Files, self.context().IsFiltering())
 }
 
 // unstageFilteredFiles unstages only the visible (filtered) files from the

pkg/gui/controllers/submodules_controller.go

@@ -123,7 +123,7 @@ func (self *SubmodulesController) GetOnRenderToMain() func() {
 				if file == nil {
 					task = types.NewRenderStringTask(prefix)
 				} else {
-					cmdObj := self.c.Git().WorkingTree.WorktreeFileDiffCmdObj(file, false, !file.HasUnstagedChanges && file.HasStagedChanges, nil)
+					cmdObj := self.c.Git().WorkingTree.WorktreeFileDiffCmdObj(file, false, !file.HasUnstagedChanges && file.HasStagedChanges, file.Names())
 					task = types.NewRunCommandTaskWithPrefix(cmdObj.GetCmd(), prefix)
 				}
 			}