Change GetCommitsOptions.RefForPushedStatus to a models.Ref

This makes it easier to use the full ref in the git merge-base call, which
avoids ambiguities when there's a tag with the same name as the current branch.

This fixes a hash coloring bug in the local commits panel when there's a tag
with the same name as the checked out branch; in this case all commit hashes
that should be yellow were painted as red.

pkg/commands/git_commands/commit_loader.go

@@ -60,7 +60,7 @@ type GetCommitsOptions struct {
 	FilterAuthor         string
 	IncludeRebaseCommits bool
 	RefName              string     // e.g. "HEAD" or "my_branch"
-	RefForPushedStatus   string // the ref to use for determining pushed/unpushed status
+	RefForPushedStatus   models.Ref // the ref to use for determining pushed/unpushed status
 	// determines if we show the whole git graph i.e. pass the '--all' flag
 	All bool
 	// If non-empty, show divergence from this ref (left-right log)
@@ -112,7 +112,7 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit,
 	passedFirstPushedCommit := false
 	// I can get this before
 	firstPushedCommit, err := self.getFirstPushedCommit(opts.RefForPushedStatus)
-	if err != nil {
+	if err != nil || firstPushedCommit == "" {
 		// must have no upstream branch so we'll consider everything as pushed
 		passedFirstPushedCommit = true
 	}
@@ -581,11 +581,15 @@ func ignoringWarnings(commandOutput string) string {
 
 // getFirstPushedCommit returns the first commit hash which has been pushed to the ref's upstream.
 // all commits above this are deemed unpushed and marked as such.
-func (self *CommitLoader) getFirstPushedCommit(refName string) (string, error) {
+func (self *CommitLoader) getFirstPushedCommit(ref models.Ref) (string, error) {
+	if ref == nil {
+		return "", nil
+	}
+
 	output, err := self.cmd.New(
 		NewGitCmd("merge-base").
-			Arg(refName).
+			Arg(ref.FullRefName()).
-			Arg(strings.TrimPrefix(refName, "refs/heads/") + "@{u}").
+			Arg(ref.RefName() + "@{u}").
 			ToArgv(),
 	).
 		DontLog().

pkg/commands/git_commands/commit_loader_test.go

@@ -41,9 +41,9 @@ func TestGetCommits(t *testing.T) {
 		{
 			testName: "should return no commits if there are none",
 			logOrder: "topo-order",
-			opts:     GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
+			opts:     GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
 			runner: oscommands.NewFakeRunner(t).
-				ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
+				ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
 				ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil),
 
 			expectedCommitOpts: []models.NewCommitOpts{},
@@ -52,7 +52,7 @@ func TestGetCommits(t *testing.T) {
 		{
 			testName: "should use proper upstream name for branch",
 			logOrder: "topo-order",
-			opts:     GetCommitsOptions{RefName: "refs/heads/mybranch", RefForPushedStatus: "refs/heads/mybranch", IncludeRebaseCommits: false},
+			opts:     GetCommitsOptions{RefName: "refs/heads/mybranch", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
 			runner: oscommands.NewFakeRunner(t).
 				ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
 				ExpectGitArgs([]string{"log", "refs/heads/mybranch", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil),
@@ -63,11 +63,11 @@ func TestGetCommits(t *testing.T) {
 		{
 			testName:     "should return commits if they are present",
 			logOrder:     "topo-order",
-			opts:         GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
+			opts:         GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
 			mainBranches: []string{"master", "main", "develop"},
 			runner: oscommands.NewFakeRunner(t).
 				// here it's seeing which commits are yet to be pushed
-				ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
+				ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
 				// here it's actually getting all the commits in a formatted form, one per line
 				ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, commitsOutput, nil).
 				// here it's testing which of the configured main branches have an upstream
@@ -199,11 +199,11 @@ func TestGetCommits(t *testing.T) {
 		{
 			testName:     "should not call merge-base for mainBranches if none exist",
 			logOrder:     "topo-order",
-			opts:         GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
+			opts:         GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
 			mainBranches: []string{"master", "main"},
 			runner: oscommands.NewFakeRunner(t).
 				// here it's seeing which commits are yet to be pushed
-				ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
+				ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
 				// here it's actually getting all the commits in a formatted form, one per line
 				ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil).
 				// here it's testing which of the configured main branches exist; neither does
@@ -235,11 +235,11 @@ func TestGetCommits(t *testing.T) {
 		{
 			testName:     "should call merge-base for all main branches that exist",
 			logOrder:     "topo-order",
-			opts:         GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
+			opts:         GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
 			mainBranches: []string{"master", "main", "develop", "1.0-hotfixes"},
 			runner: oscommands.NewFakeRunner(t).
 				// here it's seeing which commits are yet to be pushed
-				ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
+				ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
 				// here it's actually getting all the commits in a formatted form, one per line
 				ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil).
 				// here it's testing which of the configured main branches exist
@@ -273,9 +273,9 @@ func TestGetCommits(t *testing.T) {
 		{
 			testName: "should not specify order if `log.order` is `default`",
 			logOrder: "default",
-			opts:     GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
+			opts:     GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
 			runner: oscommands.NewFakeRunner(t).
-				ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
+				ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
 				ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil),
 
 			expectedCommitOpts: []models.NewCommitOpts{},
@@ -284,9 +284,9 @@ func TestGetCommits(t *testing.T) {
 		{
 			testName: "should set filter path",
 			logOrder: "default",
-			opts:     GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", FilterPath: "src"},
+			opts:     GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, FilterPath: "src"},
 			runner: oscommands.NewFakeRunner(t).
-				ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
+				ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
 				ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--follow", "--name-status", "--no-show-signature", "--", "src"}, "", nil),
 
 			expectedCommitOpts: []models.NewCommitOpts{},

pkg/gui/controllers/helpers/refresh_helper.go

@@ -288,37 +288,37 @@ func (self *RefreshHelper) refreshCommitsAndCommitFiles() {
 	}
 }
 
-func (self *RefreshHelper) determineCheckedOutBranchName() string {
+func (self *RefreshHelper) determineCheckedOutRef() models.Ref {
 	if rebasedBranch := self.c.Git().Status.BranchBeingRebased(); rebasedBranch != "" {
 		// During a rebase we're on a detached head, so cannot determine the
 		// branch name in the usual way. We need to read it from the
 		// ".git/rebase-merge/head-name" file instead.
-		return strings.TrimPrefix(rebasedBranch, "refs/heads/")
+		return &models.Branch{Name: strings.TrimPrefix(rebasedBranch, "refs/heads/")}
 	}
 
 	if bisectInfo := self.c.Git().Bisect.GetInfo(); bisectInfo.Bisecting() && bisectInfo.GetStartHash() != "" {
 		// Likewise, when we're bisecting we're on a detached head as well. In
 		// this case we read the branch name from the ".git/BISECT_START" file.
-		return bisectInfo.GetStartHash()
+		return &models.Branch{Name: bisectInfo.GetStartHash()}
 	}
 
 	// In all other cases, get the branch name by asking git what branch is
 	// checked out. Note that if we're on a detached head (for reasons other
 	// than rebasing or bisecting, i.e. it was explicitly checked out), then
 	// this will return an empty string.
-	if branchName, err := self.c.Git().Branch.CurrentBranchName(); err == nil {
+	if branchName, err := self.c.Git().Branch.CurrentBranchName(); err == nil && branchName != "" {
-		return branchName
+		return &models.Branch{Name: branchName}
 	}
 
 	// Should never get here unless the working copy is corrupt
-	return ""
+	return nil
 }
 
 func (self *RefreshHelper) refreshCommitsWithLimit() error {
 	self.c.Mutexes().LocalCommitsMutex.Lock()
 	defer self.c.Mutexes().LocalCommitsMutex.Unlock()
 
-	checkedOutBranchName := self.determineCheckedOutBranchName()
+	checkedOutRef := self.determineCheckedOutRef()
 	commits, err := self.c.Git().Loaders.CommitLoader.GetCommits(
 		git_commands.GetCommitsOptions{
 			Limit:                self.c.Contexts().LocalCommits.GetLimitCommits(),
@@ -326,7 +326,7 @@ func (self *RefreshHelper) refreshCommitsWithLimit() error {
 			FilterAuthor:         self.c.Modes().Filtering.GetAuthor(),
 			IncludeRebaseCommits: true,
 			RefName:              self.refForLog(),
-			RefForPushedStatus:   checkedOutBranchName,
+			RefForPushedStatus:   checkedOutRef,
 			All:                  self.c.Contexts().LocalCommits.GetShowWholeGitGraph(),
 			MainBranches:         self.c.Model().MainBranches,
 			HashPool:             self.c.Model().HashPool,
@@ -338,7 +338,11 @@ func (self *RefreshHelper) refreshCommitsWithLimit() error {
 	self.c.Model().Commits = commits
 	self.RefreshAuthors(commits)
 	self.c.Model().WorkingTreeStateAtLastCommitRefresh = self.c.Git().Status.WorkingTreeState()
-	self.c.Model().CheckedOutBranch = checkedOutBranchName
+	if checkedOutRef != nil {
+		self.c.Model().CheckedOutBranch = checkedOutRef.RefName()
+	} else {
+		self.c.Model().CheckedOutBranch = ""
+	}
 
 	self.refreshView(self.c.Contexts().LocalCommits)
 	return nil
@@ -360,7 +364,7 @@ func (self *RefreshHelper) refreshSubCommitsWithLimit() error {
 			IncludeRebaseCommits:    false,
 			RefName:                 self.c.Contexts().SubCommits.GetRef().FullRefName(),
 			RefToShowDivergenceFrom: self.c.Contexts().SubCommits.GetRefToShowDivergenceFrom(),
-			RefForPushedStatus:      self.c.Contexts().SubCommits.GetRef().FullRefName(),
+			RefForPushedStatus:      self.c.Contexts().SubCommits.GetRef(),
 			MainBranches:            self.c.Model().MainBranches,
 			HashPool:                self.c.Model().HashPool,
 		},

pkg/gui/controllers/helpers/sub_commits_helper.go

@@ -39,7 +39,7 @@ func (self *SubCommitsHelper) ViewSubCommits(opts ViewSubCommitsOpts) error {
 			FilterAuthor:            self.c.Modes().Filtering.GetAuthor(),
 			IncludeRebaseCommits:    false,
 			RefName:                 opts.Ref.FullRefName(),
-			RefForPushedStatus:      opts.Ref.FullRefName(),
+			RefForPushedStatus:      opts.Ref,
 			RefToShowDivergenceFrom: opts.RefToShowDivergenceFrom,
 			MainBranches:            self.c.Model().MainBranches,
 			HashPool:                self.c.Model().HashPool,