Refactor INI package (first step) (#25024)

The INI package has many bugs and quirks, and in fact it is
unmaintained.

This PR is the first step for the INI package refactoring: 

* Use Gitea's "config_provider" to provide INI access
* Deprecate the INI package by golangci.yml rule

.golangci.yml

@@ -86,6 +86,7 @@ linters-settings:
       - io/ioutil: "use os or io instead"
       - golang.org/x/exp: "it's experimental and unreliable."
       - code.gitea.io/gitea/modules/git/internal: "do not use the internal package, use AddXxx function instead"
+      - gopkg.in/ini.v1: "do not use the ini package, use gitea's config system instead"
 
 issues:
   max-issues-per-linter: 0

build/backport-locales.go

@@ -12,7 +12,7 @@ import (
 	"path/filepath"
 	"strings"
 
-	"gopkg.in/ini.v1"
+	"code.gitea.io/gitea/modules/setting"
 )
 
 func main() {
@@ -22,14 +22,13 @@ func main() {
 		os.Exit(1)
 	}
 
-	ini.PrettyFormat = false
 	mustNoErr := func(err error) {
 		if err != nil {
 			panic(err)
 		}
 	}
-	collectInis := func(ref string) map[string]*ini.File {
-		inis := map[string]*ini.File{}
+	collectInis := func(ref string) map[string]setting.ConfigProvider {
+		inis := map[string]setting.ConfigProvider{}
 		err := filepath.WalkDir("options/locale", func(path string, d os.DirEntry, err error) error {
 			if err != nil {
 				return err
@@ -37,10 +36,7 @@ func main() {
 			if d.IsDir() || !strings.HasSuffix(d.Name(), ".ini") {
 				return nil
 			}
-			cfg, err := ini.LoadSources(ini.LoadOptions{
-				IgnoreInlineComment:         true,
-				UnescapeValueCommentSymbols: true,
-			}, path)
+			cfg, err := setting.NewConfigProviderForLocale(path)
 			mustNoErr(err)
 			inis[path] = cfg
 			fmt.Printf("collecting: %s @ %s\n", path, ref)

contrib/environment-to-ini/environment-to-ini.go

@@ -9,10 +9,8 @@ import (
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
-	"code.gitea.io/gitea/modules/util"
 
 	"github.com/urfave/cli"
-	"gopkg.in/ini.v1"
 )
 
 // EnvironmentPrefix environment variables prefixed with this represent ini values to write
@@ -97,19 +95,10 @@ func runEnvironmentToIni(c *cli.Context) error {
 	providedWorkPath := c.String("work-path")
 	setting.SetCustomPathAndConf(providedCustom, providedConf, providedWorkPath)
 
-	cfg := ini.Empty()
-	confFileExists, err := util.IsFile(setting.CustomConf)
+	cfg, err := setting.NewConfigProviderFromFile(&setting.Options{CustomConf: setting.CustomConf, AllowEmpty: true})
 	if err != nil {
-		log.Fatal("Unable to check if %s is a file. Error: %v", setting.CustomConf, err)
+		log.Fatal("Failed to load custom conf '%s': %v", setting.CustomConf, err)
 	}
-	if confFileExists {
-		if err := cfg.Append(setting.CustomConf); err != nil {
-			log.Fatal("Failed to load custom conf '%s': %v", setting.CustomConf, err)
-		}
-	} else {
-		log.Warn("Custom config '%s' not found, ignore this if you're running first time", setting.CustomConf)
-	}
-	cfg.NameMapper = ini.SnackCase
 
 	prefixGitea := c.String("prefix") + "__"
 	suffixFile := "__FILE"

modules/repository/repo.go

@@ -27,7 +27,7 @@ import (
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/util"
 
-	"gopkg.in/ini.v1"
+	"gopkg.in/ini.v1" //nolint:depguard
 )
 
 /*
@@ -241,7 +241,7 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User,
 // cleanUpMigrateGitConfig removes mirror info which prevents "push --all".
 // This also removes possible user credentials.
 func cleanUpMigrateGitConfig(configPath string) error {
-	cfg, err := ini.Load(configPath)
+	cfg, err := ini.Load(configPath) // FIXME: the ini package doesn't really work with git config files
 	if err != nil {
 		return fmt.Errorf("open config file: %w", err)
 	}

modules/setting/config_env.go

@@ -10,8 +10,6 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/modules/log"
-
-	"gopkg.in/ini.v1"
 )
 
 const escapeRegexpString = "_0[xX](([0-9a-fA-F][0-9a-fA-F])+)_"
@@ -89,7 +87,7 @@ func decodeEnvironmentKey(prefixGitea, suffixFile, envKey string) (ok bool, sect
 	return ok, section, key, useFileValue
 }
 
-func EnvironmentToConfig(cfg *ini.File, prefixGitea, suffixFile string, envs []string) (changed bool) {
+func EnvironmentToConfig(cfg ConfigProvider, prefixGitea, suffixFile string, envs []string) (changed bool) {
 	for _, kv := range envs {
 		idx := strings.IndexByte(kv, '=')
 		if idx < 0 {

modules/setting/config_env_test.go

@@ -8,7 +8,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"gopkg.in/ini.v1"
 )
 
 func TestDecodeEnvSectionKey(t *testing.T) {
@@ -71,15 +70,15 @@ func TestDecodeEnvironmentKey(t *testing.T) {
 }
 
 func TestEnvironmentToConfig(t *testing.T) {
-	cfg := ini.Empty()
+	cfg, _ := NewConfigProviderFromData("")
 
 	changed := EnvironmentToConfig(cfg, "GITEA__", "__FILE", nil)
 	assert.False(t, changed)
 
-	cfg, err := ini.Load([]byte(`
+	cfg, err := NewConfigProviderFromData(`
 [sec]
 key = old
-`))
+`)
 	assert.NoError(t, err)
 
 	changed = EnvironmentToConfig(cfg, "GITEA__", "__FILE", []string{"GITEA__sec__key=new"})

modules/setting/config_provider.go

@@ -8,36 +8,71 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"time"
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/util"
 
-	ini "gopkg.in/ini.v1"
+	"gopkg.in/ini.v1" //nolint:depguard
 )
 
+type ConfigKey interface {
+	Name() string
+	Value() string
+	SetValue(v string)
+
+	In(defaultVal string, candidates []string) string
+	String() string
+	Strings(delim string) []string
+
+	MustString(defaultVal string) string
+	MustBool(defaultVal ...bool) bool
+	MustInt(defaultVal ...int) int
+	MustInt64(defaultVal ...int64) int64
+	MustDuration(defaultVal ...time.Duration) time.Duration
+}
+
 type ConfigSection interface {
 	Name() string
-	MapTo(interface{}) error
+	MapTo(any) error
 	HasKey(key string) bool
-	NewKey(name, value string) (*ini.Key, error)
-	Key(key string) *ini.Key
-	Keys() []*ini.Key
-	ChildSections() []*ini.Section
+	NewKey(name, value string) (ConfigKey, error)
+	Key(key string) ConfigKey
+	Keys() []ConfigKey
+	ChildSections() []ConfigSection
 }
 
 // ConfigProvider represents a config provider
 type ConfigProvider interface {
 	Section(section string) ConfigSection
+	Sections() []ConfigSection
 	NewSection(name string) (ConfigSection, error)
 	GetSection(name string) (ConfigSection, error)
 	Save() error
+	SaveTo(filename string) error
 }
 
+type iniConfigProvider struct {
+	opts    *Options
+	ini     *ini.File
+	newFile bool // whether the file has not existed previously
+}
+
+type iniConfigSection struct {
+	sec *ini.Section
+}
+
+var (
+	_ ConfigProvider = (*iniConfigProvider)(nil)
+	_ ConfigSection  = (*iniConfigSection)(nil)
+	_ ConfigKey      = (*ini.Key)(nil)
+)
+
 // ConfigSectionKey only searches the keys in the given section, but it is O(n).
 // ini package has a special behavior:  with "[sec] a=1" and an empty "[sec.sub]",
 // then in "[sec.sub]", Key()/HasKey() can always see "a=1" because it always tries parent sections.
 // It returns nil if the key doesn't exist.
-func ConfigSectionKey(sec ConfigSection, key string) *ini.Key {
+func ConfigSectionKey(sec ConfigSection, key string) ConfigKey {
 	if sec == nil {
 		return nil
 	}
@@ -64,7 +99,7 @@ func ConfigSectionKeyString(sec ConfigSection, key string, def ...string) string
 // and the returned key is safe to be used with "MustXxx", it doesn't change the parent's values.
 // Otherwise, ini.Section.Key().MustXxx would pollute the parent section's keys.
 // It never returns nil.
-func ConfigInheritedKey(sec ConfigSection, key string) *ini.Key {
+func ConfigInheritedKey(sec ConfigSection, key string) ConfigKey {
 	k := sec.Key(key)
 	if k != nil && k.String() != "" {
 		newKey, _ := sec.NewKey(k.Name(), k.String())
@@ -85,41 +120,64 @@ func ConfigInheritedKeyString(sec ConfigSection, key string, def ...string) stri
 	return ""
 }
 
-type iniFileConfigProvider struct {
-	opts *Options
-	*ini.File
-	newFile bool // whether the file has not existed previously
+func (s *iniConfigSection) Name() string {
+	return s.sec.Name()
 }
 
-// NewConfigProviderFromData this function is only for testing
+func (s *iniConfigSection) MapTo(v any) error {
+	return s.sec.MapTo(v)
+}
+
+func (s *iniConfigSection) HasKey(key string) bool {
+	return s.sec.HasKey(key)
+}
+
+func (s *iniConfigSection) NewKey(name, value string) (ConfigKey, error) {
+	return s.sec.NewKey(name, value)
+}
+
+func (s *iniConfigSection) Key(key string) ConfigKey {
+	return s.sec.Key(key)
+}
+
+func (s *iniConfigSection) Keys() (keys []ConfigKey) {
+	for _, k := range s.sec.Keys() {
+		keys = append(keys, k)
+	}
+	return keys
+}
+
+func (s *iniConfigSection) ChildSections() (sections []ConfigSection) {
+	for _, s := range s.sec.ChildSections() {
+		sections = append(sections, &iniConfigSection{s})
+	}
+	return sections
+}
+
+// NewConfigProviderFromData this function is mainly for testing purpose
 func NewConfigProviderFromData(configContent string) (ConfigProvider, error) {
-	var cfg *ini.File
-	var err error
-	if configContent == "" {
-		cfg = ini.Empty()
-	} else {
-		cfg, err = ini.Load(strings.NewReader(configContent))
-		if err != nil {
-			return nil, err
-		}
+	cfg, err := ini.Load(strings.NewReader(configContent))
+	if err != nil {
+		return nil, err
 	}
 	cfg.NameMapper = ini.SnackCase
-	return &iniFileConfigProvider{
-		File:    cfg,
+	return &iniConfigProvider{
+		ini:     cfg,
 		newFile: true,
 	}, nil
 }
 
 type Options struct {
-	CustomConf                string // the ini file path
-	AllowEmpty                bool   // whether not finding configuration files is allowed (only true for the tests)
-	ExtraConfig               string
-	DisableLoadCommonSettings bool
+	CustomConf  string // the ini file path
+	AllowEmpty  bool   // whether not finding configuration files is allowed
+	ExtraConfig string
+
+	DisableLoadCommonSettings bool // only used by "Init()", not used by "NewConfigProvider()"
 }
 
-// newConfigProviderFromFile load configuration from file.
+// NewConfigProviderFromFile load configuration from file.
 // NOTE: do not print any log except error.
-func newConfigProviderFromFile(opts *Options) (*iniFileConfigProvider, error) {
+func NewConfigProviderFromFile(opts *Options) (ConfigProvider, error) {
 	cfg := ini.Empty()
 	newFile := true
 
@@ -147,61 +205,77 @@ func newConfigProviderFromFile(opts *Options) (*iniFileConfigProvider, error) {
 	}
 
 	cfg.NameMapper = ini.SnackCase
-	return &iniFileConfigProvider{
+	return &iniConfigProvider{
 		opts:    opts,
-		File:    cfg,
+		ini:     cfg,
 		newFile: newFile,
 	}, nil
 }
 
-func (p *iniFileConfigProvider) Section(section string) ConfigSection {
-	return p.File.Section(section)
+func (p *iniConfigProvider) Section(section string) ConfigSection {
+	return &iniConfigSection{sec: p.ini.Section(section)}
 }
 
-func (p *iniFileConfigProvider) NewSection(name string) (ConfigSection, error) {
-	return p.File.NewSection(name)
+func (p *iniConfigProvider) Sections() (sections []ConfigSection) {
+	for _, s := range p.ini.Sections() {
+		sections = append(sections, &iniConfigSection{s})
+	}
+	return sections
 }
 
-func (p *iniFileConfigProvider) GetSection(name string) (ConfigSection, error) {
-	return p.File.GetSection(name)
+func (p *iniConfigProvider) NewSection(name string) (ConfigSection, error) {
+	sec, err := p.ini.NewSection(name)
+	if err != nil {
+		return nil, err
+	}
+	return &iniConfigSection{sec: sec}, nil
 }
 
-// Save save the content into file
-func (p *iniFileConfigProvider) Save() error {
-	if p.opts.CustomConf == "" {
+func (p *iniConfigProvider) GetSection(name string) (ConfigSection, error) {
+	sec, err := p.ini.GetSection(name)
+	if err != nil {
+		return nil, err
+	}
+	return &iniConfigSection{sec: sec}, nil
+}
+
+// Save saves the content into file
+func (p *iniConfigProvider) Save() error {
+	filename := p.opts.CustomConf
+	if filename == "" {
 		if !p.opts.AllowEmpty {
 			return fmt.Errorf("custom config path must not be empty")
 		}
 		return nil
 	}
-
 	if p.newFile {
-		if err := os.MkdirAll(filepath.Dir(CustomConf), os.ModePerm); err != nil {
-			return fmt.Errorf("failed to create '%s': %v", CustomConf, err)
+		if err := os.MkdirAll(filepath.Dir(filename), os.ModePerm); err != nil {
+			return fmt.Errorf("failed to create '%s': %v", filename, err)
 		}
 	}
-	if err := p.SaveTo(p.opts.CustomConf); err != nil {
-		return fmt.Errorf("failed to save '%s': %v", p.opts.CustomConf, err)
+	if err := p.ini.SaveTo(filename); err != nil {
+		return fmt.Errorf("failed to save '%s': %v", filename, err)
 	}
 
 	// Change permissions to be more restrictive
-	fi, err := os.Stat(CustomConf)
+	fi, err := os.Stat(filename)
 	if err != nil {
 		return fmt.Errorf("failed to determine current conf file permissions: %v", err)
 	}
 
 	if fi.Mode().Perm() > 0o600 {
-		if err = os.Chmod(CustomConf, 0o600); err != nil {
+		if err = os.Chmod(filename, 0o600); err != nil {
 			log.Warn("Failed changing conf file permissions to -rw-------. Consider changing them manually.")
 		}
 	}
 	return nil
 }
 
-// a file is an implementation ConfigProvider and other implementations are possible, i.e. from docker, k8s, …
-var _ ConfigProvider = &iniFileConfigProvider{}
+func (p *iniConfigProvider) SaveTo(filename string) error {
+	return p.ini.SaveTo(filename)
+}
 
-func mustMapSetting(rootCfg ConfigProvider, sectionName string, setting interface{}) {
+func mustMapSetting(rootCfg ConfigProvider, sectionName string, setting any) {
 	if err := rootCfg.Section(sectionName).MapTo(setting); err != nil {
 		log.Fatal("Failed to map %s settings: %v", sectionName, err)
 	}
@@ -219,3 +293,23 @@ func deprecatedSettingDB(rootCfg ConfigProvider, oldSection, oldKey string) {
 		log.Error("Deprecated `[%s]` `%s` present which has been copied to database table sys_setting", oldSection, oldKey)
 	}
 }
+
+// NewConfigProviderForLocale loads locale configuration from source and others. "string" if for a local file path, "[]byte" is for INI content
+func NewConfigProviderForLocale(source any, others ...any) (ConfigProvider, error) {
+	iniFile, err := ini.LoadSources(ini.LoadOptions{
+		IgnoreInlineComment:         true,
+		UnescapeValueCommentSymbols: true,
+	}, source, others...)
+	if err != nil {
+		return nil, fmt.Errorf("unable to load locale ini: %w", err)
+	}
+	iniFile.BlockMode = false
+	return &iniConfigProvider{
+		ini:     iniFile,
+		newFile: true,
+	}, nil
+}
+
+func init() {
+	ini.PrettyFormat = false
+}

modules/setting/config_provider_test.go

@@ -4,6 +4,7 @@
 package setting
 
 import (
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -64,3 +65,57 @@ key = 123
 	assert.Equal(t, "", ConfigSectionKeyString(sec, "empty"))
 	assert.Equal(t, "def", ConfigSectionKeyString(secSub, "empty"))
 }
+
+func TestNewConfigProviderFromFile(t *testing.T) {
+	_, err := NewConfigProviderFromFile(&Options{CustomConf: "no-such.ini", AllowEmpty: false})
+	assert.ErrorContains(t, err, "unable to find configuration file")
+
+	// load non-existing file and save
+	testFile := t.TempDir() + "/test.ini"
+	testFile1 := t.TempDir() + "/test1.ini"
+	cfg, err := NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true})
+	assert.NoError(t, err)
+
+	sec, _ := cfg.NewSection("foo")
+	_, _ = sec.NewKey("k1", "a")
+	assert.NoError(t, cfg.Save())
+	_, _ = sec.NewKey("k2", "b")
+	assert.NoError(t, cfg.SaveTo(testFile1))
+
+	bs, err := os.ReadFile(testFile)
+	assert.NoError(t, err)
+	assert.Equal(t, "[foo]\nk1=a\n", string(bs))
+
+	bs, err = os.ReadFile(testFile1)
+	assert.NoError(t, err)
+	assert.Equal(t, "[foo]\nk1=a\nk2=b\n", string(bs))
+
+	// load existing file and save
+	cfg, err = NewConfigProviderFromFile(&Options{CustomConf: testFile, AllowEmpty: true})
+	assert.NoError(t, err)
+	assert.Equal(t, "a", cfg.Section("foo").Key("k1").String())
+	sec, _ = cfg.NewSection("bar")
+	_, _ = sec.NewKey("k1", "b")
+	assert.NoError(t, cfg.Save())
+	bs, err = os.ReadFile(testFile)
+	assert.NoError(t, err)
+	assert.Equal(t, "[foo]\nk1=a\n\n[bar]\nk1=b\n", string(bs))
+}
+
+func TestNewConfigProviderForLocale(t *testing.T) {
+	// load locale from file
+	localeFile := t.TempDir() + "/locale.ini"
+	_ = os.WriteFile(localeFile, []byte(`k1=a`), 0o644)
+	cfg, err := NewConfigProviderForLocale(localeFile)
+	assert.NoError(t, err)
+	assert.Equal(t, "a", cfg.Section("").Key("k1").String())
+
+	// load locale from bytes
+	cfg, err = NewConfigProviderForLocale([]byte("k1=foo\nk2=bar"))
+	assert.NoError(t, err)
+	assert.Equal(t, "foo", cfg.Section("").Key("k1").String())
+	cfg, err = NewConfigProviderForLocale([]byte("k1=foo\nk2=bar"), []byte("k2=xxx"))
+	assert.NoError(t, err)
+	assert.Equal(t, "foo", cfg.Section("").Key("k1").String())
+	assert.Equal(t, "xxx", cfg.Section("").Key("k2").String())
+}

modules/setting/setting.go

@@ -201,7 +201,7 @@ func Init(opts *Options) {
 		opts.CustomConf = CustomConf
 	}
 	var err error
-	CfgProvider, err = newConfigProviderFromFile(opts)
+	CfgProvider, err = NewConfigProviderFromFile(opts)
 	if err != nil {
 		log.Fatal("Init[%v]: %v", opts, err)
 	}

modules/translation/i18n/localestore.go

@@ -7,8 +7,7 @@ import (
 	"fmt"
 
 	"code.gitea.io/gitea/modules/log"
-
-	"gopkg.in/ini.v1"
+	"code.gitea.io/gitea/modules/setting"
 )
 
 // This file implements the static LocaleStore that will not watch for changes
@@ -47,14 +46,10 @@ func (store *localeStore) AddLocaleByIni(langName, langDesc string, source, more
 	l := &locale{store: store, langName: langName, idxToMsgMap: make(map[int]string)}
 	store.localeMap[l.langName] = l
 
-	iniFile, err := ini.LoadSources(ini.LoadOptions{
-		IgnoreInlineComment:         true,
-		UnescapeValueCommentSymbols: true,
-	}, source, moreSource)
+	iniFile, err := setting.NewConfigProviderForLocale(source, moreSource)
 	if err != nil {
 		return fmt.Errorf("unable to load ini: %w", err)
 	}
-	iniFile.BlockMode = false
 
 	for _, section := range iniFile.Sections() {
 		for _, key := range section.Keys() {

routers/install/install.go

@@ -35,7 +35,6 @@ import (
 	"code.gitea.io/gitea/services/forms"
 
 	"gitea.com/go-chi/session"
-	"gopkg.in/ini.v1"
 )
 
 const (
@@ -371,17 +370,11 @@ func SubmitInstall(ctx *context.Context) {
 	}
 
 	// Save settings.
-	cfg := ini.Empty()
-	isFile, err := util.IsFile(setting.CustomConf)
+	cfg, err := setting.NewConfigProviderFromFile(&setting.Options{CustomConf: setting.CustomConf, AllowEmpty: true})
 	if err != nil {
-		log.Error("Unable to check if %s is a file. Error: %v", setting.CustomConf, err)
-	}
-	if isFile {
-		// Keeps custom settings if there is already something.
-		if err = cfg.Append(setting.CustomConf); err != nil {
-			log.Error("Failed to load custom conf '%s': %v", setting.CustomConf, err)
-		}
+		log.Error("Failed to load custom conf '%s': %v", setting.CustomConf, err)
 	}
+
 	cfg.Section("database").Key("DB_TYPE").SetValue(setting.Database.Type.String())
 	cfg.Section("database").Key("HOST").SetValue(setting.Database.Host)
 	cfg.Section("database").Key("NAME").SetValue(setting.Database.Name)