I applaud you for stepping out of your way to add these comments! Maybe ErrorHint() can get one too?

Pull this comment above the struct.

Do we still need Values.opaque after this change?

jhk

Yes, I think there is something there. We call .setToDefault in two places:

1. in SettingsValues.Init, at which point the cluster version is generally initialized
2. in the settings updater, which is even later

I think we could pass the default through the (*Settings).Values and use setToDefault here then. I.e. this code

cockroach/pkg/settings/cluster/cluster_settings.go

Lines 150 to 153 in 7bed111

	sv := &s.SV
	s.Version = clusterversion.MakeVersionHandleWithOverride(
	&s.SV, binaryVersion, binaryMinSupportedVersion)
	sv.Init(s.Version)

would do something like

ClusterVersionSetting.Override(&st.SV, "19.2-1")
sv.Init(handle)

Also look at this code, it's basically setting the version like any other setting, really looks like everything here should go away and the setting should look like any other:

cockroach/pkg/settings/cluster/settings.go

Lines 135 to 150 in 6ae15a7

	// InitializeVersion initializes the Version field of this setting. Before this
	// method has been called, usage of the Version field is illegal and leads to a
	// fatal error.
	func (s *Settings) InitializeVersion(cv ClusterVersion) error {
	b, err := protoutil.Marshal(&cv)
	if err != nil {
	return err
	}
	// Note that we don't call `updater.ResetRemaining()`.
	updater := settings.NewUpdater(&s.SV)
	if err := updater.Set(KeyVersionSetting, string(b), version.Typ()); err != nil {
	return err
	}
	s.Version.baseVersion.Store(&cv)
	return nil
	}