Fix discarding submodule changes in nested folders

[brandondong]

• PR Description

This PR addresses #3951.

The current rules for discarding submodule changes is that no other changed item must be also selected. There are some bugs with the current implementation when submodules are in folders. The filed issue goes into more detail.

As part of this PR, I also tentatively changed the disabled message ("Range select not supported for submodules" -> "Multiselection not supported for submodules"). The former was not quite accurate because you could select a single line (folder) but the reset action still needs to be disabled (folder contains submodule change and some other change). Not sure if there is some better phrasing.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[brandondong]

Using a hardcoded folder prevents calling CloneIntoSubmodule twice in one test.

[stefanhaller]

Sorry for reviewing with such a long delay; just stumbled over this when looking over the open PRs. Better late than never I suppose...

This looks very good, thanks. A few smaller things below. Also, don't forget to rebase onto current master.

It would also be good if the commit message of the first commit could contain an explanation of what the problems are that are fixed by this. Basically what you said in the PR description and/or the issue. I prefer to have this in the commit message so that it's easier to make sense of it when blaming code.

[stefanhaller]

This feels overly complicated to me, there must be a simpler way to express this logic. Is the following equivalent?

	fileCount := 0
	submoduleCount := 0

	for _, node := range selectedNodes {
		_ = node.ForEachFile(func(f *models.File) error {
			if f.IsSubmodule(submodules) {
				submoduleCount++
			} else {
				fileCount++
			}
			return nil
		})
		if submoduleCount > 0 && (fileCount > 0 || submoduleCount > 1) {
			return &types.DisabledReason{Text: self.c.Tr.MultiSelectNotSupportedForSubmodules}
		}
	}

[brandondong]

In the case where a range select contains two items, a folder and a nested submodule change, the proposed snippet will incorrectly reject an attempt to discard.

[brandondong]

One approach would be to call normalisedSelectedNodes before and then it should be equivalent.

[stefanhaller]

In the case where a range select contains two items, a folder and a nested submodule change, the proposed snippet will incorrectly reject an attempt to discard.

I see. I do wonder how relevant this scenario is in practice, but if we can support it, why not. But if you find it important, then it would be good to add a test that catches it; I tried the above version in your branch, and all tests passed.

One approach would be to call normalisedSelectedNodes before and then it should be equivalent.

Either that, or something like the following:

hasFiles := false
uniqueSelectedSubmodules := set.New[*models.SubmoduleConfig]()

for _, node := range selectedNodes {
	_ = node.ForEachFile(func(f *models.File) error {
		if submodule := f.SubmoduleConfig(submodules); submodule != nil {
			uniqueSelectedSubmodules.Add(submodule)
		} else {
			hasFiles = true
		}
		return nil
	})
	if uniqueSelectedSubmodules.Len() > 0 && (hasFiles || uniqueSelectedSubmodules.Len() > 1) {
		return &types.DisabledReason{Text: self.c.Tr.MultiSelectNotSupportedForSubmodules}
	}
}

Set doesn't have a Len method yet, so this would have to be added:

diff --git i/vendor/github.com/jesseduffield/generics/set/set.go w/vendor/github.com/jesseduffield/generics/set/set.go
index 3e1bb69a3..0f0f4beb1 100644
--- i/vendor/github.com/jesseduffield/generics/set/set.go
+++ w/vendor/github.com/jesseduffield/generics/set/set.go
@@ -39,6 +39,10 @@ func (s *Set[T]) Includes(value T) bool {
 	return s.hashMap[value]
 }
 
+func (s *Set[T]) Len() int {
+	return len(s.hashMap)
+}
+
 // output slice is not necessarily in the same order that items were added
 func (s *Set[T]) ToSlice() []T {
 	return maps.Keys(s.hashMap)

[brandondong]

Sounds good. I opened a PR to add a Len() function (jesseduffield/generics#1) and updated the integration test.

[brandondong]

Dependency bumped and suggested approach applied.

[stefanhaller]

I found the name of this function a bit confusing at first, until I realized that it means "find first file that matches the given predicate". I wonder if there's some way to make this more obvious. Maybe appending By is already enough? This seems to be a convention used by samber/lo, e.g. lo.FindKeyBy.

I'd also rename test to predicate, but I see there's already precedence for the name test in this file. (Though I wouldn't mind a separate cleanup commit that renames them all to predicate, but maybe that's just personal taste. I'll leave that to you to decide.)

[stefanhaller]

Fantastic work, thanks for addressing all my nitpicks. I made one last minor tweak to the commit history (moving the cleanup commit first).

Looking forward to your next contribution!