ronantakizawa/codereview-bench huggingface dataset:
Repository: jesseduffield/lazygit
File: pkg/commands/git_commands/stash_loader.go (Go)

Code before:
			if lines[i] == filterPath {
				stashEntries = append(stashEntries, currentStashEntry)
				continue outer
			}
		}
	}
	return stashEntries
}

func (self *StashLoader) getUnfilteredStashEntries() []*models.StashEntry {
	cmdArgs := NewGitCmd("stash").Arg("list", "-z", "--pretty=%gs date@{%ct}").ToArgv()

	rawString, _ := self.cmd.New(cmdArgs).DontLog().RunWithOutput()
	return lo.Map(utils.SplitNul(rawString), func(line string, index int) *models.StashEntry {
		return self.stashEntryFromLine(line, index)
	})
}

func (c *StashLoader) stashEntryFromLine(line string, index int) *models.StashEntry {
	model := &models.StashEntry{
		Name:  line,
		Index: index,
	}

	regular := regexp.MustCompile(`(?P<line>.*?) date@\{(?P<age>.*?)\}`)
	match := regular.FindStringSubmatch(line)
	if match == nil {
		return model
	}

	msg, tstr := match[1], match[2]
	t, err := strconv.ParseInt(tstr, 10, 64)
	if err != nil {
		return model
	}

	unixTimestamp := time.Unix(t, 0)
	model.Time = &unixTimestamp
	model.Name = msg
	return model
}


Review comment (suggestion):
OK, my bad since I advised you to go with the code from the issue comment without looking at it closely enough.

I think this can be simplified so that we don't need this regex business. If you make the format string `"--pretty=%ct|%gs"`, then you can simply split the string on `|`. I picked `|` arbitrarily, any non-digit character will do.

Something like this:
```go
tstr, msg, ok := strings.Cut(line, "|")
if ok {
	model.Name = msg
	t, err := strconv.ParseInt(tstr, 10, 64)
	if err == nil {
		model.Recency = utils.UnixToTimeAgo(t)
	}
}
return model
```

Code after:
				stashEntries = append(stashEntries, currentStashEntry)
				continue outer
			}
		}
	}
	return stashEntries
}

func (self *StashLoader) getUnfilteredStashEntries() []*models.StashEntry {
	cmdArgs := NewGitCmd("stash").Arg("list", "-z", "--pretty=%ct|%gs").ToArgv()

	rawString, _ := self.cmd.New(cmdArgs).DontLog().RunWithOutput()
	return lo.Map(utils.SplitNul(rawString), func(line string, index int) *models.StashEntry {
		return self.stashEntryFromLine(line, index)
	})
}

func (c *StashLoader) stashEntryFromLine(line string, index int) *models.StashEntry {
	model := &models.StashEntry{
		Name:  line,
		Index: index,
	}

	tstr, msg, ok := strings.Cut(line, "|")
	if !ok {
		return model
	}

	t, err := strconv.ParseInt(tstr, 10, 64)
	if err != nil {
		return model
	}

	model.Name = msg
	model.Recency = utils.UnixToTimeAgo(t)

	return model
}
Repository: jesseduffield/lazygit
File: pkg/commands/models/stash_entry.go (Go)

Code before:
package models

import (
	"errors"
	"fmt"
	"time"

	"github.com/jesseduffield/lazygit/pkg/utils"
)

// StashEntry : A git stash entry
type StashEntry struct {
	Index int
	Time  *time.Time
	Name  string
}

func (s *StashEntry) FullRefName() string {
	return s.RefName()
}

func (s *StashEntry) hasTime() bool {
	return s.Time != nil
}

func (s *StashEntry) Age() (string, error) {
	if !s.hasTime() {
		return "", errors.New("no time associated to this stash entry")
	}
	return utils.UnixToTimeAgo(s.Time.Unix()), nil
}

func (s *StashEntry) Line() string {
	line := s.Name
	if !s.hasTime() {
		return line
	}

	age, err := s.Age()

Review comment (bug):
I'd make this consistent with how we do this for branches, and have a `Recency string` field. No accessors at all needed. Clients just work with the field directly, it will be the empty string if the loader failed to determine it for some reason (which can't really happen except when there are bugs in git).

Code after:
package models

import "fmt"

// StashEntry : A git stash entry
type StashEntry struct {
	Index   int
	Recency string
	Name    string
}

func (s *StashEntry) FullRefName() string {
	return s.RefName()
}

func (s *StashEntry) RefName() string {
	return fmt.Sprintf("stash@{%d}", s.Index)
}

func (s *StashEntry) ParentRefName() string {
	return s.RefName() + "^"
}

func (s *StashEntry) ID() string {
	return s.RefName()
}

func (s *StashEntry) Description() string {
	return s.RefName() + ": " + s.Name
}
Repository: jesseduffield/lazygit
File: pkg/gui/presentation/stash_entries.go (Go)

Code before:
	"github.com/jesseduffield/lazygit/pkg/commands/models"
	"github.com/jesseduffield/lazygit/pkg/gui/presentation/icons"
	"github.com/jesseduffield/lazygit/pkg/theme"
	"github.com/samber/lo"
)

func GetStashEntryListDisplayStrings(stashEntries []*models.StashEntry, diffName string) [][]string {
	return lo.Map(stashEntries, func(stashEntry *models.StashEntry, _ int) []string {
		diffed := stashEntry.RefName() == diffName
		return getStashEntryDisplayStrings(stashEntry, diffed)
	})
}

// getStashEntryDisplayStrings returns the display string of branch
func getStashEntryDisplayStrings(s *models.StashEntry, diffed bool) []string {
	textStyle := theme.DefaultTextColor
	if diffed {
		textStyle = theme.DiffTerminalColor
	}

	res := make([]string, 0, 2)
	if icons.IsIconEnabled() {
		res = append(res, textStyle.Sprint(icons.IconForStash(s)))
	}

	res = append(res, textStyle.Sprint(s.Line()))
	return res
}


Review comment (suggestion):
We should do exactly what we do for branches here, and make the recency a separate column (the first one, it should go before the icon). Also, color it cyan [like we do for branches](https://github.com/jesseduffield/lazygit/blob/66b608b2f9857296645f964b67b821e431d7b807/pkg/gui/presentation/branches.go#L101). (No need for a recencyColor variable of course.)

Code after:
	"github.com/jesseduffield/lazygit/pkg/commands/models"
	"github.com/jesseduffield/lazygit/pkg/gui/presentation/icons"
	"github.com/jesseduffield/lazygit/pkg/gui/style"
	"github.com/jesseduffield/lazygit/pkg/theme"
	"github.com/samber/lo"
)

func GetStashEntryListDisplayStrings(stashEntries []*models.StashEntry, diffName string) [][]string {
	return lo.Map(stashEntries, func(stashEntry *models.StashEntry, _ int) []string {
		diffed := stashEntry.RefName() == diffName
		return getStashEntryDisplayStrings(stashEntry, diffed)
	})
}

// getStashEntryDisplayStrings returns the display string of branch
func getStashEntryDisplayStrings(s *models.StashEntry, diffed bool) []string {
	textStyle := theme.DefaultTextColor
	if diffed {
		textStyle = theme.DiffTerminalColor
	}

	res := make([]string, 0, 3)
	res = append(res, style.FgCyan.Sprint(s.Recency))

	if icons.IsIconEnabled() {
		res = append(res, textStyle.Sprint(icons.IconForStash(s)))
	}

	res = append(res, textStyle.Sprint(s.Name))
	return res
}
Repository: jesseduffield/lazygit
File: pkg/integration/tests/stash/stash.go (Go)

Code before:

var Stash = NewIntegrationTest(NewIntegrationTestArgs{
	Description:  "Stashing files directly (not going through the stash menu)",
	ExtraCmdArgs: []string{},
	Skip:         false,
	SetupConfig:  func(config *config.AppConfig) {},
	SetupRepo: func(shell *Shell) {
		shell.EmptyCommit("initial commit")
		shell.CreateFile("file", "content")
		shell.GitAddAll()
	},
	Run: func(t *TestDriver, keys config.KeybindingConfig) {
		t.Views().Stash().
			IsEmpty()

		t.Views().Files().
			Lines(
				Contains("file"),
			).
			Press(keys.Files.StashAllChanges)

		t.ExpectPopup().Prompt().Title(Equals("Stash changes")).Type("my stashed file").Confirm()

		t.Views().Stash().
			Lines(
				MatchesRegexp("\\ds .* my stashed file"),
			)

		t.Views().Files().
			IsEmpty()
	},
})


Review comment (security):
Nitpick: it's useful to use backticks instead of double quotes for regular expressions, you don't have to escape the backslash then and it makes the regex easier to read (not a big deal in this simple case, but useful to remember in general).

```suggestion
				MatchesRegexp(`\ds .* my stashed file`),
```

Code after:

var Stash = NewIntegrationTest(NewIntegrationTestArgs{
	Description:  "Stashing files directly (not going through the stash menu)",
	ExtraCmdArgs: []string{},
	Skip:         false,
	SetupConfig:  func(config *config.AppConfig) {},
	SetupRepo: func(shell *Shell) {
		shell.EmptyCommit("initial commit")
		shell.CreateFile("file", "content")
		shell.GitAddAll()
	},
	Run: func(t *TestDriver, keys config.KeybindingConfig) {
		t.Views().Stash().
			IsEmpty()

		t.Views().Files().
			Lines(
				Contains("file"),
			).
			Press(keys.Files.StashAllChanges)

		t.ExpectPopup().Prompt().Title(Equals("Stash changes")).Type("my stashed file").Confirm()

		t.Views().Stash().
			Lines(
				MatchesRegexp(`\ds .* my stashed file`),
			)

		t.Views().Files().
			IsEmpty()
	},
})
Repository: jesseduffield/lazygit
File: pkg/commands/git_commands/branch_loader_test.go (Go)

Code before:
package git_commands

// "*|feat/detect-purge|origin/feat/detect-purge|[ahead 1]"
import (
	"testing"

	"github.com/jesseduffield/lazygit/pkg/commands/models"
	"github.com/stretchr/testify/assert"
)

func TestObtainBranch(t *testing.T) {
	type scenario struct {
		testName       string
		input          []string
		expectedBranch *models.Branch
	}

	scenarios := []scenario{
		{
			testName: "TrimHeads",
			input:    []string{"", "heads/a_branch", "", "", "subject", "123", ""},
			expectedBranch: &models.Branch{
				Name:       "a_branch",
				Pushables:  "?",
				Pullables:  "?",
				Head:       false,
				Subject:    "subject",
				CommitHash: "123",
			},
		},
		{
			testName: "NoUpstream",
			input:    []string{"", "a_branch", "", "", "subject", "123", ""},
			expectedBranch: &models.Branch{
				Name:       "a_branch",
				Pushables:  "?",
				Pullables:  "?",
				Head:       false,
				Subject:    "subject",
				CommitHash: "123",
			},
		},
		{
			testName: "IsHead",
			input:    []string{"*", "a_branch", "", "", "subject", "123", ""},
			expectedBranch: &models.Branch{

Review comment (suggestion):
We are mocking what git returns to us here, and an empty string is very unrealistic, git will never return that. I'd suggest to return a real number that could happen in practice; one way to do that would be
```
	now := time.Now().Unix()
	twoAndAHalfHoursAgo := strconv.Itoa(int(now - 2.5*60*60))
```
This will result in a recency string of `2h`.

If you follow the suggestion to add a `storeCommitterDateAsRecency` parameter, it would be good to add that to the scenario struct and add one more test case where it is true.

Code after:
package git_commands

// "*|feat/detect-purge|origin/feat/detect-purge|[ahead 1]"
import (
	"strconv"
	"testing"
	"time"

	"github.com/jesseduffield/lazygit/pkg/commands/models"
	"github.com/stretchr/testify/assert"
)

func TestObtainBranch(t *testing.T) {
	type scenario struct {
		testName                 string
		input                    []string
		storeCommitDateAsRecency bool
		expectedBranch           *models.Branch
	}

	// Use a time stamp of 2 1/2 hours ago, resulting in a recency string of "2h"
	now := time.Now().Unix()
	timeStamp := strconv.Itoa(int(now - 2.5*60*60))

	scenarios := []scenario{
		{
			testName:                 "TrimHeads",
			input:                    []string{"", "heads/a_branch", "", "", "subject", "123", timeStamp},
			storeCommitDateAsRecency: false,
			expectedBranch: &models.Branch{
				Name:       "a_branch",
				Pushables:  "?",
				Pullables:  "?",
				Head:       false,
				Subject:    "subject",
				CommitHash: "123",
			},
		},
		{
			testName:                 "NoUpstream",
			input:                    []string{"", "a_branch", "", "", "subject", "123", timeStamp},
			storeCommitDateAsRecency: false,
			expectedBranch: &models.Branch{
				Name:       "a_branch",
				Pushables:  "?",
				Pullables:  "?",
Repository: jesseduffield/lazygit
File: pkg/commands/git_commands/branch_loader.go (Go)

Code before:
		Arg(fmt.Sprintf("--sort=%s", sortOrder)).
		Arg(fmt.Sprintf("--format=%s", format)).
		Arg("refs/heads").
		ToArgv()

	return self.cmd.New(cmdArgs).DontLog().RunWithOutput()
}

var branchFields = []string{
	"HEAD",
	"refname:short",
	"upstream:short",
	"upstream:track",
	"subject",
	"objectname",
	"committerdate:unix",
}

// Obtain branch information from parsed line output of getRawBranches()
func obtainBranch(split []string, storeCommitDateAsRecency bool) *models.Branch {
	headMarker := split[0]
	fullName := split[1]
	upstreamName := split[2]
	track := split[3]
	subject := split[4]
	commitHash := split[5]

	name := strings.TrimPrefix(fullName, "heads/")
	pushables, pullables, gone := parseUpstreamInfo(upstreamName, track)

	recency := ""
	if storeCommitDateAsRecency {
		commitDate := split[6]
		if unixTimestamp, err := strconv.ParseInt(commitDate, 10, 64); err == nil {
			recency = utils.UnixToTimeAgo(unixTimestamp)
		}
	}

	return &models.Branch{
		Name:         name,
		Recency:      recency,
		Pushables:    pushables,
		Pullables:    pullables,
		UpstreamGone: gone,
		Head:         headMarker == "*",
		Subject:      subject,
		CommitHash:   commitHash,
	}
}

func parseUpstreamInfo(upstreamName string, track string) (string, string, bool) {

Review comment (suggestion):
I'd prefer if this stays here; it's good to see how the fields are unpacked in one place. This requires adding a time stamp to the mock data of all tests, which is preferable too because it's closer to what git returns in production. (Changed in 5f272433fabd)

Code after:
		Arg(fmt.Sprintf("--sort=%s", sortOrder)).
		Arg(fmt.Sprintf("--format=%s", format)).
		Arg("refs/heads").
		ToArgv()

	return self.cmd.New(cmdArgs).DontLog().RunWithOutput()
}

var branchFields = []string{
	"HEAD",
	"refname:short",
	"upstream:short",
	"upstream:track",
	"subject",
	"objectname",
	"committerdate:unix",
}

// Obtain branch information from parsed line output of getRawBranches()
func obtainBranch(split []string, storeCommitDateAsRecency bool) *models.Branch {
	headMarker := split[0]
	fullName := split[1]
	upstreamName := split[2]
	track := split[3]
	subject := split[4]
	commitHash := split[5]
	commitDate := split[6]

	name := strings.TrimPrefix(fullName, "heads/")
	pushables, pullables, gone := parseUpstreamInfo(upstreamName, track)

	recency := ""
	if storeCommitDateAsRecency {
		if unixTimestamp, err := strconv.ParseInt(commitDate, 10, 64); err == nil {
			recency = utils.UnixToTimeAgo(unixTimestamp)
		}
	}

	return &models.Branch{
		Name:         name,
		Recency:      recency,
		Pushables:    pushables,
		Pullables:    pullables,
		UpstreamGone: gone,
		Head:         headMarker == "*",
		Subject:      subject,
		CommitHash:   commitHash,
	}
}

func parseUpstreamInfo(upstreamName string, track string) (string, string, bool) {
Repository: jesseduffield/lazygit
File: pkg/commands/git_commands/branch_loader_test.go (Go)

Code before:
package git_commands

// "*|feat/detect-purge|origin/feat/detect-purge|[ahead 1]"
import (
	"strconv"
	"testing"
	"time"

	"github.com/jesseduffield/lazygit/pkg/commands/models"
	"github.com/stretchr/testify/assert"
)

func TestObtainBranch(t *testing.T) {
	type scenario struct {
		testName       string
		input          []string
		withRecency    bool
		expectedBranch *models.Branch
	}

	now := time.Now().Unix()
	twoAndAHalfHoursAgo := strconv.Itoa(int(now - 2.5*60*60))

	scenarios := []scenario{
		{
			testName:    "TrimHeads",
			input:       []string{"", "heads/a_branch", "", "", "subject", "123"},
			withRecency: false,
			expectedBranch: &models.Branch{
				Name:       "a_branch",
				Pushables:  "?",
				Pullables:  "?",
				Head:       false,
				Subject:    "subject",
				CommitHash: "123",
			},
		},
		{
			testName:    "NoUpstream",
			input:       []string{"", "a_branch", "", "", "subject", "123"},
			withRecency: false,
			expectedBranch: &models.Branch{

Review comment (suggestion):
This should be called `storeCommitDateAsRecency`, it's one of the inputs to the function we're testing. (Changed in 5f272433fabd)

Code after:
package git_commands

// "*|feat/detect-purge|origin/feat/detect-purge|[ahead 1]"
import (
	"strconv"
	"testing"
	"time"

	"github.com/jesseduffield/lazygit/pkg/commands/models"
	"github.com/stretchr/testify/assert"
)

func TestObtainBranch(t *testing.T) {
	type scenario struct {
		testName                 string
		input                    []string
		storeCommitDateAsRecency bool
		expectedBranch           *models.Branch
	}

	// Use a time stamp of 2 1/2 hours ago, resulting in a recency string of "2h"
	now := time.Now().Unix()
	timeStamp := strconv.Itoa(int(now - 2.5*60*60))

	scenarios := []scenario{
		{
			testName:                 "TrimHeads",
			input:                    []string{"", "heads/a_branch", "", "", "subject", "123", timeStamp},
			storeCommitDateAsRecency: false,
			expectedBranch: &models.Branch{
				Name:       "a_branch",
				Pushables:  "?",
				Pullables:  "?",
				Head:       false,
				Subject:    "subject",
				CommitHash: "123",
			},
		},
		{
			testName:                 "NoUpstream",
			input:                    []string{"", "a_branch", "", "", "subject", "123", timeStamp},
			storeCommitDateAsRecency: false,
Repository: jesseduffield/lazygit
File: pkg/commands/git_commands/patch.go (Go)

Code before:
	}

	if err := self.ApplyPatch(patch, ApplyPatchOpts{Index: true, ThreeWay: true}); err != nil {
		_ = self.rebase.AbortRebase()
		return err
	}

	if err := self.commit.CommitCmdObj(commitSummary, commitDescription).Run(); err != nil {
		return err
	}

	if self.rebase.onSuccessfulContinue != nil {
		return errors.New("You are midway through another rebase operation. Please abort to start again")
	}

	self.PatchBuilder.Reset()
	return self.rebase.ContinueRebase()
}

// We have just applied a patch in reverse to discard it from a commit; if we
// now try to apply the patch again to move it to a later commit, or to the
// index, then this would conflict "with itself" in case the patch contained
// only some lines of a range of adjacent added lines. To solve this, we
// get the diff of HEAD and the original commit and then apply that.
func (self *PatchCommands) diffHeadAgainstCommit(commit *models.Commit) (string, error) {
	cmdArgs := NewGitCmd("diff").Config("diff.noprefix=false").Arg("--no-ext-diff").Arg("HEAD.." + commit.Sha).ToArgv()

	return self.cmd.New(cmdArgs).RunWithOutput()
}


Review comment (suggestion):
Formatting nitpick: we tend to format longer chains of arguments like this:
```suggestion
	cmdArgs := NewGitCmd("diff").
		Config("diff.noprefix=false").
		Arg("--no-ext-diff").
		Arg("HEAD.." + commit.Sha).
		ToArgv()
```

Code after:
	}

	if err := self.ApplyPatch(patch, ApplyPatchOpts{Index: true, ThreeWay: true}); err != nil {
		_ = self.rebase.AbortRebase()
		return err
	}

	if err := self.commit.CommitCmdObj(commitSummary, commitDescription).Run(); err != nil {
		return err
	}

	if self.rebase.onSuccessfulContinue != nil {
		return errors.New("You are midway through another rebase operation. Please abort to start again")
	}

	self.PatchBuilder.Reset()
	return self.rebase.ContinueRebase()
}

// We have just applied a patch in reverse to discard it from a commit; if we
// now try to apply the patch again to move it to a later commit, or to the
// index, then this would conflict "with itself" in case the patch contained
// only some lines of a range of adjacent added lines. To solve this, we
// get the diff of HEAD and the original commit and then apply that.
func (self *PatchCommands) diffHeadAgainstCommit(commit *models.Commit) (string, error) {
	cmdArgs := NewGitCmd("diff").
		Config("diff.noprefix=false").
		Arg("--no-ext-diff").
		Arg("HEAD.." + commit.Sha).
		ToArgv()

	return self.cmd.New(cmdArgs).RunWithOutput()
}
Repository: jesseduffield/lazygit
File: pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go (Go)

Code before:
			Press(keys.Commits.CherryPickCopy).
			Tap(func() {
				t.Views().Information().Content(Contains("1 commit copied"))
			}).
			SelectNextItem().
			Press(keys.Commits.CherryPickCopy)

		t.Views().Information().Content(Contains("2 commits copied"))

		t.Views().Commits().
			Focus().
			TopLines(
				Contains("first change"),
			).
			Press(keys.Commits.PasteCommits)

		t.ExpectPopup().Alert().
			Title(Equals("Cherry-pick")).
			Content(Contains("Are you sure you want to cherry-pick the copied commits onto this branch?")).
			Confirm()

		t.Common().AcknowledgeConflicts()
		t.Views().Information().Content(Contains("2 commits copied"))

		t.Views().Files().
			IsFocused().
			SelectedLine(Contains("file")).
			PressEnter()

		t.Views().MergeConflicts().
			IsFocused().
			// picking 'Second change'
			SelectNextItem().
			PressPrimaryAction()

		t.Common().ContinueOnConflictsResolved()

		t.Views().Files().IsEmpty()

		t.Views().Commits().
			Focus().
			TopLines(
				Contains("second-change-branch unrelated change"),
				Contains("second change"),
				Contains("first change"),
			).
			SelectNextItem().
			Tap(func() {
				// because we picked 'Second change' when resolving the conflict,
				// we now see this commit as having replaced First Change with Second Change,
				// as opposed to replacing 'Original' with 'Second change'

Review comment (suggestion):
Worth adding a comment explaining why we expect to see this i.e.:
"cherry pick selection is not cleared when there are conflicts, so that the user is able to abort and try again without having to re-copy the commits"

Code after:
			Press(keys.Commits.CherryPickCopy).
			Tap(func() {
				t.Views().Information().Content(Contains("1 commit copied"))
			}).
			SelectNextItem().
			Press(keys.Commits.CherryPickCopy)

		t.Views().Information().Content(Contains("2 commits copied"))

		t.Views().Commits().
			Focus().
			TopLines(
				Contains("first change"),
			).
			Press(keys.Commits.PasteCommits)

		t.ExpectPopup().Alert().
			Title(Equals("Cherry-pick")).
			Content(Contains("Are you sure you want to cherry-pick the copied commits onto this branch?")).
			Confirm()

		t.Common().AcknowledgeConflicts()

		// cherry pick selection is not cleared when there are conflicts, so that the user
		// is able to abort and try again without having to re-copy the commits
		t.Views().Information().Content(Contains("2 commits copied"))

		t.Views().Files().
			IsFocused().
			SelectedLine(Contains("file")).
			PressEnter()

		t.Views().MergeConflicts().
			IsFocused().
			// picking 'Second change'
			SelectNextItem().
			PressPrimaryAction()

		t.Common().ContinueOnConflictsResolved()

		t.Views().Files().IsEmpty()

		t.Views().Commits().
			Focus().
			TopLines(
				Contains("second-change-branch unrelated change"),
				Contains("second change"),
				Contains("first change"),
			).
			SelectNextItem().
			Tap(func() {