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

Code before:
package buffer

import (
	"github.com/zyedidia/micro/v2/internal/config"
	"github.com/zyedidia/micro/v2/internal/screen"
)

func (b *Buffer) SetOptionNative(option string, nativeValue interface{}) error {
	b.Settings[option] = nativeValue

	if option == "fastdirty" {
		if !nativeValue.(bool) {
			e := calcHash(b, &b.origHash)
			if e == ErrFileTooLarge {
				b.Settings["fastdirty"] = true
			}
		}
	} else if option == "statusline" {
		screen.Redraw()
	} else if option == "filetype" {
		settings := config.ParsedSettings()
		settings["filetype"] = nativeValue
		config.InitLocalSettings(settings, b.Path)
		for k, v := range config.DefaultCommonSettings() {
			if k == "filetype" {
				continue
			}
			if _, ok := config.VolatileSettings[k]; ok {
				// filetype should not override volatile settings
				continue
			}
			if _, ok := settings[k]; ok {
				b.SetOptionNative(k, settings[k])
			} else {
				b.SetOptionNative(k, v)
			}
		}
		b.UpdateRules()

Review comment (suggestion):
Ehh... this causes a regression: modify the buffer (without saving it) and run `reload` -> statusbar now shows that the buffer is not modified (while it is) and Ctrl-Q closes the buffer without asking to save changes.

That's because we update `b.origHash` here, so `b.Modified()` now returns true. So now we know what was the `!b.Modified()` check for...

So for the case when the buffer is modified, we need to find out the buffer size in some other way.

Code after:
package buffer

import (
	"crypto/md5"
	"reflect"

	"github.com/zyedidia/micro/v2/internal/config"
	"github.com/zyedidia/micro/v2/internal/screen"
)

func (b *Buffer) ReloadSettings(reloadFiletype bool) {
	settings := config.ParsedSettings()

	if _, ok := b.LocalSettings["filetype"]; !ok && reloadFiletype {
		// need to update filetype before updating other settings based on it
		b.Settings["filetype"] = "unknown"
		if v, ok := settings["filetype"]; ok {
			b.Settings["filetype"] = v
		}
	}

	// update syntax rules, which will also update filetype if needed
	b.UpdateRules()
	settings["filetype"] = b.Settings["filetype"]

	config.InitLocalSettings(settings, b.Path)
	for k, v := range config.DefaultCommonSettings() {
		if k == "filetype" {
			// prevent recursion
			continue
		}
		if _, ok := config.VolatileSettings[k]; ok {
			// reload should not override volatile settings
			continue
		}
		if _, ok := b.LocalSettings[k]; ok {
			// reload should not override local settings
			continue
Repository: zyedidia/micro
File: internal/buffer/settings.go (Go)

Code before:
package buffer

import (
	"crypto/md5"

	"github.com/zyedidia/micro/v2/internal/config"
	"github.com/zyedidia/micro/v2/internal/screen"
)

func (b *Buffer) SetOptionNative(option string, nativeValue interface{}) error {
	b.Settings[option] = nativeValue

	if option == "fastdirty" {
		if !nativeValue.(bool) {
			var hash [md5.Size]byte
			e := calcHash(b, &hash)
			if e == ErrFileTooLarge {
				b.Settings["fastdirty"] = true
			}
		}
	} else if option == "statusline" {
		screen.Redraw()
	} else if option == "filetype" {
		settings := config.ParsedSettings()
		settings["filetype"] = nativeValue
		config.InitLocalSettings(settings, b.Path)
		for k, v := range config.DefaultCommonSettings() {
			if k == "filetype" {
				continue
			}
			if _, ok := config.VolatileSettings[k]; ok {
				// filetype should not override volatile settings
				continue
			}
			if _, ok := settings[k]; ok {
				b.SetOptionNative(k, settings[k])
			} else {
				b.SetOptionNative(k, v)
			}
		}
		b.UpdateRules()

Review comment (bug):
So we calculate the hash while we don't need to calculate it at all (we don't use the calculated `hash` value at all), we just need to find out the file size?

Actually it looks like we do need to calculate `b.origHash` here (but only if `!b.Modified()`), since if `fastdirty` was previously enabled, `b.origHash` was not previously calculated.

...And looks like we also have yet another bug here: `b.origHash` may contain some stale value, which was calculated at some point in the past when `fastdirty` was enabled (before it got disabled), so here, in the case if the buffer is modified, we don't update `b.origHash`, so we will erroneously use this old outdated value. So it looks like we should invalidate `b.origHash` (e.g. set it to zero?) in this case.

Code after:
package buffer

import (
	"crypto/md5"
	"reflect"

	"github.com/zyedidia/micro/v2/internal/config"
	"github.com/zyedidia/micro/v2/internal/screen"
)

func (b *Buffer) ReloadSettings(reloadFiletype bool) {
	settings := config.ParsedSettings()

	if _, ok := b.LocalSettings["filetype"]; !ok && reloadFiletype {
		// need to update filetype before updating other settings based on it
		b.Settings["filetype"] = "unknown"
		if v, ok := settings["filetype"]; ok {
			b.Settings["filetype"] = v
		}
	}

	// update syntax rules, which will also update filetype if needed
	b.UpdateRules()
	settings["filetype"] = b.Settings["filetype"]

	config.InitLocalSettings(settings, b.Path)
	for k, v := range config.DefaultCommonSettings() {
		if k == "filetype" {
			// prevent recursion
			continue
		}
		if _, ok := config.VolatileSettings[k]; ok {
			// reload should not override volatile settings
			continue
		}
		if _, ok := b.LocalSettings[k]; ok {
			// reload should not override local settings
			continue
		}
		if _, ok := settings[k]; ok {
			b.DoSetOptionNative(k, settings[k])
Repository: zyedidia/micro
File: internal/buffer/buffer.go (Go)

Code before:
				// for empty files, use the fileformat setting instead of
				// autodetection
				switch settings["fileformat"] {
				case "unix":
					ff = FFUnix
				case "dos":
					ff = FFDos
				}
			}

			b.LineArray = NewLineArray(uint64(size), ff, reader)
		}
		b.EventHandler = NewEventHandler(b.SharedBuffer, b.cursors)

		// The last time this file was modified
		b.UpdateModTime()
	}

	if b.Settings["readonly"].(bool) && b.Type == BTDefault {
		b.Type.Readonly = true
	}

	switch b.Endings {
	case FFUnix:
		b.Settings["fileformat"] = "unix"
		b.LocalSettings["fileformat"] = true
	case FFDos:
		b.Settings["fileformat"] = "dos"
		b.LocalSettings["fileformat"] = true
	}

	b.UpdateRules()
	// init local settings again now that we know the filetype
	config.InitLocalSettings(b.Settings, b.Path)

	if _, err := os.Stat(filepath.Join(config.ConfigDir, "buffers")); os.IsNotExist(err) {
		os.Mkdir(filepath.Join(config.ConfigDir, "buffers"), os.ModePerm)
	}

	if startcursor.X != -1 && startcursor.Y != -1 {
		b.StartCursor = startcursor
	} else if b.Settings["savecursor"].(bool) || b.Settings["saveundo"].(bool) {
		err := b.Unserialize()
		if err != nil {
			screen.TermMessage(err)
		}
	}

	b.AddCursor(NewCursor(b, b.StartCursor))
	b.GetActiveCursor().Relocate()


Review comment (suggestion):
It gets added to `LocalSettings` even if it is set to default, not autodetected (i.e. if the file is empty).

Code after:
					ff = FFUnix
				case "dos":
					ff = FFDos
				}
			} else {
				// in case of autodetection treat as locally set
				b.LocalSettings["fileformat"] = true
			}

			b.LineArray = NewLineArray(uint64(size), ff, reader)
		}
		b.EventHandler = NewEventHandler(b.SharedBuffer, b.cursors)

		// The last time this file was modified
		b.UpdateModTime()
	}

	if b.Settings["readonly"].(bool) && b.Type == BTDefault {
		b.Type.Readonly = true
	}

	switch b.Endings {
	case FFUnix:
		b.Settings["fileformat"] = "unix"
	case FFDos:
		b.Settings["fileformat"] = "dos"
	}

	b.UpdateRules()
	// init local settings again now that we know the filetype
	config.InitLocalSettings(b.Settings, b.Path)

	if _, err := os.Stat(filepath.Join(config.ConfigDir, "buffers")); os.IsNotExist(err) {
		os.Mkdir(filepath.Join(config.ConfigDir, "buffers"), os.ModePerm)
	}

	if startcursor.X != -1 && startcursor.Y != -1 {
		b.StartCursor = startcursor
	} else if b.Settings["savecursor"].(bool) || b.Settings["saveundo"].(bool) {
		err := b.Unserialize()
		if err != nil {
			screen.TermMessage(err)
		}
	}

	b.AddCursor(NewCursor(b, b.StartCursor))
	b.GetActiveCursor().Relocate()

	if !b.Settings["fastdirty"].(bool) && !found {
		if size > LargeFileThreshold {
			// If the file is larger than LargeFileThreshold fastdirty needs to be on
Repository: zyedidia/micro
File: internal/buffer/settings.go (Go)

Code before:
package buffer

import (
	"crypto/md5"
	"reflect"

	"github.com/zyedidia/micro/v2/internal/config"
	"github.com/zyedidia/micro/v2/internal/screen"
)

func (b *Buffer) ReloadSettings(filetype bool) {
	settings := config.ParsedSettings()

	if _, ok := b.LocalSettings["filetype"]; ok || filetype {
		settings["filetype"] = b.Settings["filetype"]
	} else if _, ok := settings["filetype"]; ok && settings["filetype"] != "unknown" {
		b.Settings["filetype"] = settings["filetype"]
	} else {
		b.Settings["filetype"] = "unknown"
	}
	if b.Settings["filetype"] == "unknown" {
		b.UpdateRules()
		settings["filetype"] = b.Settings["filetype"]
	}

	config.InitLocalSettings(settings, b.Path)
	for k, v := range config.DefaultCommonSettings() {
		if k == "filetype" {
			// prevent recursion
			continue
		}
		if _, ok := config.VolatileSettings[k]; ok {
			// reload should not override volatile settings
			continue
		}
		if _, ok := b.LocalSettings[k]; ok {
			// reload should not override local settings
			continue
		}
		if _, ok := settings[k]; ok {
			b.DoSetOptionNative(k, settings[k])
		} else {
			b.DoSetOptionNative(k, v)
		}
	}
}


Review comment (suggestion):
We also update rules (unconditionally) in `DoSetOptionNative("filetype")` and in `reload` (and we indeed should do that, especially in `reload`, since it may reload syntax files). So we unnecessarily update them twice?

So maybe we should instead update rules just here, in `ReloadSettings()`, unconditionally, but before updating `settings["filetype"]`?

i.e. something like:

```
	if _, ok := b.LocalSettings["filetype"]; !ok && !filetype {
		// need to update filetype before updating other settings based on it
		b.Settings["filetype"] = "unknown"
		if v, ok := settings["filetype"]; ok {
			b.Settings["filetype"] = v
		}
	}

	// update syntax rules, which will also update filetype if needed
	b.UpdateRules()
	settings["filetype"] = b.Settings["filetype"]
```

Code after:
package buffer

import (
	"crypto/md5"
	"reflect"

	"github.com/zyedidia/micro/v2/internal/config"
	"github.com/zyedidia/micro/v2/internal/screen"
)

func (b *Buffer) ReloadSettings(reloadFiletype bool) {
	settings := config.ParsedSettings()

	if _, ok := b.LocalSettings["filetype"]; !ok && reloadFiletype {
		// need to update filetype before updating other settings based on it
		b.Settings["filetype"] = "unknown"
		if v, ok := settings["filetype"]; ok {
			b.Settings["filetype"] = v
		}
	}

	// update syntax rules, which will also update filetype if needed
	b.UpdateRules()
	settings["filetype"] = b.Settings["filetype"]

	config.InitLocalSettings(settings, b.Path)
	for k, v := range config.DefaultCommonSettings() {
		if k == "filetype" {
			// prevent recursion
			continue
		}
		if _, ok := config.VolatileSettings[k]; ok {
			// reload should not override volatile settings
			continue
		}
		if _, ok := b.LocalSettings[k]; ok {
			// reload should not override local settings
			continue
		}
		if _, ok := settings[k]; ok {
			b.DoSetOptionNative(k, settings[k])
		} else {
			b.DoSetOptionNative(k, v)
		}
	}
}
Repository: zyedidia/micro
File: internal/buffer/settings.go (Go)

Code before:
package buffer

import (
	"crypto/md5"

	"github.com/zyedidia/micro/v2/internal/config"
	"github.com/zyedidia/micro/v2/internal/screen"
)

func (b *Buffer) SetOptionNative(option string, nativeValue interface{}) error {
	b.Settings[option] = nativeValue

	if option == "fastdirty" {
		if !nativeValue.(bool) {
			if !b.Modified() {
				e := calcHash(b, &b.origHash)
				if e == ErrFileTooLarge {
					b.Settings["fastdirty"] = true
				}
			} else {
				b.origHash = [md5.Size]byte{}
				if b.Size() > LargeFileThreshold {
					b.Settings["fastdirty"] = true
				}
			}
		}
	} else if option == "statusline" {
		screen.Redraw()
	} else if option == "filetype" {
		settings := config.ParsedSettings()
		settings["filetype"] = nativeValue
		config.InitLocalSettings(settings, b.Path)
		for k, v := range config.DefaultCommonSettings() {
			if k == "filetype" {
				continue
			}
			if _, ok := config.VolatileSettings[k]; ok {
				// filetype should not override volatile settings
				continue
			}
			if _, ok := settings[k]; ok {
				b.SetOptionNative(k, settings[k])
			} else {
				b.SetOptionNative(k, v)
			}
		}
		b.UpdateRules()

Review comment (performance):
Just FYI this made me reveal a bunch of other small issues around `calcHash()`: #3430

...And just now I've also realized that this checking of file size inside `calcHash()` itself doesn't seem to make much sense, actually. It doesn't really optimize anything, since just calculating the file size (i.e. adding up line lengths) is probably faster than md5 calculation in any case (even if we optimized `calcHash()` to break the loop once `size` exceeds `LargeFileThreshold`).

So it seems it would be both simpler, cleaner and faster to let `calcHash()` just unconditionally calculate the hash of the given buffer, and explicitly check its size before calling `calcHash()` in all cases when size checking is needed.

Code after:
package buffer

import (
	"crypto/md5"
	"reflect"

	"github.com/zyedidia/micro/v2/internal/config"
	"github.com/zyedidia/micro/v2/internal/screen"
)

func (b *Buffer) ReloadSettings(reloadFiletype bool) {
	settings := config.ParsedSettings()

	if _, ok := b.LocalSettings["filetype"]; !ok && reloadFiletype {
		// need to update filetype before updating other settings based on it
		b.Settings["filetype"] = "unknown"
		if v, ok := settings["filetype"]; ok {
			b.Settings["filetype"] = v
		}
	}

	// update syntax rules, which will also update filetype if needed
	b.UpdateRules()
	settings["filetype"] = b.Settings["filetype"]

	config.InitLocalSettings(settings, b.Path)
	for k, v := range config.DefaultCommonSettings() {
		if k == "filetype" {
			// prevent recursion
			continue
		}
		if _, ok := config.VolatileSettings[k]; ok {
			// reload should not override volatile settings
			continue
		}
		if _, ok := b.LocalSettings[k]; ok {
			// reload should not override local settings
			continue
		}
		if _, ok := settings[k]; ok {
			b.DoSetOptionNative(k, settings[k])
		} else {
			b.DoSetOptionNative(k, v)
		}
	}
}
Repository: zyedidia/micro
File: internal/action/command.go (Go)

Code before:
}

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
	}

	option := args[0]
	value := args[1]

	err := SetGlobalOption(option, value)
	if err == config.ErrInvalidOption {
		err := h.Buf.SetOption(option, value)
		if err != nil {
			InfoBar.Error(err)
		}
	} else if err != nil {
		InfoBar.Error(err)
	}

Review comment (suggestion):
Still not checked for errors.
`reset ruler aaa` -> silently does nothing.

Code after:

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
	}

	option := args[0]
	value := args[1]

	err := SetGlobalOption(option, value)
	if err == config.ErrInvalidOption {
		err := h.Buf.SetOption(option, value)
		if err != nil {
			InfoBar.Error(err)
		}
	} else if err != nil {
		InfoBar.Error(err)
	}
}
Repository: zyedidia/micro
File: internal/shell/shell.go (Go)

Code before:
	inputCmd := args[0]

	// Shut down the screen because we're going to interact directly with the shell
	screenb := screen.TempFini()

	args = args[1:]

	// Set up everything for the command
	outputBytes := &bytes.Buffer{}
	cmd := exec.Command(inputCmd, args...)
	cmd.Stdin = os.Stdin
	if getOutput {
		cmd.Stdout = io.MultiWriter(os.Stdout, outputBytes)
	} else {
		cmd.Stdout = os.Stdout
	}
	cmd.Stderr = os.Stderr

	// This is a trap for Ctrl-C so that it doesn't kill micro
	// micro is killed if the signal is ignored only on Windows, so it is
	// received
	c := make(chan os.Signal, 1)
	signal.Reset(os.Interrupt)
	signal.Notify(c, os.Interrupt)
	err = cmd.Start()
	if err == nil {
		err = cmd.Wait()
	}

	output := outputBytes.String()

	if wait {
		// This is just so we don't return right away and let the user press enter to return
		screen.TermMessage("")
	}

	// Start the screen back up
	screen.TempStart(screenb)

	signal.Notify(util.Sigterm, os.Interrupt)
	signal.Stop(c)

	return output, err
}

// UserCommand runs the shell command
// The openTerm argument specifies whether a terminal should be opened (for viewing output
// or interacting with stdin)
// func UserCommand(input string, openTerm bool, waitToFinish bool) string {
// 	if !openTerm {
// 		RunBackgroundShell(input)

Review comment (bug):
Print error if it failed?

I've just tried this, seems to work ok (e.g. when trying to run `RunInteractiveShell()` with a non-existing command):
```
--- a/internal/shell/shell.go
+++ b/internal/shell/shell.go
@@ -104,6 +104,8 @@ func RunInteractiveShell(input string, wait bool, getOutput bool) (string, error
 	err = cmd.Start()
 	if err == nil {
 		err = cmd.Wait()
+	} else {
+		screen.TermMessage(err)
 	}
 
 	output := outputBytes.String()
```

Code after:
	inputCmd := args[0]

	// Shut down the screen because we're going to interact directly with the shell
	screenb := screen.TempFini()

	args = args[1:]

	// Set up everything for the command
	outputBytes := &bytes.Buffer{}
	cmd := exec.Command(inputCmd, args...)
	cmd.Stdin = os.Stdin
	if getOutput {
		cmd.Stdout = io.MultiWriter(os.Stdout, outputBytes)
	} else {
		cmd.Stdout = os.Stdout
	}
	cmd.Stderr = os.Stderr

	// This is a trap for Ctrl-C so that it doesn't kill micro
	// micro is killed if the signal is ignored only on Windows, so it is
	// received
	c := make(chan os.Signal, 1)
	signal.Reset(os.Interrupt)
	signal.Notify(c, os.Interrupt)
	err = cmd.Start()
	if err == nil {
		err = cmd.Wait()
		if wait {
			// This is just so we don't return right away and let the user press enter to return
			screen.TermMessage("")
		}
	} else {
		screen.TermMessage(err)
	}

	output := outputBytes.String()

	// Start the screen back up
	screen.TempStart(screenb)

	signal.Notify(util.Sigterm, os.Interrupt)
	signal.Stop(c)

	return output, err
}

// UserCommand runs the shell command
// The openTerm argument specifies whether a terminal should be opened (for viewing output
// or interacting with stdin)
// func UserCommand(input string, openTerm bool, waitToFinish bool) string {
// 	if !openTerm {
Repository: zyedidia/micro
File: internal/action/actions.go (Go)

Code before:
	h.HSplitBuf(buffer.NewBufferFromString("", "", buffer.BTDefault))

	return true
}

// Unsplit closes all splits in the current tab except the active one
func (h *BufPane) Unsplit() bool {
	tab := h.tab
	n := tab.GetNode(h.splitID)
	ok := n.Unsplit()
	if ok {
		tab.RemovePane(tab.GetPane(h.splitID))
		tab.Resize()
		tab.SetActive(len(tab.Panes) - 1)

		return true
	}
	return false
}

// NextSplit changes the view to the next split
func (h *BufPane) NextSplit() bool {
	if len(h.tab.Panes) == 1 {
		return false
	}
	lastTabIndex := len(h.tab.Panes) - 1
	if h.tab.active == lastTabIndex {
		return false
	}

	a := h.tab.active
	if a < len(h.tab.Panes)-1 {
		a++
	} else {
		a = 0
	}

	h.tab.SetActive(a)

	return true
}

// PreviousSplit changes the view to the previous split
func (h *BufPane) PreviousSplit() bool {
	if len(h.tab.Panes) == 1 {
		return false
	}
	if h.tab.active == 0 {
		return false
	}


Review comment (suggestion):
`lastSplitIndex`? `lastPaneIdx`?

...And actually I don't find this variable (here and in other functions) very useful. `if h.tab.active == len(h.tab.Panes)-1` would be fine. But that's a nit.

Code after:
}

// NextSplit changes the view to the next split
func (h *BufPane) NextSplit() bool {
	if h.tab.active == len(h.tab.Panes)-1 {
		return false
	}
	h.tab.SetActive(h.tab.active + 1)
	return true
}

// PreviousSplit changes the view to the previous split
func (h *BufPane) PreviousSplit() bool {
	if h.tab.active == 0 {
		return false
	}
	h.tab.SetActive(h.tab.active - 1)
	return true
}

// FirstSplit changes the view to the first split
func (h *BufPane) FirstSplit() bool {
	if h.tab.active == 0 {
		return false
	}
	h.tab.SetActive(0)
	return true
}

// LastSplit changes the view to the last split
func (h *BufPane) LastSplit() bool {
	lastPaneIdx := len(h.tab.Panes) - 1
	if h.tab.active == lastPaneIdx {
		return false
	}
	h.tab.SetActive(lastPaneIdx)
	return true
}

var curmacro []interface{}
var recordingMacro bool

// ToggleMacro toggles recording of a macro
func (h *BufPane) ToggleMacro() bool {
	recordingMacro = !recordingMacro
	if recordingMacro {
		curmacro = []interface{}{}
		InfoBar.Message("Recording")
	} else {
		InfoBar.Message("Stopped recording")
	}