ronantakizawa/codereview-bench huggingface dataset:
Repository: jesseduffield/lazygit
File: pkg/utils/yaml_utils/yaml_utils.go (Go)

Code before:
	"errors"
	"fmt"

	"gopkg.in/yaml.v3"
)

// takes a yaml document in bytes, a path to a key, and a value to set. The value must be a scalar.
func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, error) {
	// Parse the YAML file.
	var node yaml.Node
	err := yaml.Unmarshal(yamlBytes, &node)
	if err != nil {
		return nil, fmt.Errorf("failed to parse YAML: %w", err)
	}

	// Empty document: need to create the top-level map ourselves
	if len(node.Content) == 0 {
		node.Content = append(node.Content, &yaml.Node{
			Kind: yaml.MappingNode,
		})
	}

	body := node.Content[0]

	if body.Kind != yaml.MappingNode {
		return yamlBytes, errors.New("yaml document is not a dictionary")
	}

	updateYamlNode(body, path, value)

	// Convert the updated YAML node back to YAML bytes.
	updatedYAMLBytes, err := yaml.Marshal(body)
	if err != nil {
		return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
	}

	return updatedYAMLBytes, nil
}

// Recursive function to update the YAML node.
func updateYamlNode(node *yaml.Node, path []string, value string) {
	if len(path) == 0 {
		node.Value = value
		return
	}

	key := path[0]
	for i := 0; i < len(node.Content)-1; i += 2 {
		if node.Content[i].Value == key {
			updateYamlNode(node.Content[i+1], path[1:], value)
			return

Review comment (suggestion):
Returning the unchanged `yamlBytes` here; could also return an empty bytes slice. Does it matter? (Clients are unlikely to look at the returned content when there was an error.)

Code after:
	"errors"
	"fmt"

	"gopkg.in/yaml.v3"
)

// takes a yaml document in bytes, a path to a key, and a value to set. The value must be a scalar.
func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, error) {
	// Parse the YAML file.
	var node yaml.Node
	err := yaml.Unmarshal(yamlBytes, &node)
	if err != nil {
		return nil, fmt.Errorf("failed to parse YAML: %w", err)
	}

	// Empty document: need to create the top-level map ourselves
	if len(node.Content) == 0 {
		node.Content = append(node.Content, &yaml.Node{
			Kind: yaml.MappingNode,
		})
	}

	body := node.Content[0]

	if body.Kind != yaml.MappingNode {
		return yamlBytes, errors.New("yaml document is not a dictionary")
	}

	if didChange, err := updateYamlNode(body, path, value); err != nil || !didChange {
		return yamlBytes, err
	}

	// Convert the updated YAML node back to YAML bytes.
	updatedYAMLBytes, err := yaml.Marshal(body)
	if err != nil {
		return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
	}

	return updatedYAMLBytes, nil
}

// Recursive function to update the YAML node.
func updateYamlNode(node *yaml.Node, path []string, value string) (bool, error) {
	if len(path) == 0 {
		if node.Kind != yaml.ScalarNode {
			return false, errors.New("yaml node is not a scalar")
		}
		if node.Value != value {
			node.Value = value
			return true, nil
		}
Repository: jesseduffield/lazygit
File: pkg/tasks/tasks.go (Go)

Code before:
			}

			// for pty's we need to call onDone here so that cmd.Wait() doesn't block forever
			if onDoneWrapper != nil {
				onDoneWrapper()
			}
		})

		loadingMutex := deadlock.Mutex{}

		// not sure if it's the right move to redefine this or not
		self.readLines = make(chan LinesToRead, 1024)

		done := make(chan struct{})

		scanner := bufio.NewScanner(r)
		scanner.Split(bufio.ScanLines)

		data := make(chan []byte)

		go utils.Safe(func() {
			for scanner.Scan() {
				data <- scanner.Bytes()
			}
			close(data)
		})

		loaded := false

		go utils.Safe(func() {
			ticker := time.NewTicker(time.Millisecond * 200)
			defer ticker.Stop()
			select {
			case <-stop:
				return
			case <-ticker.C:
				loadingMutex.Lock()
				if !loaded {
					self.beforeStart()
					_, _ = self.writer.Write([]byte("loading..."))
					self.refreshView()
				}
				loadingMutex.Unlock()
			}
		})

		go utils.Safe(func() {
			isViewStale := true
			writeToView := func(content []byte) {
				_, _ = self.writer.Write(content)
				isViewStale = true

Review comment (suggestion):
Is there a risk that we'll get a bunch of these goroutines banked up after iterating through a bunch of items in a side view? Like, do we eventually make it out of the `scanner` loop here? If not, it would be good if there was a way to force us to return from the goroutine.

Although in saying that, if that were possible we'd be able to solve it directly in the original code, hmm...

Code after:
			}

			// for pty's we need to call onDone here so that cmd.Wait() doesn't block forever
			if onDoneWrapper != nil {
				onDoneWrapper()
			}
		})

		loadingMutex := deadlock.Mutex{}

		// not sure if it's the right move to redefine this or not
		self.readLines = make(chan LinesToRead, 1024)

		done := make(chan struct{})

		scanner := bufio.NewScanner(r)
		scanner.Split(bufio.ScanLines)

		data := make(chan []byte)

		// We're reading from the scanner in a separate goroutine because on windows
		// if running git through a shim, we sometimes kill the parent process without
		// killing its children, meaning the scanner blocks forever. This solution
		// leaves us with a dead goroutine, but it's better than blocking all
		// rendering to main views.
		go utils.Safe(func() {
			for scanner.Scan() {
				data <- scanner.Bytes()
			}
			close(data)
		})

		loaded := false

		go utils.Safe(func() {
			ticker := time.NewTicker(time.Millisecond * 200)
			defer ticker.Stop()
			select {
			case <-stop:
				return
			case <-ticker.C:
				loadingMutex.Lock()
				if !loaded {
					self.beforeStart()
					_, _ = self.writer.Write([]byte("loading..."))
					self.refreshView()
				}
				loadingMutex.Unlock()
			}
		})
Repository: jesseduffield/lazygit
File: pkg/gui/presentation/commits.go (Go)

Code before:
	cols = append(cols, bisectString)
	if fullDescription {
		cols = append(cols, style.FgBlue.Sprint(
			utils.UnixToDateSmart(now, commit.UnixTimestamp, timeFormat, shortTimeFormat),
		))
	}
	cols = append(
		cols,
		actionString,
		authorFunc(commit.AuthorName),
		graphLine+tagString+theme.DefaultTextColor.Sprint(name),
	)

	return cols
}

func getBisectStatusColor(status BisectStatus) style.TextStyle {
	switch status {
	case BisectStatusNone:
		return style.FgBlack
	case BisectStatusNew:
		return style.FgRed
	case BisectStatusOld:
		return style.FgGreen
	case BisectStatusSkipped:
		return style.FgYellow
	case BisectStatusCurrent:
		return style.FgMagenta
	case BisectStatusCandidate:
		return style.FgBlue
	}

	// shouldn't land here
	return style.FgWhite
}

func getShaColor(
	commit *models.Commit,
	diffName string,
	cherryPickedCommitShaSet *set.Set[string],
	isMarkedBaseCommit bool,
	bisectStatus BisectStatus,
	bisectInfo *git_commands.BisectInfo,
) style.TextStyle {
	if bisectInfo.Started() {
		return getBisectStatusColor(bisectStatus)
	}

	diffed := commit.Sha != "" && commit.Sha == diffName
	shaColor := theme.DefaultTextColor
	switch commit.Status {

Review comment (suggestion):
Marked the commit as "RFC" because this is not the most beautiful design. Suggestions for a better UI welcome; but I prefer this very explicit and in-your-face visualization over just coloring the sha.

Code after:
		if commit.ExtraInfo != "" {
			tagString = style.FgMagenta.SetBold().Sprint(commit.ExtraInfo) + " "
		}
	} else {
		if len(commit.Tags) > 0 {
			tagString = theme.DiffTerminalColor.SetBold().Sprint(strings.Join(commit.Tags, " ")) + " "
		}

		if branchHeadsToVisualize.Includes(commit.Sha) && commit.Status != models.StatusMerged {
			tagString = style.FgCyan.SetBold().Sprint(
				lo.Ternary(icons.IsIconEnabled(), icons.BRANCH_ICON, "*") + " " + tagString)
		}
	}

	name := commit.Name
	if parseEmoji {
		name = emoji.Sprint(name)
	}

	if isYouAreHereCommit {
		color := lo.Ternary(commit.Action == models.ActionConflict, style.FgRed, style.FgYellow)
		youAreHere := color.Sprintf("<-- %s ---", common.Tr.YouAreHere)
		name = fmt.Sprintf("%s %s", youAreHere, name)
	} else if isMarkedBaseCommit {
		rebaseFromHere := style.FgYellow.Sprint(common.Tr.MarkedCommitMarker)
		name = fmt.Sprintf("%s %s", rebaseFromHere, name)
	} else if !willBeRebased {
		willBeRebased := style.FgYellow.Sprint("✓")
		name = fmt.Sprintf("%s %s", willBeRebased, name)
	}

	authorFunc := authors.ShortAuthor
	if fullDescription {
		authorFunc = authors.LongAuthor
	}

	cols := make([]string, 0, 7)
	if icons.IsIconEnabled() {
		cols = append(cols, shaColor.Sprint(icons.IconForCommit(commit)))
	}
	cols = append(cols, shaColor.Sprint(commit.ShortSha()))
	cols = append(cols, bisectString)
	if fullDescription {
		cols = append(cols, style.FgBlue.Sprint(
			utils.UnixToDateSmart(now, commit.UnixTimestamp, timeFormat, shortTimeFormat),
		))
	}
	cols = append(
		cols,
		actionString,
		authorFunc(commit.AuthorName),
Repository: jesseduffield/lazygit
File: pkg/gui/controllers/switch_to_sub_commits_controller.go (Go)

Code before:

	setSubCommits func([]*models.Commit)
}

func NewSwitchToSubCommitsController(
	controllerCommon *ControllerCommon,
	setSubCommits func([]*models.Commit),
	context CanSwitchToSubCommits,
) *SwitchToSubCommitsController {
	return &SwitchToSubCommitsController{
		baseController: baseController{},
		c:              controllerCommon,
		context:        context,
		setSubCommits:  setSubCommits,
	}
}

func (self *SwitchToSubCommitsController) GetKeybindings(opts types.KeybindingsOpts) []*types.Binding {
	bindings := []*types.Binding{
		{
			Handler:     self.viewCommits,
			Key:         opts.GetKey(opts.Config.Universal.GoInto),
			Description: self.c.Tr.ViewCommits,
		},
		{
			Handler:     self.viewLeftRightCommits,
			Key:         opts.GetKey(opts.Config.Branches.ViewLeftRightCommits),
			Description: self.c.Tr.ViewLeftRightCommits,
		},
	}

	return bindings
}

func (self *SwitchToSubCommitsController) GetOnClick() func() error {
	return self.viewCommits
}

func (self *SwitchToSubCommitsController) viewCommitsImpl(leftRight bool) error {
	ref := self.context.GetSelectedRef()
	if ref == nil {
		return nil
	}

	upstreamRefName := ""
	if leftRight {
		if branch, ok := ref.(*models.Branch); ok {
			if branch.UpstreamRemote != "" && branch.UpstreamBranch != "" {
				upstreamRefName = fmt.Sprintf("refs/remotes/%s/%s", branch.UpstreamRemote, branch.UpstreamBranch)
			}
		}

Review comment (refactor):
This controller is attached to a few contexts:
```
		gui.State.Contexts.Branches,
		gui.State.Contexts.RemoteBranches,
		gui.State.Contexts.Tags,
		gui.State.Contexts.ReflogCommits,
```
But we only want this keybinding for branches right?

Might need to create a 'switch_to_subcommits_helper' which is a reflection of a poor architecture here: often controller code needs to be used from somewhere else and the only solution is to extract that code out into a helper.

Code after:
	controllerCommon *ControllerCommon,
	context CanSwitchToSubCommits,
) *SwitchToSubCommitsController {
	return &SwitchToSubCommitsController{
		baseController: baseController{},
		c:              controllerCommon,
		context:        context,
	}
}

func (self *SwitchToSubCommitsController) GetKeybindings(opts types.KeybindingsOpts) []*types.Binding {
	bindings := []*types.Binding{
		{
			Handler:     self.viewCommits,
			Key:         opts.GetKey(opts.Config.Universal.GoInto),
			Description: self.c.Tr.ViewCommits,
		},
	}

	return bindings
}

func (self *SwitchToSubCommitsController) GetOnClick() func() error {
	return self.viewCommits
}

func (self *SwitchToSubCommitsController) viewCommits() error {
	ref := self.context.GetSelectedRef()
	if ref == nil {
		return nil
	}

	return self.c.Helpers().SubCommits.ViewSubCommits(helpers.ViewSubCommitsOpts{
		Ref:             ref,
		TitleRef:        ref.RefName(),
		Context:         self.context,
		ShowBranchHeads: self.context.ShowBranchHeadsInSubCommits(),
	})
}

func (self *SwitchToSubCommitsController) Context() types.Context {
	return self.context
}
Repository: jesseduffield/lazygit
File: pkg/gui/controllers/branches_controller.go (Go)

Code before:
						},
					)
					if err != nil {
						return err
					}

					self.setSubCommits(commits)
					self.c.Helpers().Refresh.RefreshAuthors(commits)

					subCommitsContext := self.c.Contexts().SubCommits
					subCommitsContext.SetSelectedLineIdx(0)
					subCommitsContext.SetParentContext(self.context())
					subCommitsContext.SetWindowName(self.context().GetWindowName())
					subCommitsContext.SetTitleRef(ref.Description())
					subCommitsContext.SetRef(ref)
					subCommitsContext.SetLimitCommits(true)
					subCommitsContext.SetShowBranchHeads(self.context().ShowBranchHeadsInSubCommits())
					subCommitsContext.ClearSearchString()
					subCommitsContext.GetView().ClearSearch()

					err = self.c.PostRefreshUpdate(self.c.Contexts().SubCommits)
					if err != nil {
						return err
					}

					return self.c.PushContext(self.c.Contexts().SubCommits)
				},
				Key: 'v',
			},
			{
				LabelColumns: []string{self.c.Tr.UnsetUpstream},
				OnPress: func() error {
					if err := self.c.Git().Branch.UnsetUpstream(selectedBranch.Name); err != nil {
						return self.c.Error(err)
					}
					if err := self.c.Refresh(types.RefreshOptions{
						Mode: types.SYNC,
						Scope: []types.RefreshableView{
							types.BRANCHES,
							types.COMMITS,
						},
					}); err != nil {
						return self.c.Error(err)
					}
					return nil
				},
				Key: 'u',
			},
			{
				LabelColumns: []string{self.c.Tr.SetUpstream},
				OnPress: func() error {

Review comment (refactor):
All of this code is duplicated from `switch_to_sub_commits_controller.go`, we probably want to move it to a shared place. Should this become a `sub_commits_helper`?

Code after:
					}
					if err := self.c.Refresh(types.RefreshOptions{
						Mode: types.SYNC,
						Scope: []types.RefreshableView{
							types.BRANCHES,
							types.COMMITS,
						},
					}); err != nil {
						return self.c.Error(err)
					}
					return nil
				},
				Key: 'u',
			},
			{
				LabelColumns: []string{self.c.Tr.SetUpstream},
				OnPress: func() error {
					return self.c.Helpers().Upstream.PromptForUpstreamWithoutInitialContent(selectedBranch, func(upstream string) error {
						upstreamRemote, upstreamBranch, err := self.c.Helpers().Upstream.ParseUpstream(upstream)
						if err != nil {
							return self.c.Error(err)
						}

						if err := self.c.Git().Branch.SetUpstream(upstreamRemote, upstreamBranch, selectedBranch.Name); err != nil {
							return self.c.Error(err)
						}
						if err := self.c.Refresh(types.RefreshOptions{
							Mode: types.SYNC,
							Scope: []types.RefreshableView{
								types.BRANCHES,
								types.COMMITS,
							},
						}); err != nil {
							return self.c.Error(err)
						}
						return nil
					})
				},
				Key: 's',
			},
		},
	})
}

func (self *BranchesController) Context() types.Context {
	return self.context()
}

func (self *BranchesController) context() *context.BranchesContext {
	return self.c.Contexts().Branches
}
Repository: jesseduffield/lazygit
File: pkg/gui/context/search_trait.go (Go)

Code before:
	"github.com/jesseduffield/lazygit/pkg/gui/keybindings"
	"github.com/jesseduffield/lazygit/pkg/theme"
	"github.com/jesseduffield/lazygit/pkg/utils"
)

type SearchTrait struct {
	c *ContextCommon

	searchHistory *utils.CircularBuffer[string]
	searchString  string
}

func NewSearchTrait(c *ContextCommon) *SearchTrait {
	return &SearchTrait{
		c:             c,
		searchHistory: utils.NewCircularBuffer[string](10),
	}
}

func (self *SearchTrait) GetSearchString() string {
	return self.searchString
}

func (self *SearchTrait) SetSearchString(searchString string) {
	self.searchString = searchString
	if searchString != "" {
		self.searchHistory.Push(searchString)
	}
}

func (self *SearchTrait) GetSearchHistory() *utils.CircularBuffer[string] {
	return self.searchHistory
}

func (self *SearchTrait) ClearSearchString() {
	self.SetSearchString("")
}

// used for type switch
func (self *SearchTrait) IsSearchableContext() {}

func (self *SearchTrait) onSelectItemWrapper(innerFunc func(int) error) func(int, int, int) error {
	keybindingConfig := self.c.UserConfig.Keybinding

	return func(y int, index int, total int) error {
		if total == 0 {
			self.c.SetViewContent(
				self.c.Views().Search,
				fmt.Sprintf(
					self.c.Tr.NoMatchesFor,
					self.searchString,

Review comment (suggestion):
Maybe it would be better just to early return if `searchString` is empty?

No reason to set `self.searchString` if it's empty, and it would be a bit cleaner.

Code after:
	"github.com/jesseduffield/lazygit/pkg/gui/keybindings"
	"github.com/jesseduffield/lazygit/pkg/theme"
)

type SearchTrait struct {
	c *ContextCommon
	*SearchHistory

	searchString string
}

func NewSearchTrait(c *ContextCommon) *SearchTrait {
	return &SearchTrait{
		c:             c,
		SearchHistory: NewSearchHistory(),
	}
}

func (self *SearchTrait) GetSearchString() string {
	return self.searchString
}

func (self *SearchTrait) SetSearchString(searchString string) {
	self.searchString = searchString
}

func (self *SearchTrait) ClearSearchString() {
	self.SetSearchString("")
}

// used for type switch
func (self *SearchTrait) IsSearchableContext() {}

func (self *SearchTrait) onSelectItemWrapper(innerFunc func(int) error) func(int, int, int) error {
	keybindingConfig := self.c.UserConfig.Keybinding

	return func(y int, index int, total int) error {
		if total == 0 {
			self.c.SetViewContent(
				self.c.Views().Search,
				fmt.Sprintf(
					self.c.Tr.NoMatchesFor,
					self.searchString,
					theme.OptionsFgColor.Sprintf(self.c.Tr.ExitSearchMode, keybindings.Label(keybindingConfig.Universal.Return)),
				),
			)
			return nil
		}
		self.c.SetViewContent(
			self.c.Views().Search,
			fmt.Sprintf(
Repository: jesseduffield/lazygit
File: pkg/utils/history_buffer.go (Go)

Code before:
package utils

import "fmt"

type HistoryBuffer[T any] struct {
	maxSize int
	Items   []T
}

func NewHistoryBuffer[T any](maxSize int) *HistoryBuffer[T] {
	return &HistoryBuffer[T]{
		maxSize: maxSize,
		Items:   make([]T, 0, maxSize),
	}
}

func (self *HistoryBuffer[T]) Push(item T) {
	if len(self.Items) == self.maxSize {
		self.Items = self.Items[:len(self.Items)-1]
	}
	self.Items = append([]T{item}, self.Items...)
}

func (self *HistoryBuffer[T]) Pop() (T, error) {
	var item T
	if len(self.Items) == 0 {
		return item, fmt.Errorf("Buffer is empty")
	}
	item = self.Items[0]
	self.Items = self.Items[1:]
	return item, nil
}

Review comment (suggestion):
```suggestion
	items   []T
```
This ought to be private

Code after:
package utils

import "fmt"

type HistoryBuffer[T any] struct {
	maxSize int
	items   []T
}

func NewHistoryBuffer[T any](maxSize int) *HistoryBuffer[T] {
	return &HistoryBuffer[T]{
		maxSize: maxSize,
		items:   make([]T, 0, maxSize),
	}
}

func (self *HistoryBuffer[T]) Push(item T) {
	if len(self.items) == self.maxSize {
		self.items = self.items[:len(self.items)-1]
	}
	self.items = append([]T{item}, self.items...)
}

func (self *HistoryBuffer[T]) PeekAt(index int) (T, error) {
	var item T
	if len(self.items) == 0 {
		return item, fmt.Errorf("Buffer is empty")
	}
	if len(self.items) <= index || index < -1 {
		return item, fmt.Errorf("Index out of range")
	}
	if index == -1 {
Repository: jesseduffield/lazygit
File: pkg/integration/components/test_driver.go (Go)

Code before:
package components

import (
	"fmt"
	"time"

	"github.com/atotto/clipboard"
	"github.com/jesseduffield/lazygit/pkg/config"
	integrationTypes "github.com/jesseduffield/lazygit/pkg/integration/types"
)

type TestDriver struct {
	gui             integrationTypes.GuiDriver
	keys            config.KeybindingConfig
	pushKeyDelay    int
	mouseClickDelay int
	*assertionHelper
	shell *Shell
}

func NewTestDriver(gui integrationTypes.GuiDriver, shell *Shell, keys config.KeybindingConfig, pushKeyDelay, mouseClickDelay int) *TestDriver {
	return &TestDriver{
		gui:             gui,
		keys:            keys,
		pushKeyDelay:    pushKeyDelay,
		mouseClickDelay: mouseClickDelay,
		assertionHelper: &assertionHelper{gui: gui},
		shell:           shell,
	}
}

// key is something like 'w' or '<space>'. It's best not to pass a direct value,
// but instead to go through the default user config to get a more meaningful key name
func (self *TestDriver) press(keyStr string) {
	self.SetCaption(fmt.Sprintf("Pressing %s", keyStr))
	self.gui.PressKey(keyStr)
	self.Wait(self.pushKeyDelay)
}

// for use when typing or navigating, because in demos we want that to happen
// faster

Review comment (suggestion):
I'm happy to use the same delay as pushKeyDelay: we could rename it to `inputDelay` to capture both kinds of inputs. That goes for the env var too: we can rename to `INPUT_DELAY` (nobody would depend on this env var because it's used internally)

Code after:
package components

import (
	"fmt"
	"time"

	"github.com/atotto/clipboard"
	"github.com/jesseduffield/lazygit/pkg/config"
	integrationTypes "github.com/jesseduffield/lazygit/pkg/integration/types"
)

type TestDriver struct {
	gui        integrationTypes.GuiDriver
	keys       config.KeybindingConfig
	inputDelay int
	*assertionHelper
	shell *Shell
}

func NewTestDriver(gui integrationTypes.GuiDriver, shell *Shell, keys config.KeybindingConfig, inputDelay int) *TestDriver {
	return &TestDriver{
		gui:             gui,
		keys:            keys,
		inputDelay:      inputDelay,
		assertionHelper: &assertionHelper{gui: gui},
		shell:           shell,
	}
}

// key is something like 'w' or '<space>'. It's best not to pass a direct value,
// but instead to go through the default user config to get a more meaningful key name
func (self *TestDriver) press(keyStr string) {
	self.SetCaption(fmt.Sprintf("Pressing %s", keyStr))
	self.gui.PressKey(keyStr)
	self.Wait(self.inputDelay)
}

// for use when typing or navigating, because in demos we want that to happen
// faster
func (self *TestDriver) pressFast(keyStr string) {
	self.SetCaption("")