diff --git a/pkg/gui/controllers/commits_files_controller.go b/pkg/gui/controllers/commits_files_controller.go index 690114dbb..4aa46a28c 100644 --- a/pkg/gui/controllers/commits_files_controller.go +++ b/pkg/gui/controllers/commits_files_controller.go @@ -175,7 +175,7 @@ func (self *CommitFilesController) GetOnRenderToMain() func() { from, to := self.context().GetFromAndToForDiff() from, reverse := self.c.Modes().Diffing.GetFromAndReverseArgsForDiff(from) - paths := pathsForDiff(node.Raw(), self.context().GetRoot().Raw(), self.context().IsFiltering()) + paths := self.pathsForDiff(node) cmdObj := self.c.Git().WorkingTree.ShowFileDiffCmdObj(from, to, reverse, paths, false) task := types.NewRunPtyTask(cmdObj.GetCmd()) @@ -263,8 +263,7 @@ func (self *CommitFilesController) openCopyMenu() error { copyFileDiffItem := &types.MenuItem{ Label: self.c.Tr.CopySelectedDiff, OnPress: func() error { - paths := pathsForDiff(node.Raw(), self.context().GetRoot().Raw(), self.context().IsFiltering()) - return self.copyDiffToClipboard(paths, self.c.Tr.FileDiffCopiedToast) + return self.copyDiffToClipboard(self.pathsForDiff(node), self.c.Tr.FileDiffCopiedToast) }, DisabledReason: self.require(self.singleItemSelected())(), Keys: menuKey('s'), @@ -617,6 +616,11 @@ func (self *CommitFilesController) GetOnClickFocusedMainView() func(mainViewName } } +func (self *CommitFilesController) pathsForDiff(node *filetree.CommitFileNode) []string { + 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 // could also be cleaned up with some generics func normalisedSelectedCommitFileNodes(selectedNodes []*filetree.CommitFileNode) []*filetree.CommitFileNode { diff --git a/pkg/gui/controllers/diff_paths.go b/pkg/gui/controllers/diff_paths.go index 4cb34f3cc..e9c12606f 100644 --- a/pkg/gui/controllers/diff_paths.go +++ b/pkg/gui/controllers/diff_paths.go @@ -1,9 +1,11 @@ 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 @@ -15,11 +17,11 @@ type fileWithNames[T any] interface { Names() []string } -// pathsForDiff returns the paths to limit a diff command to for showing the -// changes of the given node. root is the root of the tree that the node belongs -// to, and isFiltering says whether that tree is reduced to the files matching a -// text filter. -func pathsForDiff[T any, PT fileWithNames[T]](node *filetree.Node[T], root *filetree.Node[T], isFiltering bool) []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() } @@ -30,9 +32,9 @@ func pathsForDiff[T any, PT fileWithNames[T]](node *filetree.Node[T], root *file // Passing the directory would bring back the files that the filter hides, // so we spell out the ones it leaves. var paths []string - forEachFileInDir[T, PT](root, dir, func(file PT) { - paths = append(paths, file.Names()...) - }) + for _, file := range filesInDir[T, PT](filesInTree(root), dir) { + paths = append(paths, PT(file).Names()...) + } return paths } @@ -40,29 +42,88 @@ func pathsForDiff[T any, PT fileWithNames[T]](node *filetree.Node[T], root *file // 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. - paths := []string{dir} - forEachFileInDir[T, PT](root, dir, func(file PT) { - if path := file.GetPath(); !isInDir(path, dir) { - paths = append(paths, path) + 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 previousPath := file.GetPreviousPath(); previousPath != "" && !isInDir(previousPath, dir) { - paths = append(paths, previousPath) + if p := file.GetPreviousPath(); p != "" && !isInDir(p, dir) { + outsidePaths = append(outsidePaths, p) } - }) - return paths + } + + return dropContainedPaths(append([]string{dir}, collapseToDirs[T, PT](outsidePaths, files, dir)...)) } -// forEachFileInDir calls cb for each file in the tree that the given directory -// contains, either at its current or at its previous path. -func forEachFileInDir[T any, PT fileWithNames[T]](root *filetree.Node[T], dir string, cb func(PT)) { - _ = root.ForEachFile(func(f *T) error { - file := PT(f) - previousPath := file.GetPreviousPath() - if isInDir(file.GetPath(), dir) || (previousPath != "" && isInDir(previousPath, dir)) { - cb(file) +// 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 { diff --git a/pkg/gui/controllers/diff_paths_test.go b/pkg/gui/controllers/diff_paths_test.go index ef095a29a..549ed4f31 100644 --- a/pkg/gui/controllers/diff_paths_test.go +++ b/pkg/gui/controllers/diff_paths_test.go @@ -9,7 +9,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestPathsForDiff(t *testing.T) { +func TestDiffPathsForNode(t *testing.T) { files := []*models.CommitFile{ {Path: "dir/file1", PreviousPath: "file1", ChangeStatus: "R"}, {Path: "dir/file2-renamed", PreviousPath: "dir/file2", ChangeStatus: "R"}, @@ -20,6 +20,7 @@ func TestPathsForDiff(t *testing.T) { scenarios := []struct { testName string + files []*models.CommitFile // defaults to the files above selectedPath string isFiltering bool expectedPaths []string @@ -51,6 +52,38 @@ func TestPathsForDiff(t *testing.T) { 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", @@ -66,6 +99,7 @@ func TestPathsForDiff(t *testing.T) { 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 { @@ -73,7 +107,7 @@ func TestPathsForDiff(t *testing.T) { }) assert.True(t, found, "no node for path %s", s.selectedPath) - assert.Equal(t, s.expectedPaths, pathsForDiff(node, root, s.isFiltering)) + 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 0eceeef32..e61e1409c 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -369,7 +369,7 @@ func (self *FilesController) renderWorkingTreeDiff(node *filetree.FileNode) { split := self.c.UserConfig().Gui.SplitDiff == "always" || (node.GetHasUnstagedChanges() && node.GetHasStagedChanges()) mainShowsStaged := !split && node.GetHasStagedChanges() - paths := pathsForDiff(node.Raw(), self.context().GetRoot().Raw(), self.context().IsFiltering()) + paths := self.pathsForDiff(node) cmdObj := self.c.Git().WorkingTree.WorktreeFileDiffCmdObj(node, false, mainShowsStaged, paths) title := self.c.Tr.UnstagedChanges if mainShowsStaged { @@ -643,6 +643,11 @@ func (self *FilesController) press(nodes []*filetree.FileNode) error { 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 // given nodes, correctly partitioning by tracked/untracked. func (self *FilesController) unstageFilteredFiles(nodes []*filetree.FileNode) error {