From 9a485ddfa278ee85e61ebb59f5c40a6c3d8fb888 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 5 Aug 2021 20:42:52 +0200 Subject: [PATCH] :nail_care: Cleanup local branch handling during `pr checkout` --- pkg/cmd/pr/checkout/checkout.go | 41 +++++++++++----------------- pkg/cmd/pr/checkout/checkout_test.go | 33 ++++++++-------------- 2 files changed, 28 insertions(+), 46 deletions(-) diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index 5d45856cd..651a7b3f4 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -167,12 +167,7 @@ func cmdsForExistingRemote(remote *context.Remote, pr *api.PullRequest, opts *Ch cmds = append(cmds, []string{"git", "merge", "--ff-only", fmt.Sprintf("refs/remotes/%s", remoteBranch)}) } default: - cmds = append( - cmds, - []string{"git", "checkout", "-b", localBranch, "--no-track", remoteBranch}, - []string{"git", "config", fmt.Sprintf("branch.%s.remote", localBranch), remote.Name}, - []string{"git", "config", fmt.Sprintf("branch.%s.merge", pr.HeadRefName), "refs/heads/" + pr.HeadRefName}, - ) + cmds = append(cmds, []string{"git", "checkout", "-b", localBranch, "--track", remoteBranch}) } return cmds @@ -180,13 +175,6 @@ func cmdsForExistingRemote(remote *context.Remote, pr *api.PullRequest, opts *Ch func cmdsForMissingRemote(pr *api.PullRequest, baseURLOrName, repoHost, defaultBranch, protocol string, opts *CheckoutOptions) [][]string { var cmds [][]string - - newBranchName := pr.HeadRefName - // avoid naming the new branch the same as the default branch - if newBranchName == defaultBranch { - newBranchName = fmt.Sprintf("%s/%s", pr.HeadRepositoryOwner.Login, newBranchName) - } - ref := fmt.Sprintf("refs/pull/%d/head", pr.Number) if opts.Detach { @@ -195,8 +183,16 @@ func cmdsForMissingRemote(pr *api.PullRequest, baseURLOrName, repoHost, defaultB return cmds } + localBranch := pr.HeadRefName + if opts.BranchName != "" { + localBranch = opts.BranchName + } else if pr.HeadRefName == defaultBranch { + // avoid naming the new branch the same as the default branch + localBranch = fmt.Sprintf("%s/%s", pr.HeadRepositoryOwner.Login, localBranch) + } + currentBranch, _ := opts.Branch() - if newBranchName == currentBranch { + if localBranch == currentBranch { // PR head matches currently checked out branch cmds = append(cmds, []string{"git", "fetch", baseURLOrName, ref}) if opts.Force { @@ -206,19 +202,14 @@ func cmdsForMissingRemote(pr *api.PullRequest, baseURLOrName, repoHost, defaultB cmds = append(cmds, []string{"git", "merge", "--ff-only", "FETCH_HEAD"}) } } else { - // create a new branch if opts.Force { - cmds = append(cmds, []string{"git", "fetch", baseURLOrName, fmt.Sprintf("%s:%s", ref, newBranchName), "--force"}) + cmds = append(cmds, []string{"git", "fetch", baseURLOrName, fmt.Sprintf("%s:%s", ref, localBranch), "--force"}) } else { // TODO: check if non-fast-forward and suggest to use `--force` - cmds = append(cmds, []string{"git", "fetch", baseURLOrName, fmt.Sprintf("%s:%s", ref, newBranchName)}) + cmds = append(cmds, []string{"git", "fetch", baseURLOrName, fmt.Sprintf("%s:%s", ref, localBranch)}) } - if opts.BranchName != "" { - cmds = append(cmds, []string{"git", "checkout", "-b", opts.BranchName, "--track", newBranchName}) - } else { - cmds = append(cmds, []string{"git", "checkout", newBranchName}) - } + cmds = append(cmds, []string{"git", "checkout", localBranch}) } remote := baseURLOrName @@ -228,9 +219,9 @@ func cmdsForMissingRemote(pr *api.PullRequest, baseURLOrName, repoHost, defaultB remote = ghrepo.FormatRemoteURL(headRepo, protocol) mergeRef = fmt.Sprintf("refs/heads/%s", pr.HeadRefName) } - if missingMergeConfigForBranch(newBranchName) { - cmds = append(cmds, []string{"git", "config", fmt.Sprintf("branch.%s.remote", newBranchName), remote}) - cmds = append(cmds, []string{"git", "config", fmt.Sprintf("branch.%s.merge", newBranchName), mergeRef}) + if missingMergeConfigForBranch(localBranch) { + cmds = append(cmds, []string{"git", "config", fmt.Sprintf("branch.%s.remote", localBranch), remote}) + cmds = append(cmds, []string{"git", "config", fmt.Sprintf("branch.%s.merge", localBranch), mergeRef}) } return cmds diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index 73e7141b6..7386cf2fc 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -101,14 +101,12 @@ func Test_checkoutRun(t *testing.T) { }, }, { - name: "with local branch rename", + name: "with local branch rename and existing git remote", opts: &CheckoutOptions{ SelectorArg: "123", BranchName: "foobar", Finder: func() shared.PRFinder { - baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") - pr.MaintainerCanModify = true - pr.HeadRepository = nil + baseRepo, pr := stubPR("OWNER/REPO:master", "OWNER/REPO:feature") finder := shared.NewMockFinder("123", pr, baseRepo) return finder }(), @@ -123,11 +121,9 @@ func Test_checkoutRun(t *testing.T) { "origin": "OWNER/REPO", }, runStubs: func(cs *run.CommandStubber) { - cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") - cs.Register(`git config branch\.feature\.merge`, 1, "") - cs.Register(`git checkout -b foobar --track feature`, 0, "") - cs.Register(`git config branch\.feature\.remote origin`, 0, "") - cs.Register(`git config branch\.feature\.merge refs/pull/123/head`, 0, "") + cs.Register(`git show-ref --verify -- refs/heads/foobar`, 1, "") + cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") + cs.Register(`git checkout -b foobar --track origin/feature`, 0, "") }, }, { @@ -138,7 +134,6 @@ func Test_checkoutRun(t *testing.T) { Finder: func() shared.PRFinder { baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") pr.MaintainerCanModify = true - pr.HeadRepository = nil finder := shared.NewMockFinder("123", pr, baseRepo) return finder }(), @@ -153,11 +148,11 @@ func Test_checkoutRun(t *testing.T) { "origin": "OWNER/REPO", }, runStubs: func(cs *run.CommandStubber) { - cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") - cs.Register(`git config branch\.feature\.merge`, 1, "") - cs.Register(`git checkout -b foobar --track feature`, 0, "") - cs.Register(`git config branch\.feature\.remote origin`, 0, "") - cs.Register(`git config branch\.feature\.merge refs/pull/123/head`, 0, "") + cs.Register(`git config branch\.foobar\.merge`, 1, "") + cs.Register(`git fetch origin refs/pull/123/head:foobar`, 0, "") + cs.Register(`git checkout foobar`, 0, "") + cs.Register(`git config branch\.foobar\.remote https://github.com/hubot/REPO.git`, 0, "") + cs.Register(`git config branch\.foobar\.merge refs/heads/feature`, 0, "") }, }, } @@ -272,9 +267,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") - cs.Register(`git checkout -b feature --no-track origin/feature`, 0, "") - cs.Register(`git config branch\.feature\.remote origin`, 0, "") - cs.Register(`git config branch\.feature\.merge refs/heads/feature`, 0, "") + cs.Register(`git checkout -b feature --track origin/feature`, 0, "") output, err := runCommand(http, nil, "master", `123`) assert.NoError(t, err) @@ -327,9 +320,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { cs.Register(`git fetch robot-fork \+refs/heads/feature:refs/remotes/robot-fork/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") - cs.Register(`git checkout -b feature --no-track robot-fork/feature`, 0, "") - cs.Register(`git config branch\.feature\.remote robot-fork`, 0, "") - cs.Register(`git config branch\.feature\.merge refs/heads/feature`, 0, "") + cs.Register(`git checkout -b feature --track robot-fork/feature`, 0, "") output, err := runCommand(http, remotes, "master", `123`) assert.NoError(t, err)