From bac12231c3e4e473005267a29a299d13efabfa15 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Thu, 5 Oct 2023 18:17:22 +1100 Subject: [PATCH] Don't bother checking for origin/main when loading commits The real reason I'm doing this is to that it's easier to explain the logic behind setting commit merged statuses. Now it's just 'try the upstream, fallback to the local branch'. Other justifications: * we should avoid assuming that 'origin' is the main upstream remote * it's one less git call * surely the main branch _will_ have an upsream. I doubt many people have benefitted from the extra fallback to origin --- pkg/commands/git_commands/commit_loader.go | 16 ++-------------- pkg/commands/git_commands/commit_loader_test.go | 13 ++++--------- 2 files changed, 6 insertions(+), 23 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 4b0a23692..14a6c5449 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -589,20 +589,8 @@ func (self *CommitLoader) getExistingMainBranches() []string { return } - // If this failed, a local branch for this main branch doesn't exist or it - // has no upstream configured. Try looking for one in the "origin" remote. - ref := "refs/remotes/origin/" + branchName - if err := self.cmd.New( - NewGitCmd("rev-parse").Arg("--verify", "--quiet", ref).ToArgv(), - ).DontLog().Run(); err == nil { - existingBranches[i] = ref - return - } - - // If this failed as well, try if we have the main branch as a local - // branch. This covers the case where somebody is using git locally - // for something, but never pushing anywhere. - ref = "refs/heads/" + branchName + // If this failed, fallback to the local branch + ref := "refs/heads/" + branchName if err := self.cmd.New( NewGitCmd("rev-parse").Arg("--verify", "--quiet", ref).ToArgv(), ).DontLog().Run(); err == nil { diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index 3f8fbd58e..7ae514e94 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -74,12 +74,10 @@ func TestGetCommits(t *testing.T) { // 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%D%x00%p%x00%s%x00%m", "--abbrev=40", "--no-show-signature", "--"}, commitsOutput, nil). // here it's testing which of the configured main branches have an upstream - ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "refs/remotes/origin/master", nil). // this one does - ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "main@{u}"}, "", errors.New("error")). // this one doesn't, so it checks origin instead - ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/remotes/origin/main"}, "", nil). // yep, origin/main exists - ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "develop@{u}"}, "", errors.New("error")). // this one doesn't, so it checks origin instead - ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/remotes/origin/develop"}, "", errors.New("error")). // doesn't exist there, either, so it checks for a local branch - ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/heads/develop"}, "", errors.New("error")). // no local branch either + ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "refs/remotes/origin/master", nil). // this one does + ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "main@{u}"}, "refs/remotes/origin/main", nil). // this one does too + ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "develop@{u}"}, "", errors.New("error")). // this one doesn't, so check for a local branch + ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/heads/develop"}, "", errors.New("error")). // no local branch either // here it's seeing where our branch diverged from the master branch so that we can mark that commit and parent commits as 'merged' ExpectGitArgs([]string{"merge-base", "HEAD", "refs/remotes/origin/master", "refs/remotes/origin/main"}, "26c07b1ab33860a1a7591a0638f9925ccf497ffa", nil), @@ -212,10 +210,8 @@ func TestGetCommits(t *testing.T) { ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%p%x00%s%x00%m", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). // here it's testing which of the configured main branches exist; neither does ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "", errors.New("error")). - ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/remotes/origin/master"}, "", errors.New("error")). ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/heads/master"}, "", errors.New("error")). ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "main@{u}"}, "", errors.New("error")). - ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/remotes/origin/main"}, "", errors.New("error")). ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/heads/main"}, "", errors.New("error")), expectedCommits: []*models.Commit{ @@ -250,7 +246,6 @@ func TestGetCommits(t *testing.T) { // here it's testing which of the configured main branches exist ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "refs/remotes/origin/master", nil). ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "main@{u}"}, "", errors.New("error")). - ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/remotes/origin/main"}, "", errors.New("error")). ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/heads/main"}, "", errors.New("error")). ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "develop@{u}"}, "refs/remotes/origin/develop", nil). ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "1.0-hotfixes@{u}"}, "refs/remotes/origin/1.0-hotfixes", nil).