Conversation
billygriffin
left a comment
There was a problem hiding this comment.
Thanks! I'm wondering whether --local is the best verbiage here, and not something like --branch-name and -b. Anyone have thoughts on that? -b feels more consistent with what you'd expect using git checkout -b
|
|
mislav
left a comment
There was a problem hiding this comment.
Thanks for working on this!
In addition to my line comments, an important note: it seems that you have only added this functionality to cmdsForExistingRemote. However, if --branch flag was to work in all cases, then you will have to edit cmdsForMissingRemote. An existing git remote will not always be located for a PR; in fact, cmdsForMissingRemote is more common for PRs to open source repos that come from forks.
mislav
left a comment
There was a problem hiding this comment.
Thanks for the fast update!
Could there also be an added test for the case when opts.BranchName is set, but there is no existing git remote that matches the head repository of the PR?
pkg/cmd/pr/checkout/checkout.go
Outdated
| cmds = append(cmds, []string{"git", "checkout", newBranchName}) | ||
|
|
||
| if opts.BranchName != "" { | ||
| cmds = append(cmds, []string{"git", "checkout", "-b", opts.BranchName, "--no-track", newBranchName}) |
There was a problem hiding this comment.
We should use --track instead of --no-track here, because --track is implied by git checkout if --branch wasn't specified (the else block in this conditional).
There was a problem hiding this comment.
hm. Regarding the new test, I'm not sure how to write that. Using what exists now, I thought I could just remove everything in remotes but the tests panic with what seems like an unrelated error.
{
name: "with local branch rename and no 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
finder := shared.NewMockFinder("123", pr, baseRepo)
return finder
}(),
Config: func() (config.Config, error) {
return config.NewBlankConfig(), nil
},
Branch: func() (string, error) {
return "main", nil
},
},
remotes: map[string]string{},
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 --no-track feature`, 0, "")
cs.Register(`git config branch\.feature\.remote origin`, 0, "")
cs.Register(`git config branch\.feature\.merge refs/pull/123/head`, 0, "")
},
},Allows renaming the checked out branch.
72665d3 to
9a485dd
Compare
mislav
left a comment
There was a problem hiding this comment.
I'm sorry that the tests were tricky to make— this is particularly hairy commands with lots of hard-to-track test stubs. Thank you for your hard work! I've pushed some tweaks.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Allows renaming the checked out branch.
Closes #835