Skip to content

Commit 2051733

Browse files
committed
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.
1 parent 4b9921d commit 2051733

File tree

4 files changed

+38
-30
lines changed

4 files changed

+38
-30
lines changed

pkg/commands/git_commands/commit_loader.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ type GetCommitsOptions struct {
5959
FilterPath string
6060
FilterAuthor string
6161
IncludeRebaseCommits bool
62-
RefName string // e.g. "HEAD" or "my_branch"
63-
RefForPushedStatus string // the ref to use for determining pushed/unpushed status
62+
RefName string // e.g. "HEAD" or "my_branch"
63+
RefForPushedStatus models.Ref // the ref to use for determining pushed/unpushed status
6464
// determines if we show the whole git graph i.e. pass the '--all' flag
6565
All bool
6666
// If non-empty, show divergence from this ref (left-right log)
@@ -112,7 +112,7 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit,
112112
passedFirstPushedCommit := false
113113
// I can get this before
114114
firstPushedCommit, err := self.getFirstPushedCommit(opts.RefForPushedStatus)
115-
if err != nil {
115+
if err != nil || firstPushedCommit == "" {
116116
// must have no upstream branch so we'll consider everything as pushed
117117
passedFirstPushedCommit = true
118118
}
@@ -581,11 +581,15 @@ func ignoringWarnings(commandOutput string) string {
581581

582582
// getFirstPushedCommit returns the first commit hash which has been pushed to the ref's upstream.
583583
// all commits above this are deemed unpushed and marked as such.
584-
func (self *CommitLoader) getFirstPushedCommit(refName string) (string, error) {
584+
func (self *CommitLoader) getFirstPushedCommit(ref models.Ref) (string, error) {
585+
if ref == nil {
586+
return "", nil
587+
}
588+
585589
output, err := self.cmd.New(
586590
NewGitCmd("merge-base").
587-
Arg(refName).
588-
Arg(strings.TrimPrefix(refName, "refs/heads/") + "@{u}").
591+
Arg(ref.FullRefName()).
592+
Arg(ref.RefName() + "@{u}").
589593
ToArgv(),
590594
).
591595
DontLog().

pkg/commands/git_commands/commit_loader_test.go

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,9 @@ func TestGetCommits(t *testing.T) {
4141
{
4242
testName: "should return no commits if there are none",
4343
logOrder: "topo-order",
44-
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
44+
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
4545
runner: oscommands.NewFakeRunner(t).
46-
ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
46+
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
4747
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),
4848

4949
expectedCommitOpts: []models.NewCommitOpts{},
@@ -52,7 +52,7 @@ func TestGetCommits(t *testing.T) {
5252
{
5353
testName: "should use proper upstream name for branch",
5454
logOrder: "topo-order",
55-
opts: GetCommitsOptions{RefName: "refs/heads/mybranch", RefForPushedStatus: "refs/heads/mybranch", IncludeRebaseCommits: false},
55+
opts: GetCommitsOptions{RefName: "refs/heads/mybranch", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
5656
runner: oscommands.NewFakeRunner(t).
5757
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
5858
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) {
6363
{
6464
testName: "should return commits if they are present",
6565
logOrder: "topo-order",
66-
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
66+
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
6767
mainBranches: []string{"master", "main", "develop"},
6868
runner: oscommands.NewFakeRunner(t).
6969
// here it's seeing which commits are yet to be pushed
70-
ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
70+
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
7171
// here it's actually getting all the commits in a formatted form, one per line
7272
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).
7373
// here it's testing which of the configured main branches have an upstream
@@ -199,11 +199,11 @@ func TestGetCommits(t *testing.T) {
199199
{
200200
testName: "should not call merge-base for mainBranches if none exist",
201201
logOrder: "topo-order",
202-
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
202+
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
203203
mainBranches: []string{"master", "main"},
204204
runner: oscommands.NewFakeRunner(t).
205205
// here it's seeing which commits are yet to be pushed
206-
ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
206+
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
207207
// here it's actually getting all the commits in a formatted form, one per line
208208
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).
209209
// here it's testing which of the configured main branches exist; neither does
@@ -235,11 +235,11 @@ func TestGetCommits(t *testing.T) {
235235
{
236236
testName: "should call merge-base for all main branches that exist",
237237
logOrder: "topo-order",
238-
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
238+
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
239239
mainBranches: []string{"master", "main", "develop", "1.0-hotfixes"},
240240
runner: oscommands.NewFakeRunner(t).
241241
// here it's seeing which commits are yet to be pushed
242-
ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
242+
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
243243
// here it's actually getting all the commits in a formatted form, one per line
244244
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).
245245
// here it's testing which of the configured main branches exist
@@ -273,9 +273,9 @@ func TestGetCommits(t *testing.T) {
273273
{
274274
testName: "should not specify order if `log.order` is `default`",
275275
logOrder: "default",
276-
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false},
276+
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
277277
runner: oscommands.NewFakeRunner(t).
278-
ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
278+
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
279279
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),
280280

281281
expectedCommitOpts: []models.NewCommitOpts{},
@@ -284,9 +284,9 @@ func TestGetCommits(t *testing.T) {
284284
{
285285
testName: "should set filter path",
286286
logOrder: "default",
287-
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", FilterPath: "src"},
287+
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, FilterPath: "src"},
288288
runner: oscommands.NewFakeRunner(t).
289-
ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
289+
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
290290
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),
291291

292292
expectedCommitOpts: []models.NewCommitOpts{},

pkg/gui/controllers/helpers/refresh_helper.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -288,45 +288,45 @@ func (self *RefreshHelper) refreshCommitsAndCommitFiles() {
288288
}
289289
}
290290

291-
func (self *RefreshHelper) determineCheckedOutBranchName() string {
291+
func (self *RefreshHelper) determineCheckedOutRef() models.Ref {
292292
if rebasedBranch := self.c.Git().Status.BranchBeingRebased(); rebasedBranch != "" {
293293
// During a rebase we're on a detached head, so cannot determine the
294294
// branch name in the usual way. We need to read it from the
295295
// ".git/rebase-merge/head-name" file instead.
296-
return strings.TrimPrefix(rebasedBranch, "refs/heads/")
296+
return &models.Branch{Name: strings.TrimPrefix(rebasedBranch, "refs/heads/")}
297297
}
298298

299299
if bisectInfo := self.c.Git().Bisect.GetInfo(); bisectInfo.Bisecting() && bisectInfo.GetStartHash() != "" {
300300
// Likewise, when we're bisecting we're on a detached head as well. In
301301
// this case we read the branch name from the ".git/BISECT_START" file.
302-
return bisectInfo.GetStartHash()
302+
return &models.Branch{Name: bisectInfo.GetStartHash()}
303303
}
304304

305305
// In all other cases, get the branch name by asking git what branch is
306306
// checked out. Note that if we're on a detached head (for reasons other
307307
// than rebasing or bisecting, i.e. it was explicitly checked out), then
308308
// this will return an empty string.
309-
if branchName, err := self.c.Git().Branch.CurrentBranchName(); err == nil {
310-
return branchName
309+
if branchName, err := self.c.Git().Branch.CurrentBranchName(); err == nil && branchName != "" {
310+
return &models.Branch{Name: branchName}
311311
}
312312

313313
// Should never get here unless the working copy is corrupt
314-
return ""
314+
return nil
315315
}
316316

317317
func (self *RefreshHelper) refreshCommitsWithLimit() error {
318318
self.c.Mutexes().LocalCommitsMutex.Lock()
319319
defer self.c.Mutexes().LocalCommitsMutex.Unlock()
320320

321-
checkedOutBranchName := self.determineCheckedOutBranchName()
321+
checkedOutRef := self.determineCheckedOutRef()
322322
commits, err := self.c.Git().Loaders.CommitLoader.GetCommits(
323323
git_commands.GetCommitsOptions{
324324
Limit: self.c.Contexts().LocalCommits.GetLimitCommits(),
325325
FilterPath: self.c.Modes().Filtering.GetPath(),
326326
FilterAuthor: self.c.Modes().Filtering.GetAuthor(),
327327
IncludeRebaseCommits: true,
328328
RefName: self.refForLog(),
329-
RefForPushedStatus: checkedOutBranchName,
329+
RefForPushedStatus: checkedOutRef,
330330
All: self.c.Contexts().LocalCommits.GetShowWholeGitGraph(),
331331
MainBranches: self.c.Model().MainBranches,
332332
HashPool: self.c.Model().HashPool,
@@ -338,7 +338,11 @@ func (self *RefreshHelper) refreshCommitsWithLimit() error {
338338
self.c.Model().Commits = commits
339339
self.RefreshAuthors(commits)
340340
self.c.Model().WorkingTreeStateAtLastCommitRefresh = self.c.Git().Status.WorkingTreeState()
341-
self.c.Model().CheckedOutBranch = checkedOutBranchName
341+
if checkedOutRef != nil {
342+
self.c.Model().CheckedOutBranch = checkedOutRef.RefName()
343+
} else {
344+
self.c.Model().CheckedOutBranch = ""
345+
}
342346

343347
self.refreshView(self.c.Contexts().LocalCommits)
344348
return nil
@@ -360,7 +364,7 @@ func (self *RefreshHelper) refreshSubCommitsWithLimit() error {
360364
IncludeRebaseCommits: false,
361365
RefName: self.c.Contexts().SubCommits.GetRef().FullRefName(),
362366
RefToShowDivergenceFrom: self.c.Contexts().SubCommits.GetRefToShowDivergenceFrom(),
363-
RefForPushedStatus: self.c.Contexts().SubCommits.GetRef().FullRefName(),
367+
RefForPushedStatus: self.c.Contexts().SubCommits.GetRef(),
364368
MainBranches: self.c.Model().MainBranches,
365369
HashPool: self.c.Model().HashPool,
366370
},

pkg/gui/controllers/helpers/sub_commits_helper.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func (self *SubCommitsHelper) ViewSubCommits(opts ViewSubCommitsOpts) error {
3939
FilterAuthor: self.c.Modes().Filtering.GetAuthor(),
4040
IncludeRebaseCommits: false,
4141
RefName: opts.Ref.FullRefName(),
42-
RefForPushedStatus: opts.Ref.FullRefName(),
42+
RefForPushedStatus: opts.Ref,
4343
RefToShowDivergenceFrom: opts.RefToShowDivergenceFrom,
4444
MainBranches: self.c.Model().MainBranches,
4545
HashPool: self.c.Model().HashPool,

0 commit comments

Comments
 (0)