Skip to content

Commit

Permalink
Prompt for push target during pr create
Browse files Browse the repository at this point in the history 
We no longer guess the head repository using heuristics; instead, we
present the user with the choice of pushable repositories and an
additional option to create a new fork.

The new `pr create --head` flag is available for the user to specify the
head branch in `branch` or `owner:branch` format and completely skip any
forking or auto-pushing checks.
  • Loading branch information
@mislav
mislav committed Sep 16, 2020
1 parent d534a94 commit 7a8db80
Show file tree
Hide file tree
Showing 5 changed files with 269 additions and 801 deletions.
29 changes: 18 additions & 11 deletions api/queries_repo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) {
query RepositoryInfo($owner: String!, $name: String!) {
repository(owner: $owner, name: $name) {
id
name
owner { login }
hasIssuesEnabled
description
viewerPermission
Expand Down Expand Up @@ -317,8 +319,8 @@ func ForkRepo(client *Client, repo ghrepo.Interface) (*Repository, error) {
}, nil
}

// RepoFindFork finds a fork of repo affiliated with the viewer
func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) {
// RepoFindForks finds forks of the repo that are affiliated with the viewer
func RepoFindForks(client *Client, repo ghrepo.Interface, limit int) ([]*Repository, error) {
result := struct {
Repository struct {
Forks struct {
Expand All @@ -330,12 +332,13 @@ func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) {
variables := map[string]interface{}{
"owner": repo.RepoOwner(),
"repo": repo.RepoName(),
"limit": limit,
}

if err := client.GraphQL(repo.RepoHost(), `
query RepositoryFindFork($owner: String!, $repo: String!) {
query RepositoryFindFork($owner: String!, $repo: String!, $limit: Int!) {
repository(owner: $owner, name: $repo) {
forks(first: 1, affiliations: [OWNER, COLLABORATOR]) {
forks(first: $limit, affiliations: [OWNER, COLLABORATOR]) {
nodes {
id
name
Expand All @@ -350,14 +353,18 @@ func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) {
return nil, err
}

forks := result.Repository.Forks.Nodes
// we check ViewerCanPush, even though we expect it to always be true per
// `affiliations` condition, to guard against versions of GitHub with a
// faulty `affiliations` implementation
if len(forks) > 0 && forks[0].ViewerCanPush() {
return InitRepoHostname(&forks[0], repo.RepoHost()), nil
var results []*Repository
for _, r := range result.Repository.Forks.Nodes {
// we check ViewerCanPush, even though we expect it to always be true per
// `affiliations` condition, to guard against versions of GitHub with a
// faulty `affiliations` implementation
if !r.ViewerCanPush() {
continue
}
results = append(results, InitRepoHostname(&r, repo.RepoHost()))
}
return nil, &NotFoundError{errors.New("no fork found")}

return results, nil
}

type RepoMetadataResult struct {
Expand Down
23 changes: 4 additions & 19 deletions context/context.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,36 +139,21 @@ func (r *ResolvedRemotes) BaseRepo(io *iostreams.IOStreams) (ghrepo.Interface, e
return selectedRepo, err
}

func (r *ResolvedRemotes) HeadRepo(baseRepo ghrepo.Interface) (ghrepo.Interface, error) {
func (r *ResolvedRemotes) HeadRepos() ([]*api.Repository, error) {
if r.network == nil {
err := resolveNetwork(r)
if err != nil {
return nil, err
}
}

// try to find a pushable fork among existing remotes
for _, repo := range r.network.Repositories {
if repo != nil && repo.Parent != nil && repo.ViewerCanPush() && ghrepo.IsSame(repo.Parent, baseRepo) {
return repo, nil
}
}

// a fork might still exist on GitHub, so let's query for it
var notFound *api.NotFoundError
if repo, err := api.RepoFindFork(r.apiClient, baseRepo); err == nil {
return repo, nil
} else if !errors.As(err, &notFound) {
return nil, err
}

// fall back to any listed repository that has push access
var results []*api.Repository
for _, repo := range r.network.Repositories {
if repo != nil && repo.ViewerCanPush() {
return repo, nil
results = append(results, repo)
}
}
return nil, errors.New("none of the repositories have push access")
return results, nil
}

// RemoteForRepo finds the git remote that points to a repository
Expand Down
154 changes: 114 additions & 40 deletions pkg/cmd/pr/create/create.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"
"time"

"github.com/AlecAivazis/survey/v2"
"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/api"
"github.com/cli/cli/context"
Expand All @@ -17,6 +18,7 @@ import (
"github.com/cli/cli/pkg/cmdutil"
"github.com/cli/cli/pkg/githubtemplate"
"github.com/cli/cli/pkg/iostreams"
"github.com/cli/cli/pkg/prompt"
"github.com/cli/cli/utils"
"github.com/spf13/cobra"
)
Expand All @@ -40,6 +42,7 @@ type CreateOptions struct {
Title string
Body string
BaseBranch string
HeadBranch string

Reviewers []string
Assignees []string
Expand All @@ -60,13 +63,23 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co
cmd := &cobra.Command{
Use: "create",
Short: "Create a pull request",
Long: heredoc.Doc(`
Create a pull request on GitHub.
When the current branch isn't fully pushed to a git remote, a prompt will ask where
to push the branch and offer an option to fork the base repository. Use '--head' to
explicitly skip any forking or pushing behavior.
A prompt will also ask for the title and the body of the pull request. Use '--title'
and '--body' to skip this, or use '--fill' to autofill these values from git commits.
`),
Example: heredoc.Doc(`
$ gh pr create --title "The bug is fixed" --body "Everything works again"
$ gh issue create --label "bug,help wanted"
$ gh issue create --label bug --label "help wanted"
$ gh pr create --reviewer monalisa,hubot
$ gh pr create --project "Roadmap"
$ gh pr create --base develop
$ gh pr create --base develop --head monalisa:feature
`),
Args: cmdutil.NoArgsQuoteReminder,
RunE: func(cmd *cobra.Command, args []string) error {
Expand Down Expand Up @@ -96,9 +109,10 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co

fl := cmd.Flags()
fl.BoolVarP(&opts.IsDraft, "draft", "d", false, "Mark pull request as a draft")
fl.StringVarP(&opts.Title, "title", "t", "", "Supply a title. Will prompt for one otherwise.")
fl.StringVarP(&opts.Body, "body", "b", "", "Supply a body. Will prompt for one otherwise.")
fl.StringVarP(&opts.BaseBranch, "base", "B", "", "The branch into which you want your code merged")
fl.StringVarP(&opts.Title, "title", "t", "", "Title for the pull request")
fl.StringVarP(&opts.Body, "body", "b", "", "Body for the pull request")
fl.StringVarP(&opts.BaseBranch, "base", "B", "", "The `branch` into which you want your code merged")
fl.StringVarP(&opts.HeadBranch, "head", "H", "", "The `branch` that contains commits for your pull request (default: current branch)")
fl.BoolVarP(&opts.WebMode, "web", "w", false, "Open the web browser to create a pull request")
fl.BoolVarP(&opts.Autofill, "fill", "f", false, "Do not prompt for title/body and just use commit info")
fl.StringSliceVarP(&opts.Reviewers, "reviewer", "r", nil, "Request reviews from people by their `login`")
Expand Down Expand Up @@ -132,6 +146,8 @@ func createRun(opts *CreateOptions) error {
if r, ok := br.(*api.Repository); ok {
baseRepo = r
} else {
// TODO: if RepoNetwork is going to be requested anyway in `repoContext.HeadRepos()`,
// consider piggybacking on that result instead of performing a separate lookup
var err error
baseRepo, err = api.GitHubRepo(client, br)
if err != nil {
Expand All @@ -142,34 +158,108 @@ func createRun(opts *CreateOptions) error {
return fmt.Errorf("could not determine base repository: %w", err)
}

headBranch, err := opts.Branch()
if err != nil {
return fmt.Errorf("could not determine the current branch: %w", err)
isPushEnabled := false
headBranch := opts.HeadBranch
headBranchLabel := opts.HeadBranch
if headBranch == "" {
headBranch, err = opts.Branch()
if err != nil {
return fmt.Errorf("could not determine the current branch: %w", err)
}
headBranchLabel = headBranch
isPushEnabled = true
} else if idx := strings.IndexRune(headBranch, ':'); idx >= 0 {
headBranch = headBranch[idx+1:]
}

if ucc, err := git.UncommittedChangeCount(); err == nil && ucc > 0 {
fmt.Fprintf(opts.IO.ErrOut, "Warning: %s\n", utils.Pluralize(ucc, "uncommitted change"))
}

var headRepo ghrepo.Interface
var headRemote *context.Remote

// determine whether the head branch is already pushed to a remote
headBranchPushedTo := determineTrackingBranch(remotes, headBranch)
if headBranchPushedTo != nil {
for _, r := range remotes {
if r.Name != headBranchPushedTo.RemoteName {
continue
if isPushEnabled {
// determine whether the head branch is already pushed to a remote
if pushedTo := determineTrackingBranch(remotes, headBranch); pushedTo != nil {
isPushEnabled = false
for _, r := range remotes {
if r.Name != pushedTo.RemoteName {
continue
}
headRepo = r
headRemote = r
break
}
headRepo = r
headRemote = r
break
}
}

// otherwise, determine the head repository with info obtained from the API
if headRepo == nil {
if r, err := repoContext.HeadRepo(baseRepo); err == nil {
headRepo = r
// otherwise, ask the user for the head repository using info obtained from the API
if headRepo == nil && isPushEnabled && opts.IO.CanPrompt() {
pushableRepos, err := repoContext.HeadRepos()
if err != nil {
return err
}

if len(pushableRepos) == 0 {
pushableRepos, err = api.RepoFindForks(client, baseRepo, 3)
if err != nil {
return err
}
}

currentLogin, err := api.CurrentLoginName(client, baseRepo.RepoHost())
if err != nil {
return err
}

hasOwnFork := false
var pushOptions []string
for _, r := range pushableRepos {
pushOptions = append(pushOptions, ghrepo.FullName(r))
if r.RepoOwner() == currentLogin {
hasOwnFork = true
}
}

if !hasOwnFork {
pushOptions = append(pushOptions, "Create a fork of "+ghrepo.FullName(baseRepo))
}
pushOptions = append(pushOptions, "Skip pushing the branch")
pushOptions = append(pushOptions, "Cancel")

var selectedOption int
err = prompt.SurveyAskOne(&survey.Select{
Message: fmt.Sprintf("Where should we push the '%s' branch?", headBranch),
Options: pushOptions,
}, &selectedOption)
if err != nil {
return err
}

if selectedOption < len(pushableRepos) {
headRepo = pushableRepos[selectedOption]
if !ghrepo.IsSame(baseRepo, headRepo) {
headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch)
}
} else if pushOptions[selectedOption] == "Skip pushing the branch" {
isPushEnabled = false
} else if pushOptions[selectedOption] == "Cancel" {
return cmdutil.SilentError
} else {
// "Create a fork of ..."
if baseRepo.IsPrivate {
return fmt.Errorf("cannot fork private repository %s", ghrepo.FullName(baseRepo))
}
headBranchLabel = fmt.Sprintf("%s:%s", currentLogin, headBranch)
}
}

if headRepo == nil && isPushEnabled && !opts.IO.CanPrompt() {
fmt.Fprintf(opts.IO.ErrOut, "aborted: you must first push the current branch to a remote, or use the --head flag")
return cmdutil.SilentError
}

baseBranch := opts.BaseBranch
if baseBranch == "" {
baseBranch = baseRepo.DefaultBranchRef.Name
Expand All @@ -178,10 +268,6 @@ func createRun(opts *CreateOptions) error {
return fmt.Errorf("must be on a branch named differently than %q", baseBranch)
}

if ucc, err := git.UncommittedChangeCount(); err == nil && ucc > 0 {
fmt.Fprintf(opts.IO.ErrOut, "Warning: %s\n", utils.Pluralize(ucc, "uncommitted change"))
}

var milestoneTitles []string
if opts.Milestone != "" {
milestoneTitles = []string{opts.Milestone}
Expand Down Expand Up @@ -211,10 +297,6 @@ func createRun(opts *CreateOptions) error {
}

if !opts.WebMode {
headBranchLabel := headBranch
if headRepo != nil && !ghrepo.IsSame(baseRepo, headRepo) {
headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch)
}
existingPR, err := api.PullRequestForBranch(client, baseRepo, baseBranch, headBranchLabel)
var notFound *api.NotFoundError
if err != nil && !errors.As(err, &notFound) {
Expand Down Expand Up @@ -297,23 +379,15 @@ func createRun(opts *CreateOptions) error {
didForkRepo := false
// if a head repository could not be determined so far, automatically create
// one by forking the base repository
if headRepo == nil {
if baseRepo.IsPrivate {
return fmt.Errorf("cannot fork private repository '%s'", ghrepo.FullName(baseRepo))
}
if headRepo == nil && isPushEnabled {
headRepo, err = api.ForkRepo(client, baseRepo)
if err != nil {
return fmt.Errorf("error forking repo: %w", err)
}
didForkRepo = true
}

headBranchLabel := headBranch
if !ghrepo.IsSame(baseRepo, headRepo) {
headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch)
}

if headRemote == nil {
if headRemote == nil && headRepo != nil {
headRemote, _ = repoContext.RemoteForRepo(headRepo)
}

Expand All @@ -324,7 +398,7 @@ func createRun(opts *CreateOptions) error {
//
// In either case, we want to add the head repo as a new git remote so we
// can push to it.
if headRemote == nil {
if headRemote == nil && isPushEnabled {
cfg, err := opts.Config()
if err != nil {
return err
Expand All @@ -345,7 +419,7 @@ func createRun(opts *CreateOptions) error {
}

// automatically push the branch if it hasn't been pushed anywhere yet
if headBranchPushedTo == nil {
if isPushEnabled {
pushTries := 0
maxPushTries := 3
for {
Expand Down
Loading

0 comments on commit 7a8db80

Please sign in to comment.