Fix LocalFlags().NFlags to count the number of local flags that have been set explicitly

[JunNishimura]

Because the problem is a bit more complex, I have detailed the modifications. It is long, but it would be thankful if you would read it.

Issue

This issue is addressed in #1315.

LocalFlags().NFlag and LocalNonPersistentFlags().NFlag are provided to count the number of local flags that have been set explicitly, but they do not work as expected.

LocalFlags().NFlag() always return 0.

Cause of the issue

NFlag method is defined as follows,

func (f *FlagSet) NFlag() int { return len(f.actual) }

From the definition of NFlag, it looks like LocalFlags().actual is not set.

f.actual is set in the Set method.

func (f *FlagSet) Set(name, value string) error {
...
	if !flag.Changed {
		if f.actual == nil {
			f.actual = make(map[NormalizedName]*Flag)
		}
		f.actual[normalName] = flag
		f.orderedActual = append(f.orderedActual, flag)

		flag.Changed = true
	}
...
}

This Set method is called inside the processing of c.Flags().Parse(args).

c.flags and c.lflags are different objects, and Parse method is performed on only c.flags (not on c.lflags). That's why c.lflags remain empty and LocalFlags().NFlag() return 0.

How I solved

1. execute Parse for lflags

As I mentioned above, the cause of issue is that Parse method is not performed on lflags. So I
change it to perform Parse method on lflags.
https://github.com/spf13/cobra/pull/1999/files#diff-4d7d1923693fc5ce892add2ea2907a744e77ea0b50c1939ccc5067cb48a466a3R1885

2. fix addToLocal logic in LocalFlags function

Performing Parse method on lflags is not enough to set lflags.actual.

As I showed above, actual field value is set only if flag.Changed == false in the Set method. The flags in c.lflags are pointers and therefore have the same value as the flags in c.flags. In other words, at the time of LocalFlags().Parse(), flag.Changed is true, so exit Set method before setting the value in lflags.actual. Therefore, I assign false to f.Changed before adding it to c.lflags.
https://github.com/spf13/cobra/pull/1999/files#diff-4d7d1923693fc5ce892add2ea2907a744e77ea0b50c1939ccc5067cb48a466a3R1635

3. remove parent

Flags passed as arguments to lflags.Parse() must be limited to flags registered in c.lflags. Therefore, I added a process to exclude flags other than those in c.lflags from the passed arguments.
https://github.com/spf13/cobra/pull/1999/files#diff-4d7d1923693fc5ce892add2ea2907a744e77ea0b50c1939ccc5067cb48a466a3R1767-R1855

The above modifications to LocalFlags() are also made to LocalNonPersistentFlags().

Discussion

Setting f.Changed to false before adding flags to c.lflags may cause problems. This is because setting f.Changed back to true again assumes that the Parse method will be executed immediately afterwards.

I thought about creating a new flag object instead of using the flags in c.flags so that c.flags would not be affected, but some of completion unit tests did not pass.

newFlag := &flag.Flag{}
*newFlag = *f
newFlag.Changed = false
c.lflags.AddFlag(newFlag)

If you have any good ideas, I'd love to hear them.

Related Issue

• [Bug] LocalFlags().Visit / .NFlag failed, cannot count local flags have been set #1315

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[JunNishimura]

@johnSchnake
Due to the large number of revisions, there is no urgent need for a reply, but I would appreciate it if you check this PR.