diff --git a/pkg/commands/git_commands/submodule.go b/pkg/commands/git_commands/submodule.go index acb335e35..7a3cb687b 100644 --- a/pkg/commands/git_commands/submodule.go +++ b/pkg/commands/git_commands/submodule.go @@ -9,6 +9,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/samber/lo" ) // .gitmodules looks like this: @@ -86,6 +87,30 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig) return configs, nil } +// AnyHaveStageableChanges reports whether any of the given submodule paths has +// a checked-out commit that differs from the one recorded in the +// superproject's index, i.e. a change that `git add ` would actually +// stage. A submodule that only has dirty or untracked content (with no new +// commit) can't be staged from the superproject, so it won't be reported here. +func (self *SubmoduleCommands) AnyHaveStageableChanges(paths []string) (bool, error) { + if len(paths) == 0 { + return false, nil + } + + cmdArgs := NewGitCmd("submodule").Arg("status", "--").Arg(paths...).ToArgv() + output, err := self.cmd.New(cmdArgs).DontLog().RunWithOutput() + if err != nil { + return false, err + } + + // Each line looks like " ()". A '+' prefix + // means the checked-out commit differs from the index, i.e. there's a + // commit change to stage. + return lo.SomeBy(strings.Split(output, "\n"), func(line string) bool { + return strings.HasPrefix(line, "+") + }), nil +} + func (self *SubmoduleCommands) Stash(submodule *models.SubmoduleConfig) error { // if the path does not exist then it hasn't yet been initialized so we'll swallow the error // because the intention here is to have no dirty worktree state diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index 8ea4425e6..09f654e2b 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -387,6 +387,9 @@ var unstageStatusMap = map[string]string{ "A ": "??", "M ": " M", "D ": " D", + // A submodule with both a staged commit and unstageable dirty content; the + // staged commit gets unstaged, the dirty content stays. + "MM": " M", } func (self *FilesController) optimisticStage(file *models.File) bool { @@ -445,13 +448,23 @@ func (self *FilesController) optimisticChange(nodes []*filetree.FileNode, optimi return nil } -func (self *FilesController) pressWithLock(selectedNodes []*filetree.FileNode) error { - // Obtaining this lock because optimistic rendering requires us to mutate - // the files in our model. - self.c.Mutexes().RefreshingFilesMutex.Lock() - defer self.c.Mutexes().RefreshingFilesMutex.Unlock() - - for _, node := range selectedNodes { +// toggleStaged decides whether to stage or unstage the given nodes, updates the +// model optimistically, and then runs the matching git command via the supplied +// callbacks. press() (acting on the selection) and toggleStagedAll() (acting on +// the whole tree) share this; they differ only in the git commands they run, +// which is why those are passed in. +// +// If any node has unstaged changes we stage the nodes that have them (staging +// already-staged deleted files/folders would fail); otherwise we unstage all +// the nodes. +func (self *FilesController) toggleStaged( + nodes []*filetree.FileNode, + stageAction string, + unstageAction string, + stage func(unstagedNodes []*filetree.FileNode) error, + unstage func(nodes []*filetree.FileNode) error, +) error { + for _, node := range nodes { // if any files within have inline merge conflicts we can't stage or unstage, // or it'll end up with those >>>>>> lines actually staged if node.GetHasInlineMergeConflicts() { @@ -459,6 +472,56 @@ func (self *FilesController) pressWithLock(selectedNodes []*filetree.FileNode) e } } + nodes = normalisedSelectedNodes(nodes) + + unstagedNodes := filterNodesHaveUnstagedChanges(nodes, self.c.Model().Submodules) + + // Staging a submodule that only has dirty or untracked content (no new + // commit) is a no-op: the parent repo can't stage that content. When that's + // the only thing that looks stageable, don't stage; fall through to + // unstaging instead. That keeps the toggle symmetric (e.g. a fully-staged + // tree that also contains a dirty submodule still unstages on the next + // press) rather than getting stuck trying to stage the unstageable content. + shouldStage := len(unstagedNodes) > 0 + if shouldStage { + noOp, err := self.stagingWouldBeNoOp(unstagedNodes) + if err != nil { + return err + } + shouldStage = !noOp + } + + if shouldStage { + self.c.LogAction(stageAction) + + if err := self.optimisticChange(unstagedNodes, self.optimisticStage); err != nil { + return err + } + + return stage(unstagedNodes) + } + + // If there's nothing staged to unstage either, then the only thing we acted + // on was an unstageable submodule and nothing happened, so say why. + if !someNodesHaveStagedChanges(nodes) { + return errors.New(self.c.Tr.NothingToStageForSubmodule) + } + + self.c.LogAction(unstageAction) + + if err := self.optimisticChange(nodes, self.optimisticUnstage); err != nil { + return err + } + + return unstage(nodes) +} + +func (self *FilesController) pressWithLock(selectedNodes []*filetree.FileNode) error { + // Obtaining this lock because optimistic rendering requires us to mutate + // the files in our model. + self.c.Mutexes().RefreshingFilesMutex.Lock() + defer self.c.Mutexes().RefreshingFilesMutex.Unlock() + // When filtering, expand directory nodes to individual visible file paths // so that only filtered files are staged/unstaged. toPaths := func(nodes []*filetree.FileNode) []string { @@ -477,63 +540,46 @@ func (self *FilesController) pressWithLock(selectedNodes []*filetree.FileNode) e }) } - selectedNodes = normalisedSelectedNodes(selectedNodes) - - // If any node has unstaged changes, we'll stage all the selected unstaged nodes (staging already staged deleted files/folders would fail). - // Otherwise, we unstage all the selected nodes. - unstagedSelectedNodes := filterNodesHaveUnstagedChanges(selectedNodes) - - if len(unstagedSelectedNodes) > 0 { + stage := func(unstagedNodes []*filetree.FileNode) error { var extraArgs []string - if self.context().GetStatusFilter() == filetree.DisplayTracked { extraArgs = []string{"-u"} } - self.c.LogAction(self.c.Tr.Actions.StageFile) - - if err := self.optimisticChange(unstagedSelectedNodes, self.optimisticStage); err != nil { - return err - } - - if err := self.c.Git().WorkingTree.StageFiles(toPaths(unstagedSelectedNodes), extraArgs); err != nil { - return err - } - } else { - self.c.LogAction(self.c.Tr.Actions.UnstageFile) - - if err := self.optimisticChange(selectedNodes, self.optimisticUnstage); err != nil { - return err - } - - if self.context().IsFiltering() { - // When filtering, only unstage visible files - if err := self.unstageFilteredFiles(selectedNodes); err != nil { - return err - } - } else { - // need to partition the paths into tracked and untracked (where we assume directories are tracked). Then we'll run the commands separately. - trackedNodes, untrackedNodes := utils.Partition(selectedNodes, func(node *filetree.FileNode) bool { - // We treat all directories as tracked. I'm not actually sure why we do this but - // it's been the existing behaviour for a while and nobody has complained - return !node.IsFile() || node.GetIsTracked() - }) - - if len(untrackedNodes) > 0 { - if err := self.c.Git().WorkingTree.UnstageUntrackedFiles(toPaths(untrackedNodes)); err != nil { - return err - } - } - - if len(trackedNodes) > 0 { - if err := self.c.Git().WorkingTree.UnstageTrackedFiles(toPaths(trackedNodes)); err != nil { - return err - } - } - } + return self.c.Git().WorkingTree.StageFiles(toPaths(unstagedNodes), extraArgs) } - return nil + unstage := func(nodes []*filetree.FileNode) error { + if self.context().IsFiltering() { + // When filtering, only unstage visible files + return self.unstageFilteredFiles(nodes) + } + + // need to partition the paths into tracked and untracked (where we assume directories are tracked). Then we'll run the commands separately. + trackedNodes, untrackedNodes := utils.Partition(nodes, func(node *filetree.FileNode) bool { + // We treat all directories as tracked. I'm not actually sure why we do this but + // it's been the existing behaviour for a while and nobody has complained + return !node.IsFile() || node.GetIsTracked() + }) + + if len(untrackedNodes) > 0 { + if err := self.c.Git().WorkingTree.UnstageUntrackedFiles(toPaths(untrackedNodes)); err != nil { + return err + } + } + + if len(trackedNodes) > 0 { + if err := self.c.Git().WorkingTree.UnstageTrackedFiles(toPaths(trackedNodes)); err != nil { + return err + } + } + + return nil + } + + return self.toggleStaged(selectedNodes, + self.c.Tr.Actions.StageFile, self.c.Tr.Actions.UnstageFile, + stage, unstage) } func (self *FilesController) press(nodes []*filetree.FileNode) error { @@ -721,19 +767,7 @@ func (self *FilesController) toggleStagedAllWithLock() error { root := self.context().FileTreeViewModel.GetRoot() - // if any files within have inline merge conflicts we can't stage or unstage, - // or it'll end up with those >>>>>> lines actually staged - if root.GetHasInlineMergeConflicts() { - return errors.New(self.c.Tr.ErrStageDirWithInlineMergeConflicts) - } - - if root.GetHasUnstagedChanges() { - self.c.LogAction(self.c.Tr.Actions.StageAllFiles) - - if err := self.optimisticChange([]*filetree.FileNode{root}, self.optimisticStage); err != nil { - return err - } - + stage := func(unstagedNodes []*filetree.FileNode) error { if self.context().IsFiltering() { // When filtering, only stage visible files var paths []string @@ -741,35 +775,25 @@ func (self *FilesController) toggleStagedAllWithLock() error { paths = append(paths, file.Path) return nil }) - if err := self.c.Git().WorkingTree.StageFiles(paths, nil); err != nil { - return err - } - } else { - onlyTrackedFiles := self.context().GetStatusFilter() == filetree.DisplayTracked - if err := self.c.Git().WorkingTree.StageAll(onlyTrackedFiles); err != nil { - return err - } - } - } else { - self.c.LogAction(self.c.Tr.Actions.UnstageAllFiles) - - if err := self.optimisticChange([]*filetree.FileNode{root}, self.optimisticUnstage); err != nil { - return err + return self.c.Git().WorkingTree.StageFiles(paths, nil) } - if self.context().IsFiltering() { - // When filtering, only unstage visible files - if err := self.unstageFilteredFiles([]*filetree.FileNode{root}); err != nil { - return err - } - } else { - if err := self.c.Git().WorkingTree.UnstageAll(); err != nil { - return err - } - } + onlyTrackedFiles := self.context().GetStatusFilter() == filetree.DisplayTracked + return self.c.Git().WorkingTree.StageAll(onlyTrackedFiles) } - return nil + unstage := func(nodes []*filetree.FileNode) error { + if self.context().IsFiltering() { + // When filtering, only unstage visible files + return self.unstageFilteredFiles(nodes) + } + + return self.c.Git().WorkingTree.UnstageAll() + } + + return self.toggleStaged([]*filetree.FileNode{root}, + self.c.Tr.Actions.StageAllFiles, self.c.Tr.Actions.UnstageAllFiles, + stage, unstage) } func (self *FilesController) unstageFiles(node *filetree.FileNode) error { @@ -1421,12 +1445,66 @@ func someNodesHaveStagedChanges(nodes []*filetree.FileNode) bool { return lo.SomeBy(nodes, (*filetree.FileNode).GetHasStagedChanges) } -func filterNodesHaveUnstagedChanges(nodes []*filetree.FileNode) []*filetree.FileNode { +func filterNodesHaveUnstagedChanges(nodes []*filetree.FileNode, submodules []*models.SubmoduleConfig) []*filetree.FileNode { return lo.Filter(nodes, func(node *filetree.FileNode, _ int) bool { - return node.GetHasUnstagedChanges() + return node.SomeFile(func(file *models.File) bool { + return fileHasStageableUnstagedChanges(file, submodules) + }) }) } +// For a submodule, the only thing the parent repo can stage is the +// commit-pointer change; dirty or untracked content within the submodule +// shows up as an unstaged change but can never be staged from the parent. So +// once the submodule's commit is staged (leaving it at e.g. "MM"), we mustn't +// treat the leftover unstaged change as stageable, or pressing space would +// keep trying to stage it instead of unstaging it. +func fileHasStageableUnstagedChanges(file *models.File, submodules []*models.SubmoduleConfig) bool { + if !file.HasUnstagedChanges { + return false + } + + if file.IsSubmodule(submodules) { + return !file.HasStagedChanges + } + + return true +} + +// stagingWouldBeNoOp reports whether staging the given nodes would have no +// visible effect, which happens when the only things being staged are +// submodules that have dirty or untracked content but no new commit: the +// parent repo can't stage that content. If a regular file (or a submodule with +// a stageable new commit) is among them, staging does something, so this +// returns false. +func (self *FilesController) stagingWouldBeNoOp(nodes []*filetree.FileNode) (bool, error) { + submodules := self.c.Model().Submodules + + var submodulePaths []string + hasOtherStageableChanges := false + for _, node := range nodes { + _ = node.ForEachFile(func(file *models.File) error { + if file.IsSubmodule(submodules) { + submodulePaths = append(submodulePaths, file.Path) + } else if file.HasUnstagedChanges { + hasOtherStageableChanges = true + } + return nil + }) + } + + if hasOtherStageableChanges || len(submodulePaths) == 0 { + return false, nil + } + + anyStageable, err := self.c.Git().Submodule.AnyHaveStageableChanges(submodulePaths) + if err != nil { + return false, err + } + + return !anyStageable, nil +} + func findSubmoduleNode(nodes []*filetree.FileNode, submodules []*models.SubmoduleConfig) *models.File { for _, node := range nodes { submoduleNode := node.FindFirstFileBy(func(f *models.File) bool { diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 5dcc80806..d112c0379 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -917,6 +917,7 @@ type TranslationSet struct { SelectedItemIsNotABranch string SelectedItemDoesNotHaveFiles string MultiSelectNotSupportedForSubmodules string + NothingToStageForSubmodule string CommandDoesNotSupportOpeningInEditor string CustomCommands string NoApplicableCommandsInThisContext string @@ -2038,6 +2039,7 @@ func EnglishTranslationSet() *TranslationSet { SelectedItemIsNotABranch: "Selected item is not a branch", SelectedItemDoesNotHaveFiles: "Selected item does not have files to view", MultiSelectNotSupportedForSubmodules: "Multiselection not supported for submodules", + NothingToStageForSubmodule: "Nothing to stage: the parent repo can only stage a new submodule commit, not the uncommitted changes inside a submodule. Commit inside the submodule first.", CommandDoesNotSupportOpeningInEditor: "This command doesn't support switching to the editor", CustomCommands: "Custom commands", NoApplicableCommandsInThisContext: "(No applicable commands in this context)", diff --git a/pkg/integration/tests/submodule/stage.go b/pkg/integration/tests/submodule/stage.go new file mode 100644 index 000000000..b8ef5e35f --- /dev/null +++ b/pkg/integration/tests/submodule/stage.go @@ -0,0 +1,58 @@ +package submodule + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var Stage = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Stage and unstage a submodule that has both a new commit and dirty content. The new commit can be staged, but the dirty content can't, so unstaging must still work; this must hold for both the stage (space) and stage-all (a) keybindings.", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.GetUserConfig().Gui.ShowFileTree = false + }, + SetupRepo: func(shell *Shell) { + shell.EmptyCommit("first commit") + shell.CloneIntoSubmodule("my_submodule_name", "my_submodule_path") + shell.GitAddAll() + shell.Commit("add submodule") + + // Give the submodule a new commit, which is a change that the parent + // repo can stage, as well as some dirty working-tree content, which + // the parent repo can never stage. This is what gets us a "MM" status + // once the new commit is staged. + shell.RunCommand([]string{"git", "-C", "my_submodule_path", "commit", "--allow-empty", "-m", "submodule commit"}) + shell.CreateFile("my_submodule_path/dirty_file", "dirty content") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files().Focus(). + Lines( + Equals(" M my_submodule_path (submodule)").IsSelected(), + ). + // Staging the submodule stages the new commit, but the dirty + // content remains unstaged, leaving us at "MM". + PressPrimaryAction(). + Lines( + Equals("MM my_submodule_path (submodule)").IsSelected(), + ). + // Pressing again must unstage the submodule, taking us back to + // " M" rather than trying (and failing) to stage the dirty content. + PressPrimaryAction(). + Lines( + Equals(" M my_submodule_path (submodule)").IsSelected(), + ). + // The same has to hold for the stage-all keybinding, which shares + // the same decision logic: it stages the new commit... + Press(keys.Files.ToggleStagedAll). + Lines( + Equals("MM my_submodule_path (submodule)").IsSelected(), + ). + // ...and then unstages it again rather than getting stuck on the + // dirty content. + Press(keys.Files.ToggleStagedAll). + Lines( + Equals(" M my_submodule_path (submodule)").IsSelected(), + ) + }, +}) diff --git a/pkg/integration/tests/submodule/stage_all_with_dirty_submodule.go b/pkg/integration/tests/submodule/stage_all_with_dirty_submodule.go new file mode 100644 index 000000000..ca54a5970 --- /dev/null +++ b/pkg/integration/tests/submodule/stage_all_with_dirty_submodule.go @@ -0,0 +1,46 @@ +package submodule + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var StageAllWithDirtySubmodule = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "A submodule with only dirty content (which can't be staged) must not break the stage-all toggle: pressing it repeatedly should keep toggling the other files between staged and unstaged.", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.GetUserConfig().Gui.ShowFileTree = false + }, + SetupRepo: func(shell *Shell) { + shell.EmptyCommit("first commit") + shell.CloneIntoSubmodule("my_submodule_name", "my_submodule_path") + shell.GitAddAll() + shell.Commit("add submodule") + + // A submodule with dirty content but no new commit (can't be staged), + // alongside a regular file that can. + shell.CreateFile("my_submodule_path/dirty_file", "dirty content") + shell.CreateFile("regular_file", "content") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files().Focus(). + Lines( + Equals(" M my_submodule_path (submodule)"), + Equals("?? regular_file"), + ). + // Stage all: the regular file gets staged; the submodule can't be. + Press(keys.Files.ToggleStagedAll). + Lines( + Equals(" M my_submodule_path (submodule)"), + Equals("A regular_file"), + ). + // Stage all again: nothing is stageable, but the regular file is + // staged, so this unstages it rather than erroring on the submodule. + Press(keys.Files.ToggleStagedAll). + Lines( + Equals(" M my_submodule_path (submodule)"), + Equals("?? regular_file"), + ) + }, +}) diff --git a/pkg/integration/tests/submodule/stage_dirty_only.go b/pkg/integration/tests/submodule/stage_dirty_only.go new file mode 100644 index 000000000..3ae20e677 --- /dev/null +++ b/pkg/integration/tests/submodule/stage_dirty_only.go @@ -0,0 +1,53 @@ +package submodule + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var StageDirtyOnly = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Pressing space on a submodule that only has dirty content (no new commit) can't stage anything, so we explain that with an error instead of silently doing nothing.", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.GetUserConfig().Gui.ShowFileTree = false + }, + SetupRepo: func(shell *Shell) { + shell.EmptyCommit("first commit") + shell.CloneIntoSubmodule("my_submodule_name", "my_submodule_path") + shell.GitAddAll() + shell.Commit("add submodule") + + // Dirty working-tree content, but no new commit: there's nothing the + // parent repo can stage. + shell.CreateFile("my_submodule_path/dirty_file", "dirty content") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files().Focus(). + Lines( + Equals(" M my_submodule_path (submodule)").IsSelected(), + ). + PressPrimaryAction(). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Error")). + Content(Contains("Nothing to stage")). + Confirm() + }). + // The status is unchanged: nothing got staged. + Lines( + Equals(" M my_submodule_path (submodule)").IsSelected(), + ). + // Pressing "stage all" must behave the same way. + Press(keys.Files.ToggleStagedAll). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Error")). + Content(Contains("Nothing to stage")). + Confirm() + }). + Lines( + Equals(" M my_submodule_path (submodule)").IsSelected(), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 6f0032391..0f3a40634 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -426,6 +426,9 @@ var tests = []*components.IntegrationTest{ submodule.RemoveNested, submodule.Reset, submodule.ResetFolder, + submodule.Stage, + submodule.StageAllWithDirtySubmodule, + submodule.StageDirtyOnly, sync.FetchAndAutoForwardBranchesAllBranches, sync.FetchAndAutoForwardBranchesAllBranchesCheckedOutInOtherWorktree, sync.FetchAndAutoForwardBranchesNone,