Fix unstaging a submodule with dirty content (#5666)

Staging and unstaging submodules didn't work properly when the submodule
had uncommitted changes of its own (modified or untracked files inside
it). This PR fixes a few related problems:

- **Unstaging a submodule that has both a new commit and uncommitted
changes now works.** Previously, staging such a submodule left it
half-staged, and from there the stage key would only ever try to
re-stage it — there was no way to unstage it again. Now the stage key
toggles it back to unstaged as expected.

- **Trying to stage a submodule that has nothing stageable now explains
why.** When a submodule's only changes are uncommitted content inside it
(with no new commit), there's nothing the parent repository can stage.
Instead of the keypress silently doing nothing (except briefly flashing
the status to staged and then back to unstaged), lazygit now shows an
error explaining that you need to commit inside the submodule first.

- **The stage key (space) and the stage-all key (`a`) now behave
consistently.** All of the above applies equally whether you act on the
submodule directly or use "stage all", and "stage all" no longer gets
stuck or behaves differently from the stage key just because a submodule
with uncommitted changes is present in the list.

Fixes #3641.

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,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 {

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

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(),
+			)
+	},
+})

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

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(),
+			)
+	},
+})

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,