Fix unstaging a submodule with dirty content

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 <path>` 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 "<prefix><sha> <path> (<describe>)". 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

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,20 +448,80 @@ 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() {
 			return errors.New(self.c.Tr.ErrStageDirWithInlineMergeConflicts)
 		}
 	}
 
+	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
-		}
+		return self.c.Git().WorkingTree.StageFiles(toPaths(unstagedNodes), extraArgs)
+	}
 
-		if err := self.c.Git().WorkingTree.StageFiles(toPaths(unstagedSelectedNodes), extraArgs); err != nil {
-			return err
+	unstage := func(nodes []*filetree.FileNode) error {
+		if self.context().IsFiltering() {
+			// When filtering, only unstage visible files
+			return self.unstageFilteredFiles(nodes)
 		}
-	} else {
-		self.c.LogAction(self.c.Tr.Actions.UnstageFile)
 
-		if err := self.optimisticChange(selectedNodes, self.optimisticUnstage); err != nil {
-			return err
-		}
+		// 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 self.context().IsFiltering() {
-			// When filtering, only unstage visible files
-			if err := self.unstageFilteredFiles(selectedNodes); err != nil {
+		if len(untrackedNodes) > 0 {
+			if err := self.c.Git().WorkingTree.UnstageUntrackedFiles(toPaths(untrackedNodes)); 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
-				}
+		if len(trackedNodes) > 0 {
+			if err := self.c.Git().WorkingTree.UnstageTrackedFiles(toPaths(trackedNodes)); err != nil {
+				return err
 			}
 		}
+
+		return nil
 	}
 
-	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,55 +767,33 @@ 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
 			_ = root.ForEachFile(func(file *models.File) 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
-			}
+			return self.c.Git().WorkingTree.StageFiles(paths, nil)
 		}
-	} else {
-		self.c.LogAction(self.c.Tr.Actions.UnstageAllFiles)
 
-		if err := self.optimisticChange([]*filetree.FileNode{root}, self.optimisticUnstage); err != nil {
-			return err
-		}
+		onlyTrackedFiles := self.context().GetStatusFilter() == filetree.DisplayTracked
+		return self.c.Git().WorkingTree.StageAll(onlyTrackedFiles)
+	}
 
+	unstage := func(nodes []*filetree.FileNode) error {
 		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
-			}
+			return self.unstageFilteredFiles(nodes)
 		}
+
+		return self.c.Git().WorkingTree.UnstageAll()
 	}
 
-	return nil
+	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 {

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)",