App-GitHubPullRequest icon indicating copy to clipboard operation
App-GitHubPullRequest copied to clipboard

Add the ability to create new PRs

Open perigrin opened this issue 13 years ago • 5 comments

This should add the ability to create new Pull Requests.

perigrin avatar Mar 15 '13 23:03 perigrin

Thanks for the contribution! Great work! Nice job on the refactor! I do have some improvements I'd prefer to go in there.

  1. _fetch_repo_information() should cache the fetch. The way it is now it does it twice for a "create". A state variable %cache and an exists check should take care of that.
  2. _find_master_branch() needs a slightly better name. It's not obvious which repo it would find it in. Maybe _find_remote_branch(), _find_parent_branch(), _find_remote_master_branch() or _find_parent_master_branch(). Maybe the $repo should just be passed as a param to it? What do you think?
  3. $head and $base in sub create might be better named $remote_branch and $local_branch. You also need to verify that they are valid before you use them.
  4. If you haven't forked and pushed your local branch you'd get a github error, I presume. Can you inhibit that problem in some way by checking if the local branch already has a github remote and it has your HEAD commit?
  5. Your =cmd create line doesn't include specification of parameters (and the fact they're optional).
  6. I'd love it if you could make default $title and $body be the subject and body of the first commit message, just like the GH website does it.
  7. It would be double awesome if the $title and $body could be edited with $EDITOR, similar to how I do in comment(). I'd suggest that the first line is $title, and rest is $body. Please don't forget error checking. :)

If you don't have time to fix these things I might go ahead and see if I can do something about it.

robinsmidsrod avatar Mar 16 '13 09:03 robinsmidsrod

Regarding point number 3, $base is supposed to point to a branch in the parent repo (i.e. the one you forked, not your contributor's fork). And using $repo_info->{'master_branch'} seems like the sane approach.

On the other hand, $head should point to username:branch, where username is the contributor's GitHub name of the forked repo. Details at http://developer.github.com/v3/pulls/#create-a-pull-request

The challenge here is that the username is not necessarily what you get from git config github.user, but the username associated with the remote tracking branch you're working on. This could happen if you have full access on a repo that is not your own, and you use that to stage the PR.

If you haven't actually forked the parent repo yet, and is just working on getting your PR ready, git remote show -n $(git config branch.$(git rev-parse --abbrev-ref HEAD).remote) | grep -P 'Push\s+URL' | grep -v git:// doesn't show any valid push URLs. At this point you can run git remote -v | grep '(push)' | grep -v git:// to figure out if there are any push URLs available that seem writable and ask the user to git config branch.$(git rev-parse --abbrev-ref HEAD).remote someremote which should enable us to find the username. If they have no (seemingly) writable push URLs you'll have to tell them to fork the repo and add the remote.

robinsmidsrod avatar Mar 17 '13 08:03 robinsmidsrod

The issue to 3) above is that according to the docs you linked to state:

You cannot submit a pull request to one repo that requests a merge to a base of another repo.

Which I wasn't sure how they define repo. However the plan you outline here is "good enough" to try. I'll see if I can get to that when I get a chance.

perigrin avatar Mar 18 '13 15:03 perigrin

For 6) above you can use git show --quiet --pretty='%s' <commitish> for $title and git show --quiet --pretty='%b' <commitish> to get the $body. In terms of actually finding the sha1 of the first commit differing from the parent repo you could do my $parent_sha1 = _api_url("/repos/$remote_repo/git/refs/heads/$remote_branch")->{'object'}->{'sha'} (using $remote_branch from 3) and move backwards with git from your current HEAD until the commit after the one found above. That should be git log --pretty='%H' ${parent_sha1}..HEAD | tail -n1. Then you just run git show on that. Of course avoid to use shell commands like tail.

robinsmidsrod avatar Mar 18 '13 16:03 robinsmidsrod

Decided to copy this information from IRC in case I ever need to look back at it.

<robinsmidsrod> If you're still confused about 3), in terms of this PR we're working on now,
 $head should be "perigrin:master" and $base should be just "master", and it's
 a reference to the master branch in robinsmidsrod/App-GitHubPullRequest
<perigrin> right ... in general $head should be "$username:$current_branch"
 and $base should be $master_branch
<robinsmidsrod> yup - in fact, it can always be "$username:$current_branch",
 because if it is the same repo,  the $username part should just be ignored by GitHub

robinsmidsrod avatar Mar 18 '13 17:03 robinsmidsrod