Show renames when selecting a directory that a file was moved into or out of (#5924)

In a commit that moves a bunch of files from one directory to another,
showing the commit's files and selecting the target directory of those
moves would show these files as newly added rather than moved in the
main view's diff. Selecting the source directory would show them as
removed. Fix this to keep showing them as moved in both cases. The same
applies to the files panel when staging the move of a file, and when
filtering the file list down to just the source or target directory
using the `/` filter in either panel.

The decision to show them as renames when selecting the "moved-from"
directory was not an easy one; it's slightly weird because the list of
files in the side panel doesn't show them there (they appear in the
target directory), but the main view does. An alternative would have
been not to show them in that case, to match the side panel. However,
the point of selecting a directory is to see all the changes that affect
it, and the moved-out files are relevant changes you want to see there.

See
https://github.com/jesseduffield/lazygit/discussions/4899#discussioncomment-17976172.

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

@@ -369,8 +369,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
@@ -385,7 +385,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 {
@@ -643,19 +643,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)
 				}
 			}

pkg/integration/tests/commit/directory_diff_with_renamed_files.go

@@ -0,0 +1,90 @@
+package commit
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var DirectoryDiffWithRenamedFiles = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Selecting a directory in the commit files panel shows the renames of files that were moved into or out of it",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateDir("dir")
+		shell.CreateDir("dir/nested")
+		shell.CreateFileAndAdd("file1", "file1 content\n")
+		shell.CreateFileAndAdd("dir/file2", "file2 content\n")
+		shell.CreateFileAndAdd("dir/nested/file3", "file3 content\n")
+		shell.Commit("initial commit")
+		shell.RenameFileInGit("file1", "dir/file1")
+		shell.RenameFileInGit("dir/file2", "dir/file2-renamed")
+		shell.RenameFileInGit("dir/nested/file3", "file3")
+		shell.Commit("move files")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Commits().
+			Focus().
+			Lines(
+				Contains("move files").IsSelected(),
+				Contains("initial commit"),
+			).
+			PressEnter()
+
+		t.Views().CommitFiles().
+			IsFocused().
+			Lines(
+				Equals("▼ /").IsSelected(),
+				Equals("  ▼ dir"),
+				Equals("    R file1 → file1"),
+				Equals("    R file2 → file2-renamed"),
+				Equals("  R dir/nested/file3 → file3"),
+			)
+
+		t.Views().Main().ContainsLines(
+			Equals("diff --git a/file1 b/dir/file1"),
+			Equals("similarity index 100%"),
+			Equals("rename from file1"),
+			Equals("rename to dir/file1"),
+			Equals("diff --git a/dir/file2 b/dir/file2-renamed"),
+			Equals("similarity index 100%"),
+			Equals("rename from dir/file2"),
+			Equals("rename to dir/file2-renamed"),
+			Equals("diff --git a/dir/nested/file3 b/file3"),
+			Equals("similarity index 100%"),
+			Equals("rename from dir/nested/file3"),
+			Equals("rename to file3"),
+		)
+
+		t.Views().CommitFiles().
+			SelectNextItem().
+			SelectedLine(Equals("  ▼ dir"))
+
+		t.Views().Main().
+			ContainsLines(
+				Equals("diff --git a/file1 b/dir/file1"),
+				Equals("similarity index 100%"),
+				Equals("rename from file1"),
+				Equals("rename to dir/file1"),
+				Equals("diff --git a/dir/file2 b/dir/file2-renamed"),
+				Equals("similarity index 100%"),
+				Equals("rename from dir/file2"),
+				Equals("rename to dir/file2-renamed"),
+				Equals("diff --git a/dir/nested/file3 b/file3"),
+				Equals("similarity index 100%"),
+				Equals("rename from dir/nested/file3"),
+				Equals("rename to file3"),
+			)
+
+		t.Views().CommitFiles().
+			SelectNextItem().
+			SelectedLine(Equals("    R file1 → file1"))
+
+		t.Views().Main().ContainsLines(
+			Equals("diff --git a/file1 b/dir/file1"),
+			Equals("similarity index 100%"),
+			Equals("rename from file1"),
+			Equals("rename to dir/file1"),
+		)
+	},
+})

pkg/integration/tests/file/directory_diff_with_renamed_files.go

@@ -0,0 +1,86 @@
+package file
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var DirectoryDiffWithRenamedFiles = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Selecting a directory in the files panel shows the renames of files that were moved into or out of it",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateDir("dir")
+		shell.CreateDir("dir/nested")
+		shell.CreateFileAndAdd("file1", "file1 content\n")
+		shell.CreateFileAndAdd("dir/file2", "file2 content\n")
+		shell.CreateFileAndAdd("dir/nested/file3", "file3 content\n")
+		shell.Commit("initial commit")
+		shell.RenameFileInGit("file1", "dir/file1")
+		shell.RenameFileInGit("dir/file2", "dir/file2-renamed")
+		shell.RenameFileInGit("dir/nested/file3", "file3")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Files().
+			IsFocused().
+			Lines(
+				Equals("▼ /").IsSelected(),
+				Equals("  ▼ dir"),
+				Equals("    R  file1 → file1"),
+				Equals("    R  file2 → file2-renamed"),
+				Equals("  R  dir/nested/file3 → file3"),
+			)
+
+		t.Views().Main().ContainsLines(
+			Equals("diff --git a/file1 b/dir/file1"),
+			Equals("similarity index 100%"),
+			Equals("rename from file1"),
+			Equals("rename to dir/file1"),
+			Equals("diff --git a/dir/file2 b/dir/file2-renamed"),
+			Equals("similarity index 100%"),
+			Equals("rename from dir/file2"),
+			Equals("rename to dir/file2-renamed"),
+			Equals("diff --git a/dir/nested/file3 b/file3"),
+			Equals("similarity index 100%"),
+			Equals("rename from dir/nested/file3"),
+			Equals("rename to file3"),
+		)
+
+		t.Views().Files().
+			SelectNextItem().
+			SelectedLine(Equals("  ▼ dir"))
+
+		t.Views().Main().
+			ContainsLines(
+				Equals("diff --git a/file1 b/dir/file1"),
+				Equals("similarity index 100%"),
+				Equals("rename from file1"),
+				Equals("rename to dir/file1"),
+				Equals("diff --git a/dir/file2 b/dir/file2-renamed"),
+				Equals("similarity index 100%"),
+				Equals("rename from dir/file2"),
+				Equals("rename to dir/file2-renamed"),
+				Equals("diff --git a/dir/nested/file3 b/file3"),
+				Equals("similarity index 100%"),
+				Equals("rename from dir/nested/file3"),
+				Equals("rename to file3"),
+			)
+
+		// The same applies when a filter reduces the directory to a single file
+		t.Views().Files().
+			FilterOrSearch("file1").
+			Lines(
+				Equals("▼ dir").IsSelected(),
+				Equals("  R  file1 → file1"),
+			)
+
+		t.Views().Main().
+			ContainsLines(
+				Equals("diff --git a/file1 b/dir/file1"),
+				Equals("similarity index 100%"),
+				Equals("rename from file1"),
+				Equals("rename to dir/file1"),
+			)
+	},
+})

pkg/integration/tests/test_list.go

@@ -126,6 +126,7 @@ var tests = []*components.IntegrationTest{
 	commit.CreateAmendCommit,
 	commit.CreateFixupCommitInBranchStack,
 	commit.CreateTag,
+	commit.DirectoryDiffWithRenamedFiles,
 	commit.DisableCopyCommitMessageBody,
 	commit.DiscardOldFileChanges,
 	commit.DiscardRenamedFile,
@@ -231,6 +232,7 @@ var tests = []*components.IntegrationTest{
 	file.CollapseExpand,
 	file.CopyMenu,
 	file.DirWithUntrackedFile,
+	file.DirectoryDiffWithRenamedFiles,
 	file.DiscardAllDirChanges,
 	file.DiscardAllDirChangesWhenFiltering,
 	file.DiscardRangeSelect,