ronantakizawa/codereview-bench huggingface dataset:
Repository: zyedidia/micro
File: internal/config/autosave.go (Go)

Code before:
)

var Autosave chan bool
var autotime int

// lock for autosave
var autolock sync.Mutex

func init() {
	Autosave = make(chan bool)
}

func SetAutoTime(a int) {
	autolock.Lock()
	autotime = a
	autolock.Unlock()
}

func StartAutoSave() {
	go func() {
		autolock.Lock()
		a := autotime
		autolock.Unlock()
		if a < 1 {
			return
		}
		for {
			time.Sleep(time.Duration(a) * time.Second)
			autolock.Lock()
			a := autotime
			autolock.Unlock()
			if a < 1 {
				break
			}
			Autosave <- true
		}
	}()
}


Review comment (bug):
Hmm, am I reading the code (not just your code) correctly: if `autosave` is already set to a positive value and we change it to another positive value, we spawn a new goroutine without stopping the old one? So both goroutines are now running? So autosave is actually done twice more often than it should? (And if we repeat that several more times, autosave will be done several times more often than it should, etc?)

So, all this looks broken, and your code seems to be a hack around this broken code, which fixes only a part of its broken behavior.

It seems the proper fix should be: let the goroutine run all the time, and just notify it about changes of the `autotime` value. The goroutine could use `select` to wait on both the timeout and the `autotime` change notifications.

Code after:

var Autosave chan bool
var autotime chan float64

func init() {
	Autosave = make(chan bool)
	autotime = make(chan float64)
}

func SetAutoTime(a float64) {
	autotime <- a
}

func StartAutoSave() {
	go func() {
		var a float64
		var t *time.Timer
		var elapsed <-chan time.Time
		for {
			select {
			case a = <-autotime:
				if t != nil {
					t.Stop()
					for len(elapsed) > 0 {
						<-elapsed
					}
				}
				if a > 0 {
					if t != nil {
						t.Reset(time.Duration(a * float64(time.Second)))
					} else {
						t = time.NewTimer(time.Duration(a * float64(time.Second)))
						elapsed = t.C
					}
				}
			case <-elapsed:
				if a > 0 {
					t.Reset(time.Duration(a * float64(time.Second)))
					Autosave <- true
				}
			}
		}
	}()
}
Repository: zyedidia/micro
File: internal/config/autosave.go (Go)

Code before:
)

var Autosave chan bool
var autotime chan float64

// lock for autosave
var autolock sync.Mutex

func init() {
	Autosave = make(chan bool)
	autotime = make(chan float64)
}

func SetAutoTime(a float64) {
	autotime <- a
}

func StartAutoSave() {
	go func() {
		var a float64
		var elapsed <-chan time.Time
		for {
			select {
			case a = <-autotime:
				if a > 0 {
					elapsed = time.After(time.Duration(a * float64(time.Second)))
				}
			case <-elapsed:
				if a > 0 {
					elapsed = time.After(time.Duration(a * float64(time.Second)))
					Autosave <- true
				}
			}
		}
	}()
}


Review comment (suggestion):
This seems to work correctly from the autosave perspective, however it seems a bit unclean: it doesn't stop the previous `elapsed` timer, so it will still fire after some time. Although it won't cause an unwanted autosave (thanks to the fact that the `elapsed` variable is now assigned to a different channel, so we will not wait on the old channel in next iterations of the loop), this also probably means that, since no one will read from the old channel when the old timer fires, the old timer will block forever on sending to the old channel, so it will not be garbage-collected even after it fires.

Code after:

var Autosave chan bool
var autotime chan float64

func init() {
	Autosave = make(chan bool)
	autotime = make(chan float64)
}

func SetAutoTime(a float64) {
	autotime <- a
}

func StartAutoSave() {
	go func() {
		var a float64
		var t *time.Timer
		var elapsed <-chan time.Time
		for {
			select {
			case a = <-autotime:
				if t != nil {
					t.Stop()
					for len(elapsed) > 0 {
						<-elapsed
					}
				}
				if a > 0 {
					if t != nil {
						t.Reset(time.Duration(a * float64(time.Second)))
					} else {
						t = time.NewTimer(time.Duration(a * float64(time.Second)))
						elapsed = t.C
					}
				}
			case <-elapsed:
				if a > 0 {
					t.Reset(time.Duration(a * float64(time.Second)))
					Autosave <- true
				}
			}
		}
	}()
}
Repository: zyedidia/micro
File: internal/action/command.go (Go)

Code before:
func reloadRuntime(reloadPlugins bool) {
	if reloadPlugins {
		err := config.RunPluginFn("deinit")
		if err != nil {
			screen.TermMessage(err)
		}
	}

	config.InitRuntimeFiles(true)

	if reloadPlugins {
		config.InitPlugins()
	}

	oldParsedSettings := config.ParsedSettings()

	err := config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	}

	parsedSettings := config.ParsedSettings()
	defaultSettings := config.DefaultAllSettings()
	for k := range defaultSettings {
		if _, ok := parsedSettings[k]; ok {
			SetGlobalOptionNative(k, parsedSettings[k])
			continue
		}
		if _, ok := oldParsedSettings[k]; ok {
			SetGlobalOptionNative(k, defaultSettings[k])
		}
	}

	if reloadPlugins {
		err = config.LoadAllPlugins()
		if err != nil {
			screen.TermMessage(err)
		}
	}

	InitBindings()
	InitCommands()

	if reloadPlugins {
		err = config.RunPluginFn("preinit")
		if err != nil {
			screen.TermMessage(err)
		}
		err = config.RunPluginFn("init")
		if err != nil {
			screen.TermMessage(err)

Review comment (bug):
Still buggy: some options are normally overridden per buffer in `NewBuffer()`, so they should be overridden after `reload` as well. Those are at least: `fileformat`, `fastdirty` if the file is large, `encoding` if the specified encoding is invalid.

In other words, if the user runs `reload` but hasn't changed anything in `settings.json` before that, `reload` should not change any settings.

Code after:

func reloadRuntime(reloadPlugins bool) {
	if reloadPlugins {
		err := config.RunPluginFn("deinit")
		if err != nil {
			screen.TermMessage(err)
		}
	}

	config.InitRuntimeFiles(true)

	if reloadPlugins {
		config.InitPlugins()
	}

	err := config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	} else {
		parsedSettings := config.ParsedSettings()
		defaultSettings := config.DefaultAllSettings()
		for k := range defaultSettings {
			if _, ok := config.VolatileSettings[k]; ok {
				// reload should not override volatile settings
				continue
			}

			if _, ok := parsedSettings[k]; ok {
				err = doSetGlobalOptionNative(k, parsedSettings[k])
			} else {
				err = doSetGlobalOptionNative(k, defaultSettings[k])
			}
			if err != nil {
				screen.TermMessage(err)
			}
		}
	}

	if reloadPlugins {
		err = config.LoadAllPlugins()
		if err != nil {
			screen.TermMessage(err)
		}
	}

	InitBindings()
	InitCommands()

	if reloadPlugins {
		err = config.RunPluginFn("preinit")
		if err != nil {
Repository: zyedidia/micro
File: internal/buffer/settings.go (Go)

Code before:
		case "dos":
			b.Endings = FFDos
		}
		b.isModified = true
	} else if option == "syntax" {
		if !nativeValue.(bool) {
			b.ClearMatches()
		} else {
			b.UpdateRules()
		}
	} else if option == "encoding" {
		_, err := htmlindex.Get(b.Settings["encoding"].(string))
		if err != nil {
			return err
		}
		b.isModified = true
	} else if option == "readonly" && b.Type.Kind == BTDefault.Kind {
		b.Type.Readonly = nativeValue.(bool)
	} else if option == "hlsearch" {
		for _, buf := range OpenBuffers {
			if b.SharedBuffer == buf.SharedBuffer {
				buf.HighlightSearch = nativeValue.(bool)
			}
		}
	} else {
		for _, pl := range config.Plugins {
			if option == pl.Name {
				if nativeValue.(bool) {
					if !pl.Loaded {
						pl.Load()
					}
					_, err := pl.Call("init")
					if err != nil && err != config.ErrNoSuchFunction {
						screen.TermMessage(err)
					}
				} else if !nativeValue.(bool) && pl.Loaded {
					_, err := pl.Call("deinit")
					if err != nil && err != config.ErrNoSuchFunction {
						screen.TermMessage(err)
					}
				}
			}
		}
	}

	if b.OptionCallback != nil {
		b.OptionCallback(option, nativeValue)
	}

	return nil
}

Review comment (suggestion):
We don't really check this return value (thus don't report the error to the user) in `reload`...

Code after:
		} else {
			b.DoSetOptionNative(k, v)
		}
	}
}

func (b *Buffer) DoSetOptionNative(option string, nativeValue interface{}) {
	if reflect.DeepEqual(b.Settings[option], nativeValue) {
		return
	}

	b.Settings[option] = nativeValue

	if option == "fastdirty" {
		if !nativeValue.(bool) {
			if b.Size() > LargeFileThreshold {
				b.Settings["fastdirty"] = true
			} else {
				if !b.isModified {
					calcHash(b, &b.origHash)
				} else {
					// prevent using an old stale origHash value
					b.origHash = [md5.Size]byte{}
				}
			}
		}
	} else if option == "statusline" {
		screen.Redraw()
	} else if option == "filetype" {
		b.ReloadSettings(false)
	} else if option == "fileformat" {
		switch b.Settings["fileformat"].(string) {
		case "unix":
			b.Endings = FFUnix
		case "dos":
			b.Endings = FFDos
		}
		b.isModified = true
	} else if option == "syntax" {
		if !nativeValue.(bool) {
			b.ClearMatches()
		} else {
			b.UpdateRules()
		}
	} else if option == "encoding" {
		b.isModified = true
	} else if option == "readonly" && b.Type.Kind == BTDefault.Kind {
		b.Type.Readonly = nativeValue.(bool)
	} else if option == "hlsearch" {
		for _, buf := range OpenBuffers {
			if b.SharedBuffer == buf.SharedBuffer {
Repository: zyedidia/micro
File: internal/action/command.go (Go)

Code before:

	config.InitRuntimeFiles(true)

	if reloadPlugins {
		config.InitPlugins()
	}

	err := config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	}

	parsedSettings := config.ParsedSettings()
	defaultSettings := config.DefaultAllSettings()
	for k := range defaultSettings {
		if k == "fileformat" {
			continue
		}
		if _, ok := parsedSettings[k]; ok {
			err = SetGlobalOptionNative(k, parsedSettings[k])
		} else {
			err = SetGlobalOptionNative(k, defaultSettings[k])
		}
		if err != nil {
			InfoBar.Error(err)
			return
		}
	}

	if reloadPlugins {
		err = config.LoadAllPlugins()
		if err != nil {
			screen.TermMessage(err)
		}
	}

	InitBindings()
	InitCommands()

	if reloadPlugins {
		err = config.RunPluginFn("preinit")
		if err != nil {
			screen.TermMessage(err)
		}
		err = config.RunPluginFn("init")
		if err != nil {
			screen.TermMessage(err)
		}
		err = config.RunPluginFn("postinit")
		if err != nil {
			screen.TermMessage(err)

Review comment (bug):
Maybe we shouldn't just interrupt reloading settings, bindings, colorscheme etc on the halfway because of this error, but just report it via `screen.TermMessage` and continue, like we do in other error cases in this function? (`screen.TermMessage`, unlike `InfoBar.Error`, will not overwrite previous error messages.)

Only that... I've just tried that (`screen.TermMessage` without return) myself, and I'm getting this `htmlindex: invalid encoding name` twice, not once. It turns out it is returned not only when setting an incorrect `encoding` value, but also when setting any `filetype` value (if there is an incorrect `encoding` value), since setting `filetype` triggers setting other options...

And obviously, even worse if I do the same (`screen.TermMessage` without return) below for per-buffer settings as well.

...This fact that `reload` triggers updating all options including `filetype` which itself, again, triggers updating all the same options, is worth thinking about. Perhaps we should indeed check and prevent setting an option whose value doesn't change (in both above 2 cases)?

Code after:
	}

	config.InitRuntimeFiles(true)

	if reloadPlugins {
		config.InitPlugins()
	}

	err := config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	} else {
		parsedSettings := config.ParsedSettings()
		defaultSettings := config.DefaultAllSettings()
		for k := range defaultSettings {
			if _, ok := config.VolatileSettings[k]; ok {
				// reload should not override volatile settings
				continue
			}

			if _, ok := parsedSettings[k]; ok {
				err = doSetGlobalOptionNative(k, parsedSettings[k])
			} else {
				err = doSetGlobalOptionNative(k, defaultSettings[k])
			}
			if err != nil {
				screen.TermMessage(err)
			}
		}
	}

	if reloadPlugins {
		err = config.LoadAllPlugins()
		if err != nil {
			screen.TermMessage(err)
		}
	}

	InitBindings()
	InitCommands()

	if reloadPlugins {
		err = config.RunPluginFn("preinit")
		if err != nil {
			screen.TermMessage(err)
		}
		err = config.RunPluginFn("init")
		if err != nil {
			screen.TermMessage(err)
		}
		err = config.RunPluginFn("postinit")
Repository: zyedidia/micro
File: internal/action/command.go (Go)

Code before:
func (h *BufPane) ReloadCmd(args []string) {
	reloadRuntime(true)
}

// ReloadConfig reloads only the configuration
func ReloadConfig() {
	reloadRuntime(false)
}

func reloadRuntime(reloadPlugins bool) {
	if reloadPlugins {
		err := config.RunPluginFn("deinit")
		if err != nil {
			screen.TermMessage(err)
		}
	}

	config.InitRuntimeFiles(true)

	if reloadPlugins {
		config.InitPlugins()
	}

	err := config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	}

	parsedSettings := config.ParsedSettings()
	defaultSettings := config.DefaultAllSettings()
	for k := range defaultSettings {
		if _, ok := parsedSettings[k]; ok {
			SetGlobalOptionNative(k, parsedSettings[k])
		} else {
			SetGlobalOptionNative(k, defaultSettings[k])
		}
	}

	if reloadPlugins {
		err = config.LoadAllPlugins()
		if err != nil {
			screen.TermMessage(err)
		}
	}

	InitBindings()
	InitCommands()

	if reloadPlugins {
		err = config.RunPluginFn("preinit")
		if err != nil {

Review comment (bug):
Just noticed that if we failed to read settings, we probably don't need to do the below steps with settings? (I hope `ReadSettings()` -> `json5.Unmarshal()` will not modify `parsedSettings` in this case?)

Code after:
// ReloadCmd reloads all files (syntax files, colorschemes, plugins...)
func (h *BufPane) ReloadCmd(args []string) {
	reloadRuntime(true)
}

// ReloadConfig reloads only the configuration
func ReloadConfig() {
	reloadRuntime(false)
}

func reloadRuntime(reloadPlugins bool) {
	if reloadPlugins {
		err := config.RunPluginFn("deinit")
		if err != nil {
			screen.TermMessage(err)
		}
	}

	config.InitRuntimeFiles(true)

	if reloadPlugins {
		config.InitPlugins()
	}

	err := config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	} else {
		parsedSettings := config.ParsedSettings()
		defaultSettings := config.DefaultAllSettings()
		for k := range defaultSettings {
			if _, ok := config.VolatileSettings[k]; ok {
				// reload should not override volatile settings
				continue
			}

			if _, ok := parsedSettings[k]; ok {
				err = doSetGlobalOptionNative(k, parsedSettings[k])
			} else {
				err = doSetGlobalOptionNative(k, defaultSettings[k])
			}
			if err != nil {
				screen.TermMessage(err)
			}
		}
	}

	if reloadPlugins {
		err = config.LoadAllPlugins()
		if err != nil {
			screen.TermMessage(err)
Repository: zyedidia/micro
File: internal/action/command.go (Go)

Code before:
		err = config.WriteSettings(filepath.Join(config.ConfigDir, "settings.json"))
	}

	return err
}

func SetGlobalOptionNative(option string, nativeValue interface{}) error {
	return setGlobalOptionNative(option, nativeValue, true)
}

func SetGlobalOption(option, value string) error {
	if _, ok := config.GlobalSettings[option]; !ok {
		return config.ErrInvalidOption
	}

	nativeValue, err := config.GetNativeValue(option, config.GlobalSettings[option], value)
	if err != nil {
		return err
	}

	return SetGlobalOptionNative(option, nativeValue)
}

// ResetCmd resets a setting to its default value
func (h *BufPane) ResetCmd(args []string) {
	if len(args) < 1 {
		InfoBar.Error("Not enough arguments")
		return
	}

	option := args[0]

	defaultGlobals := config.DefaultGlobalSettings()
	defaultLocals := config.DefaultCommonSettings()

	if _, ok := defaultGlobals[option]; ok {
		SetGlobalOptionNative(option, defaultGlobals[option])
		return
	}
	if _, ok := defaultLocals[option]; ok {
		h.Buf.SetOptionNative(option, defaultLocals[option])
		return
	}
	InfoBar.Error(config.ErrInvalidOption)
}

// SetCmd sets an option
func (h *BufPane) SetCmd(args []string) {
	if len(args) < 2 {
		InfoBar.Error("Not enough arguments")
		return

Review comment (suggestion):
How about:
```
if write {
	return config.WriteSettings(...)
}
return nil
```

Code after:
	// ...if it's not local continue with the globals...
	if err := doSetGlobalOptionNative(option, nativeValue); err != nil {
		return err
	}

	// ...at last check the buffer locals
	for _, b := range buffer.OpenBuffers {
		b.DoSetOptionNative(option, nativeValue)
		delete(b.LocalSettings, option)
	}

	return config.WriteSettings(filepath.Join(config.ConfigDir, "settings.json"))
}

func SetGlobalOption(option, value string) error {
	if _, ok := config.GlobalSettings[option]; !ok {
		return config.ErrInvalidOption
	}

	nativeValue, err := config.GetNativeValue(option, config.GlobalSettings[option], value)
	if err != nil {
		return err
	}

	return SetGlobalOptionNative(option, nativeValue)
}

// ResetCmd resets a setting to its default value
func (h *BufPane) ResetCmd(args []string) {
	if len(args) < 1 {
		InfoBar.Error("Not enough arguments")
		return
	}

	option := args[0]
	defaults := config.DefaultAllSettings()

	if _, ok := defaults[option]; ok {
		SetGlobalOptionNative(option, defaults[option])
		return
	}
	InfoBar.Error(config.ErrInvalidOption)
}

// SetCmd sets an option
func (h *BufPane) SetCmd(args []string) {
	if len(args) < 2 {
		InfoBar.Error("Not enough arguments")
		return
	}
Repository: zyedidia/micro
File: internal/action/command.go (Go)

Code before:
						screen.TermMessage(err)
					}
				}
			}
		}
	}

	return nil
}

func processGlobalOptionNative(option string, nativeValue interface{}, overwriteLocals bool, write bool) error {
	// check for local option first...
	for _, s := range config.LocalSettings {
		if s == option {
			if overwriteLocals {
				return MainTab().CurPane().Buf.SetOptionNative(option, nativeValue, overwriteLocals)
			} else {
				return nil
			}
		}
	}

	// ...if it's not local continue with the globals...
	if !reflect.DeepEqual(config.GlobalSettings[option], nativeValue) {
		if err := setGlobalOptionNative(option, nativeValue); err != nil {
			return err
		}
	}

	// ...at last check the buffer locals
	for _, b := range buffer.OpenBuffers {
		if _, ok := b.LocalSettings[option]; ok && !overwriteLocals {
			continue
		}
		if err := b.SetOptionNative(option, nativeValue, overwriteLocals); err != nil {
			return err
		}
		delete(b.LocalSettings, option)
	}

	if write {
		return config.WriteSettings(filepath.Join(config.ConfigDir, "settings.json"))
	}

	return nil
}

func SetGlobalOptionNative(option string, nativeValue interface{}) error {
	return processGlobalOptionNative(option, nativeValue, true, true)
}


Review comment (refactor):
I'm struggling, if this really is a good location to clean up single entries in the `LocalSettings` map.

Code after:
	}

	return nil
}

func SetGlobalOptionNative(option string, nativeValue interface{}) error {
	if err := config.OptionIsValid(option, nativeValue); err != nil {
		return err
	}

	// check for local option first...
	for _, s := range config.LocalSettings {
		if s == option {
			return MainTab().CurPane().Buf.SetOptionNative(option, nativeValue)
		}
	}

	// ...if it's not local continue with the globals...
	if err := doSetGlobalOptionNative(option, nativeValue); err != nil {
		return err
	}

	// ...at last check the buffer locals
	for _, b := range buffer.OpenBuffers {
		b.DoSetOptionNative(option, nativeValue)
		delete(b.LocalSettings, option)
	}

	return config.WriteSettings(filepath.Join(config.ConfigDir, "settings.json"))
}

func SetGlobalOption(option, value string) error {
	if _, ok := config.GlobalSettings[option]; !ok {
		return config.ErrInvalidOption
	}

	nativeValue, err := config.GetNativeValue(option, config.GlobalSettings[option], value)
	if err != nil {
		return err
	}

	return SetGlobalOptionNative(option, nativeValue)
}

// ResetCmd resets a setting to its default value
func (h *BufPane) ResetCmd(args []string) {
	if len(args) < 1 {
		InfoBar.Error("Not enough arguments")
		return
	}
Repository: zyedidia/micro
File: cmd/micro/micro.go (Go)

Code before:
			log.Fatal("error starting CPU profile: ", err)
		}
		defer pprof.StopCPUProfile()
	}

	InitLog()

	err = config.InitConfigDir(*flagConfigDir)
	if err != nil {
		screen.TermMessage(err)
	}

	config.InitRuntimeFiles(true)
	config.InitPlugins()

	err = config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	}

	settings := config.ParsedSettings()
	for k, v := range settings {
		if err = config.OptionIsValid(k, v); err != nil {
			screen.TermMessage(err)
		}
	}

	err = config.InitGlobalSettings()
	if err != nil {
		screen.TermMessage(err)
	}

	// flag options
	for k, v := range optionFlags {
		if *v != "" {
			nativeValue, err := config.GetNativeValue(k, config.DefaultAllSettings()[k], *v)
			if err != nil {
				screen.TermMessage(err)
				continue
			}
			config.GlobalSettings[k] = nativeValue
			config.VolatileSettings[k] = true
		}
	}

	DoPluginFlags()

	err = screen.Init()
	if err != nil {
		fmt.Println(err)
		fmt.Println("Fatal: Micro could not initialize a Screen.")

Review comment (suggestion):
Looks ugly. Can't we just validate options directly in `config.ReadSettings()`? Especially since it looks like we are already partially doing it there (in `verifySetting()`).

Code after:
		}
		defer pprof.StopCPUProfile()
	}

	InitLog()

	err = config.InitConfigDir(*flagConfigDir)
	if err != nil {
		screen.TermMessage(err)
	}

	config.InitRuntimeFiles(true)
	config.InitPlugins()

	err = config.ReadSettings()
	if err != nil {
		screen.TermMessage(err)
	}
	err = config.InitGlobalSettings()
	if err != nil {
		screen.TermMessage(err)
	}

	// flag options
	for k, v := range optionFlags {
		if *v != "" {
			nativeValue, err := config.GetNativeValue(k, config.DefaultAllSettings()[k], *v)
			if err != nil {
				screen.TermMessage(err)
				continue
			}
			if err = config.OptionIsValid(k, nativeValue); err != nil {
				screen.TermMessage(err)
				continue
			}
			config.GlobalSettings[k] = nativeValue
			config.VolatileSettings[k] = true
		}
	}

	DoPluginFlags()

	err = screen.Init()
	if err != nil {
		fmt.Println(err)
		fmt.Println("Fatal: Micro could not initialize a Screen.")
		os.Exit(1)
	}
	m := clipboard.SetMethod(config.GetGlobalOption("clipboard").(string))
	clipErr := clipboard.Initialize(m)