`hub merge` should use GitHub's merge-button API #273

Closed
wants to merge 8 commits into from

2 participants

@wking

The hub merge functionality implemented in #164 creates the merge commit locally, but leaves the pull request open. It would be better to use GitHub's merge-button API directly. I'm agnostic on whether the commit created using the merge-button API should be fetched and merged locally after it is created on GitHub.

@wking

Fixing this should make #186 a good deal easier.

@wking

As I say in the 4a7b507 commit message, this is pull request is a work in progress. I'll put off changes to the code until I hear some positive comments on the revised scenarios (and possibly make further scenario revisions).

@wking wking referenced this pull request in swcarpentry/boot-camps Jan 3, 2013
Merged

SQL cheat sheet #7

wking added some commits Dec 31, 2012
@wking wking WIP merge.feature: Rework pull-request merges for the merge-button
I'm not sure if the second (private) merge is supposed to succeed or
not.  I'm also not sure if `hub merge …pull/…` should update the local
branch to reflect the changes (making it more like `git merge`) or if
the user should do that with a traditional Git fetch/merge (making it
more like the GitHub Merge Button).
d62de6b
@wking wking github_api.rb: Add PUT support
Parameter processing should be shared between POST and PUT.
95ea261
@wking wking added a commit to wking/email-issue-reply-testing that referenced this pull request Jan 15, 2013
@wking wking Testing for github/hub#273 1f1f707
wking added some commits Jan 15, 2013
@wking wking github_api.rb: Add GitHub Merge Button (TM) support 7cbcfd5
@wking wking commands.rb: Add a 'merge-button' command
This may be sufficiently different from Git's 'merge' command to
deserve its own name.
b12e4ca
@wking wking commands.rb: Teach merge-button the '-m COMMIT_MESSAGE' option a772761
@wking wking commands.rb: Teach merge-button the '-i PULL_ID' option
I also make the successful-merge message more explicit
($owner/$repo#$pull_id), now that the source project may be
auto-detected.
043159a
@wking

I haven't heard anything with respect to d62de6b, so here's an implementation that adds a new merge-button command. Having a new command reduces the surprise when we don't touch the local repository. No tests yet, I'll wait for some feedback first.

@mislav
GitHub member

Thanks for all the work on this, but I've decided I don't want to use the Merge Button API from hub. The Merge Button was created for scenarios where you don't have the git repository at hand. Since with hub you have the repo, it would make more sense to make a real merge and mark the pull-request as closed.

However, I don't want to mark the PR as closed immediately after merge, but only when the merge is pushed. I have to think about how to achieve this technically

@wking
@mislav
GitHub member

I'd have hub merge cache the PR ID and merge commit hash in $GIT_DIR/hub-merge. Then you'd need a hub push hook to see when you were pushing that commit hash to GitHub

That's a good suggestion!

Personally, I think it would be better to add a new --close option to hub merge (for cases where you know you're going to push the merge immediately). I'd also add --close SHA1 -i ISSUE processing to hub pull-request

Not sure, it sounds like too complex syntax to remember, and the whole workflow is a bit too manual for my taste. In most cases I would rather add new commands than new flags to existing commands

@wking
@wking
wking added some commits Jan 15, 2013
@wking wking Document the 'merge-button' command 1dcd569
@wking wking commands.rb: Extend 'display_api_exception' to print error messages
So you can see what GitHub is complaining about.  For example,

  $ hub merge-button https://github.com/defunkt/hub/pull/9999999999
  Error pushing merge button: Not Found (HTTP 404)
  #<Net::HTTPNotFound:0x7f4cf845c670>
  {"message":"Not Found"}

This was useful for debugging, when I originally thought that the
whole PUT body was optional if you didn't want to specify
'commit_message'.  In that case, I got:

  $ hub merge-button wking/email-issue-reply-testing#2
  Error pushing merge button: Bad Request (HTTP 400)
  #<Net::HTTPBadRequest:0x7f944f75a450>
  {"message":"Body should be a JSON Hash"}

Which is a lot more helpful than just "Bad Request".  It would be nice
if the 'Net::HTTP*' object hash was not printed, but I lack sufficient
Ruby-foo to know how to do that.
95f45ee
@mislav mislav referenced this pull request in git-merge/hack-day May 10, 2013
Closed

Improving `pull-request` and other features of hub CLI tool #1

@mislav
GitHub member

I'm going to close this since I already explained why I don't want to go in the Merge Button API direction. Here's the current state of things:

  • When you merge a pull request with hub merge and push it to the master branch, GitHub will detect the merge commit and automatically close the PR. So I guess the original problem that this issue was about got solved.
  • If you cherry-pick, rebase, or otherwise change the pull request commit SHA, GitHub won't be able to detect that the PR landed in master and you'll have to close the PR manually.
@mislav mislav closed this May 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment