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

Code before:
	}
	if curVal == nil {
		return config.ErrInvalidOption
	}

	if choices, ok := config.OptionChoices[option]; ok && len(choices) == 2 {
		if curVal == choices[0] {
			newVal = choices[1]
		} else {
			newVal = choices[0]
		}
	} else if curValBool, ok := curVal.(bool); ok {
		newVal = !curValBool
	} else {
		return config.ErrOptNotToggleable
	}

	if !local {
		if err := SetGlobalOptionNative(option, newVal); err == nil {
			return nil
		}
	}

	if err := h.Buf.SetOptionNative(option, newVal); err != nil {
		return err
	}
	return nil
}

// ToggleCmd toggles a toggleable option
func (h *BufPane) ToggleCmd(args []string) {
	if len(args) < 1 {
		InfoBar.Error("Not enough arguments: provide a toggleable option")
		return
	}
	if err := h.toggleOption(args[0], false); err != nil {
		InfoBar.Error(err)
	}
}

// ToggleCmd toggles a toggleable option local to the buffer
func (h *BufPane) ToggleLocalCmd(args []string) {
	if len(args) < 1 {
		InfoBar.Error("Not enough arguments: provide a toggleable option")
		return
	}
	if err := h.toggleOption(args[0], true); err != nil {
		InfoBar.Error(err)
	}
}


Review comment (suggestion):
I took this logic from `SetCmd()`.

```go
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)
	}
```

But now I wonder if making this fallback makes sense at all?

SetGlobalOption is just a SetGlobalOptionNative wrapper.
SetGlobalOptionNative have a fallback for local settings itself. https://github.com/zyedidia/micro/blob/3a7705a0909e74fd50554cbdc946767a7dd7c801/internal/action/command.go#L636-L641

Maybe I am missing something but I don't see a way where config.ErrInvalidOption is returned and makes sense to call `Buf.SetOption*()`

Code after:
	}
	if curVal == nil {
		return config.ErrInvalidOption
	}

	if choices, ok := config.OptionChoices[option]; ok && len(choices) == 2 {
		if curVal == choices[0] {
			newVal = choices[1]
		} else {
			newVal = choices[0]
		}
	} else if curValBool, ok := curVal.(bool); ok {
		newVal = !curValBool
	} else {
		return config.ErrOptNotToggleable
	}

	if local {
		if err := h.Buf.SetOptionNative(option, newVal); err != nil {
			return err
		}
	} else {
		if err := SetGlobalOptionNative(option, newVal); err != nil {
			return err
		}
	}

	return nil
}

// ToggleCmd toggles a toggleable option
func (h *BufPane) ToggleCmd(args []string) {
	if len(args) < 1 {
		InfoBar.Error("Not enough arguments: provide a toggleable option")
		return
	}
	if err := h.toggleOption(args[0], false); err != nil {
		InfoBar.Error(err)
	}
}

// ToggleCmd toggles a toggleable option local to the buffer
func (h *BufPane) ToggleLocalCmd(args []string) {
	if len(args) < 1 {
		InfoBar.Error("Not enough arguments: provide a toggleable option")
		return
	}
	if err := h.toggleOption(args[0], true); err != nil {
		InfoBar.Error(err)
	}
}
Repository: zyedidia/micro
File: internal/buffer/save.go (Go)

Code before:
		signal.Notify(sigChan, os.Interrupt)

		screenb = screen.TempFini()
		// need to start the process now, otherwise when we flush the file
		// contents to its stdin it might hang because the kernel's pipe size
		// is too small to handle the full file contents all at once
		err = cmd.Start()
		if err != nil {
			screen.TempStart(screenb)

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

			return wrappedFile{}, err
		}
	} else {
		writeCloser, err = os.OpenFile(name, os.O_WRONLY|os.O_CREATE, util.FileMode)
		if err != nil {
			return wrappedFile{}, err
		}
	}

	return wrappedFile{name, writeCloser, withSudo, screenb, cmd, sigChan}, nil
}

func (wf wrappedFile) Truncate(size int64) error {
	if wf.withSudo {
		// we don't need to stop the screen here, since it is still stopped
		// by openFile()
		cmd := exec.Command(config.GlobalSettings["sucmd"].(string), "dd", "count=0", "if=/dev/zero", "of="+wf.name)
		return cmd.Run()
	}
	return wf.writeCloser.(*os.File).Truncate(size)
}

func (wf wrappedFile) Write(b *Buffer) (int, error) {
	file := bufio.NewWriter(transform.NewWriter(wf.writeCloser, b.encoding.NewEncoder()))

	b.Lock()
	defer b.Unlock()

	if len(b.lines) == 0 {
		return 0, nil
	}

	// end of line
	var eol []byte
	if b.Endings == FFDos {
		eol = []byte{'\r', '\n'}
	} else {
		eol = []byte{'\n'}

Review comment (suggestion):
The `size` argument not used anymore? (It was not really needed in the first place, but now it is not even used.)

Code after:
		if err != nil {
			return wrappedFile{}, err
		}

		sigChan = make(chan os.Signal, 1)
		signal.Reset(os.Interrupt)
		signal.Notify(sigChan, os.Interrupt)

		screenb = screen.TempFini()
		// need to start the process now, otherwise when we flush the file
		// contents to its stdin it might hang because the kernel's pipe size
		// is too small to handle the full file contents all at once
		err = cmd.Start()
		if err != nil {
			screen.TempStart(screenb)

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

			return wrappedFile{}, err
		}
	} else {
		writeCloser, err = os.OpenFile(name, os.O_WRONLY|os.O_CREATE, util.FileMode)
		if err != nil {
			return wrappedFile{}, err
		}
	}

	return wrappedFile{name, writeCloser, withSudo, screenb, cmd, sigChan}, nil
}

func (wf wrappedFile) Truncate() error {
	if wf.withSudo {
		// we don't need to stop the screen here, since it is still stopped
		// by openFile()
		// truncate might not be available on every platfom, so use dd instead
		cmd := exec.Command(config.GlobalSettings["sucmd"].(string), "dd", "count=0", "of="+wf.name)
		return cmd.Run()
	}
	return wf.writeCloser.(*os.File).Truncate(0)
}

func (wf wrappedFile) Write(b *Buffer) (int, error) {
	file := bufio.NewWriter(transform.NewWriter(wf.writeCloser, b.encoding.NewEncoder()))

	b.Lock()
	defer b.Unlock()

	if len(b.lines) == 0 {
		return 0, nil
	}
Repository: zyedidia/micro
File: internal/buffer/save.go (Go)

Code before:
		// contents to its stdin it might hang because the kernel's pipe size
		// is too small to handle the full file contents all at once
		err = cmd.Start()
		if err != nil {
			screen.TempStart(screenb)

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

			return wrappedFile{}, err
		}
	} else {
		writeCloser, err = os.OpenFile(name, os.O_WRONLY|os.O_CREATE, util.FileMode)
		if err != nil {
			return wrappedFile{}, err
		}
	}

	return wrappedFile{name, writeCloser, withSudo, screenb, cmd, sigChan}, nil
}

func (wf wrappedFile) Truncate(size int64) error {
	if wf.withSudo {
		// we don't need to stop the screen here, since it is still stopped
		// by openFile()
		cmd := exec.Command(config.GlobalSettings["sucmd"].(string), "dd", "count=0", "if=/dev/zero", "of="+wf.name)
		return cmd.Run()
	}
	return wf.writeCloser.(*os.File).Truncate(size)
}

func (wf wrappedFile) Write(b *Buffer) (int, error) {
	file := bufio.NewWriter(transform.NewWriter(wf.writeCloser, b.encoding.NewEncoder()))

	b.Lock()
	defer b.Unlock()

	if len(b.lines) == 0 {
		return 0, nil
	}

	// end of line
	var eol []byte
	if b.Endings == FFDos {
		eol = []byte{'\r', '\n'}
	} else {
		eol = []byte{'\n'}
	}

	err := wf.Truncate(0)
	if err != nil {

Review comment (suggestion):
We can drop `if=/dev/zero` (thus avoid unneeded assumption about the presence of /dev/zero)? It doesn't matter where we "read" 0 bytes from, so we can just read them from stdin.

Code after:
		sigChan = make(chan os.Signal, 1)
		signal.Reset(os.Interrupt)
		signal.Notify(sigChan, os.Interrupt)

		screenb = screen.TempFini()
		// need to start the process now, otherwise when we flush the file
		// contents to its stdin it might hang because the kernel's pipe size
		// is too small to handle the full file contents all at once
		err = cmd.Start()
		if err != nil {
			screen.TempStart(screenb)

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

			return wrappedFile{}, err
		}
	} else {
		writeCloser, err = os.OpenFile(name, os.O_WRONLY|os.O_CREATE, util.FileMode)
		if err != nil {
			return wrappedFile{}, err
		}
	}

	return wrappedFile{name, writeCloser, withSudo, screenb, cmd, sigChan}, nil
}

func (wf wrappedFile) Truncate() error {
	if wf.withSudo {
		// we don't need to stop the screen here, since it is still stopped
		// by openFile()
		// truncate might not be available on every platfom, so use dd instead
		cmd := exec.Command(config.GlobalSettings["sucmd"].(string), "dd", "count=0", "of="+wf.name)
		return cmd.Run()
	}
	return wf.writeCloser.(*os.File).Truncate(0)
}

func (wf wrappedFile) Write(b *Buffer) (int, error) {
	file := bufio.NewWriter(transform.NewWriter(wf.writeCloser, b.encoding.NewEncoder()))

	b.Lock()
	defer b.Unlock()

	if len(b.lines) == 0 {
		return 0, nil
	}

	// end of line
	var eol []byte
	if b.Endings == FFDos {
Repository: zyedidia/micro
File: internal/buffer/search.go (Go)

Code before:
// FindNext finds the next occurrence of a given string in the buffer
// It returns the start and end location of the match (if found) and
// a boolean indicating if it was found
// May also return an error if the search regex is invalid
func (b *Buffer) FindNext(s string, start, end, from Loc, down bool, useRegex bool) ([2]Loc, bool, error) {
	if s == "" {
		return [2]Loc{}, false, nil
	}

	var r *regexp.Regexp
	var err error

	if !useRegex {
		s = regexp.QuoteMeta(s)
	}

	if b.Settings["ignorecase"].(bool) {
		r, err = regexp.Compile("(?i)" + s)
	} else {
		r, err = regexp.Compile(s)
	}

	if err == nil {
		_, err = regexp.Compile("(" + s + ")")
	}

	if err != nil {
		return [2]Loc{}, false, err
	}

	var found bool
	var l [2]Loc
	if down {
		l, found = b.findDown(r, from, end)
		if !found {
			l, found = b.findDown(r, start, end)
		}
	} else {
		l, found = b.findUp(r, from, start)
		if !found {
			l, found = b.findUp(r, end, start)
		}
	}
	return l, found, nil
}

// ReplaceRegex replaces all occurrences of 'search' with 'replace' in the given area
// and returns the number of replacements made and the number of characters
// added or removed on the last line of the range
func (b *Buffer) ReplaceRegex(start, end Loc, search *regexp.Regexp, replace []byte, captureGroups bool) (int, int) {
	if start.GreaterThan(end) {

Review comment (refactor):
I like how it is proposed to simplify in #3913:
```go
	if b.Settings["ignorecase"].(bool) {
		s = "(?i)" + s
	}
```

Afterwards we can perform your introduced check with:
```go
	r, err = regexp.Compile("(" + s + ")")
	if err != nil {
		return [2]Loc{}, false, err
	}
```

The benefit is, that we should compile the string only once to find out if it is valid or not.
Would it hurt to have it in that moment already in an additional capture group? If no, then nothing more needs to be added, if yes then the non-grouped compilation (+ additional sanity check for `err`):

```go
	r, err = regexp.Compile(s)
```

Code after:
		} else if match[1] != end {
			loc = match[1].Move(1, b)
		} else {
			break
		}
	}
	return matches
}

// FindNext finds the next occurrence of a given string in the buffer
// It returns the start and end location of the match (if found) and
// a boolean indicating if it was found
// May also return an error if the search regex is invalid
func (b *Buffer) FindNext(s string, start, end, from Loc, down bool, useRegex bool) ([2]Loc, bool, error) {
	if s == "" {
		return [2]Loc{}, false, nil
	}

	var r *regexp.Regexp
	var err error

	if !useRegex {
		s = regexp.QuoteMeta(s)
	}

	if b.Settings["ignorecase"].(bool) {
		r, err = regexp.Compile("(?i)" + s)
	} else {
		r, err = regexp.Compile(s)
	}

	if err != nil {
		return [2]Loc{}, false, err
	}

	var found bool
	var l [2]Loc
	if down {
		l, found = b.findDown(r, from, end)
		if !found {
			l, found = b.findDown(r, start, end)
		}
	} else {
		l, found = b.findUp(r, from, start)
		if !found {
			l, found = b.findUp(r, end, start)
		}
	}
	return l, found, nil
}
Repository: zyedidia/micro
File: internal/buffer/search.go (Go)

Code before:

func findLineParams(b *Buffer, start, end Loc, i int, r *regexp.Regexp) ([]byte, int, int, *regexp.Regexp) {
	l := b.LineBytes(i)
	charpos := 0
	padMode := 0

	if i == end.Y {
		nchars := util.CharacterCount(l)
		end.X = util.Clamp(end.X, 0, nchars)
		if end.X < nchars {
			l = util.SliceStart(l, end.X+1)
			padMode |= padEnd
		}
	}

	if i == start.Y {
		nchars := util.CharacterCount(l)
		start.X = util.Clamp(start.X, 0, nchars)
		if start.X > 0 {
			charpos = start.X - 1
			l = util.SliceEnd(l, charpos)
			padMode |= padStart
		}
	}

	re, err := regexp.Compile(r.String() + `\E`)
	if err == nil {
		// r contains \Q without closing \E
		r = re
	}

	if padMode == padStart {
		r = regexp.MustCompile(".(?:" + r.String() + ")")
	} else if padMode == padEnd {
		r = regexp.MustCompile("(?:" + r.String() + ").")
	} else if padMode == padStart|padEnd {
		r = regexp.MustCompile(".(?:" + r.String() + ").")
	}

	return l, charpos, padMode, r
}

func (b *Buffer) findDown(r *regexp.Regexp, start, end Loc) ([2]Loc, bool) {
	lastcn := util.CharacterCount(b.LineBytes(b.LinesNum() - 1))
	if start.Y > b.LinesNum()-1 {
		start.X = lastcn - 1
	}
	if end.Y > b.LinesNum()-1 {
		end.X = lastcn
	}
	start.Y = util.Clamp(start.Y, 0, b.LinesNum()-1)

Review comment (suggestion):
I think this approach is the best. I attempted to avoid other potential problems in my suggestions, but there seems to be none and I realized such would be too rare.

Couldn't `\E` be added only when `padMode & (padStart|padEnd) != 0`, so that it's performed only when needed and not every line?

Otherwise, the current PR should be good enough to address the issue for 2.0.15.

Code after:

func findLineParams(b *Buffer, start, end Loc, i int, r *regexp.Regexp) ([]byte, int, int, *regexp.Regexp) {
	l := b.LineBytes(i)
	charpos := 0
	padMode := 0

	if i == end.Y {
		nchars := util.CharacterCount(l)
		end.X = util.Clamp(end.X, 0, nchars)
		if end.X < nchars {
			l = util.SliceStart(l, end.X+1)
			padMode |= padEnd
		}
	}

	if i == start.Y {
		nchars := util.CharacterCount(l)
		start.X = util.Clamp(start.X, 0, nchars)
		if start.X > 0 {
			charpos = start.X - 1
			l = util.SliceEnd(l, charpos)
			padMode |= padStart
		}
	}

	if padMode != 0 {
		re, err := regexp.Compile(r.String() + `\E`)
		if err == nil {
			// r contains \Q without closing \E
			r = re
		}

		if padMode == padStart {
			r = regexp.MustCompile(".(?:" + r.String() + ")")
		} else if padMode == padEnd {
			r = regexp.MustCompile("(?:" + r.String() + ").")
		} else  {
			// padMode == padStart|padEnd
			r = regexp.MustCompile(".(?:" + r.String() + ").")
		}
	}

	return l, charpos, padMode, r
}

func (b *Buffer) findDown(r *regexp.Regexp, start, end Loc) ([2]Loc, bool) {
	lastcn := util.CharacterCount(b.LineBytes(b.LinesNum() - 1))
	if start.Y > b.LinesNum()-1 {
		start.X = lastcn - 1
	}
	if end.Y > b.LinesNum()-1 {
Repository: zyedidia/micro
File: internal/views/splits.go (Go)

Code before:
		copy(n.parent.children[ind:], successor.children)

		for i := 0; i < len(successor.children); i++ {
			n.parent.children[ind+i].parent = n.parent
		}
	}

	// Update propW and propH since the parent of the children has been updated,
	// so the children have new siblings
	n.parent.markSizes()
}

// String returns the string form of the node and all children (used for debugging)
func (n *Node) String() string {
	var strf func(n *Node, ident int) string
	strf = func(n *Node, ident int) string {
		marker := "|"
		if n.Kind == STHoriz {
			marker = "-"
		}
		var str string
		if n.parent != nil {
			str = fmt.Sprint(strings.Repeat("\t", ident), marker, n.View, n.id, n.parent.id)
		} else {
			str = fmt.Sprint(strings.Repeat("\t", ident), marker, n.View, n.id, " root")
		}
		if n.IsLeaf() {
			str += "🍁"
		}
		str += "\n"
		for _, c := range n.children {
			str += strf(c, ident+1)
		}
		return str
	}
	return strf(n, 0)
}


Review comment (suggestion):
Maybe better?
```
	marker := "/"
	if n.Kind == STVert {
		marker = "|"
	} else if n.Kind == STHoriz {
		marker = "-"
	}
	var str string
	parentId := 0
	if n.parent != nil {
		parentId = n.parent.id
	}
	str = fmt.Sprint(strings.Repeat("\t", ident), marker, n.View, n.id, parentId)
```

Code after:
		successor.Kind = n.Kind
	} else {
		// If the successor node has children, that means it is a chained parent as well.
		// Therefore it can be replaced by its own children.
		origsize := len(n.parent.children)

		// Let's say we have 5 children and want to replace [2] with its children [a] [b] [c]
		// [0] [1] [2] [3] [4] --> [0] [1] [a] [b] [c] [3] [4]
		// insertcount will be `3 - 1 = 2` in this case
		insertcount := len(successor.children) - 1

		n.parent.children = append(n.parent.children, make([]*Node, insertcount)...)
		copy(n.parent.children[ind+insertcount+1:], n.parent.children[ind+1:origsize])
		copy(n.parent.children[ind:], successor.children)

		for i := 0; i < len(successor.children); i++ {
			n.parent.children[ind+i].parent = n.parent
		}
	}

	// Update propW and propH since the parent of the children has been updated,
	// so the children have new siblings
	n.parent.markSizes()
}

// String returns the string form of the node and all children (used for debugging)
func (n *Node) String() string {
	var strf func(n *Node, ident int) string
	strf = func(n *Node, ident int) string {
		marker := ""
		if n.Kind == STHoriz {
			marker = "-"
		} else if n.Kind == STVert {
			marker = "|"
		}

		var parentId uint64 = 0
		if n.parent != nil {
			parentId = n.parent.id
		}

		str := fmt.Sprint(strings.Repeat("\t", ident), marker, n.View, n.id, parentId)
		if n.IsLeaf() {
			str += "🍁"
		}
		str += "\n"
		for _, c := range n.children {
			str += strf(c, ident+1)
		}
		return str
	}
Repository: zyedidia/micro
File: internal/views/splits.go (Go)

Code before:
	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}
	n.parent.unsplit(ind)
	if n.parent.IsLeaf() {
		return n.parent.Unsplit()
	}

	n.parent.flatten()
	return true
}

// flattens the tree by removing unnecessary intermediate parents that have only one child
// and handles the side effect of it
func (n *Node) flatten() {
	if len(n.children) != 1 {
		return
	}

	// Special case for root node
	if n.parent == nil {
		*n = *n.children[0]
		n.parent = nil
		return
	}

	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}

	// Replace current node with its child node to remove chained parent
	successor := n.children[0]
	n.parent.children[ind] = successor
	successor.parent = n.parent

	// Maintain the tree in a consistent state: any child node's kind (horiz vs vert)
	// should be the opposite of its parent's kind.
	if successor.IsLeaf() {
		successor.Kind = n.Kind
	} else {
		// If the successor node has children, that means it is a chained parent as well.
		// Therefore it can be replaced by its own children.
		origsize := len(n.parent.children)

		// Let's say we have 5 children and want to replace [2] with its children [a] [b] [c]

Review comment (bug):
Ah, yes...right. We overwrite the root node with the last remaining children, by which it receives the `id` of the children, with which it is identified in the tab resize.
But still we should keep the removed `n.Kind = STUndef`, otherwise `func (n *Node) String()` wouldn't print what we expect from the new root. Otherwise in case we agree to remove `STUndef` we've to adjust this function to print "/" on `n.parent == nil`.

Am I right that we should keep the fault in [tab.go#L382](https://github.com/micro-editor/micro/blob/3a7403bde4d508afea93a94d1113ca73cf37c1c1/internal/action/tab.go#L382) to identify that something went wrong, because `GetNode()` returns `nil` by intention in case the pane ID wasn't found?

Code after:
	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}
	n.parent.unsplit(ind)
	if n.parent.IsLeaf() {
		return n.parent.Unsplit()
	}

	n.parent.flatten()
	return true
}

// flattens the tree by removing unnecessary intermediate parents that have only one child
// and handles the side effect of it
func (n *Node) flatten() {
	if len(n.children) != 1 {
		return
	}

	// Special case for root node
	if n.parent == nil {
		*n = *n.children[0]
		n.parent = nil
		for _, c := range n.children {
			c.parent = n
		}
		if len(n.children) == 0 {
			n.Kind = STUndef
		}
		return
	}

	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}

	// Replace current node with its child node to remove chained parent
	successor := n.children[0]
	n.parent.children[ind] = successor
	successor.parent = n.parent

	// Maintain the tree in a consistent state: any child node's kind (horiz vs vert)
	// should be the opposite of its parent's kind.
	if successor.IsLeaf() {
		successor.Kind = n.Kind