diff --git a/pkg/commands/git_commands/working_tree.go b/pkg/commands/git_commands/working_tree.go index 1f9d9653bd0..846b359b38c 100644 --- a/pkg/commands/git_commands/working_tree.go +++ b/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() diff --git a/pkg/gui/controllers/commits_files_controller.go b/pkg/gui/controllers/commits_files_controller.go index 748355f6729..4aa46a28c09 100644 --- a/pkg/gui/controllers/commits_files_controller.go +++ b/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 diff --git a/pkg/gui/controllers/diff_paths.go b/pkg/gui/controllers/diff_paths.go new file mode 100644 index 00000000000..e9c12606ff2 --- /dev/null +++ b/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+"/") +} diff --git a/pkg/gui/controllers/diff_paths_test.go b/pkg/gui/controllers/diff_paths_test.go new file mode 100644 index 00000000000..549ed4f3186 --- /dev/null +++ b/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)) + }) + } +} diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index 41f9197fe75..1ecbd37371d 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/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 diff --git a/pkg/gui/controllers/submodules_controller.go b/pkg/gui/controllers/submodules_controller.go index a2dd22ed3ff..82ca509cafb 100644 --- a/pkg/gui/controllers/submodules_controller.go +++ b/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) } } diff --git a/pkg/integration/tests/commit/directory_diff_with_renamed_files.go b/pkg/integration/tests/commit/directory_diff_with_renamed_files.go new file mode 100644 index 00000000000..f849056f78c --- /dev/null +++ b/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"), + ) + }, +}) diff --git a/pkg/integration/tests/file/directory_diff_with_renamed_files.go b/pkg/integration/tests/file/directory_diff_with_renamed_files.go new file mode 100644 index 00000000000..18906bf034e --- /dev/null +++ b/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"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 15ef6f8c75d..645e9710f9b 100644 --- a/pkg/integration/tests/test_list.go +++ b/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,