Merge PRs locally

[mgalgs]

It can be useful to merge existing PRs locally to test them, etc. Add a
function and a pop-up binding to do this.

Fixes #14

[mgalgs]

Two possible problems with this PR:

1. Not sure if Fetch branches of pull requests from magit-status #14 really just wanted to fetch, or if the intent was to fetch and then merge. If it was the former then this PR doesn't really fix Fetch branches of pull requests from magit-status #14.
2. None of the pop-up actions under H currently act on individual issues, so it's a bit of a UX inconsistency.

[vermiculus]

This looks awesome! I'll have to take a look at this tonight (and read up on git-am), but I'm ok with the possible UX inconsistency.

I wouldn't say this PR properly fixes #14 since I can definitely see a use case for just wanting to fetch the branch, but this PR would still be very useful to merge as-is.

I'll keep you posted.

[vermiculus]

Alright, I've been thinking about this. Is there any way we can integrate this into Magit's natural workflows? git-am is w in magit-status – could we instead advise magit-am-apply-maildir (w m) to provide a default for read-file-name here? Perhaps locally?

(defun magit-am-apply-maildir (&optional maildir args)
  "Apply the patches from MAILDIR."
  (interactive (list (read-file-name "Apply mbox or Maildir: ")
                     (magit-am-arguments)))
  (magit-run-git-sequencer "am" args (expand-file-name maildir)))

[vermiculus]

@tarsius How sad would you be if a precedent was set for plugins inserting themselves into internal popups like magit-am-popup? That would probably be the most appropriate solution here, if possible. Such a command could complete over pull requests and provide the URL to the magit-run-get-sequencer up there.

[vermiculus]

Out of curiosity, why was this necessary? If we're applying the merge locally, that shouldn't impact GitHub's data.

[vermiculus]

I do agree this is pretty strange. It'd be nice to have this as actionable from the pull-request itself. This popup seems more oriented to actions that are 'original sins' in a way (with the possible exception of submitting a pull request being based on a branch).

[vermiculus]

Can git-am fail?

[vermiculus]

Also, -3 looks like it's something that can be overridden with git config variables. Should we not respect the user's preferences?

All this could be mitigated by using magit's am sequencer.

[vermiculus]

The module/package/namespace is magithub-issue, so this would rather be named something like magithub-issue-merge-pr-locally.

[vermiculus]

I've written some API as part of a real, concise fix to #14. It could be useful here: 23d7b72

For instance, here's an implementation using more of magit's stuff and the new utility functions:

(defun magithub-pull-request-merge (pull-request &optional args)
  "Merge PULL-REQUEST with ARGS.
See `magithub-pull-request--completing-read'.  If point is on a
pull-request object, that object is selected by default."
  (interactive (list (magithub-pull-request--completing-read)
                     (magit-am-arguments)))
  (unless (member pull-request (magithub-pull-requests))
    (user-error "Unknown pull request %S" pull-request))
  (magithub-with-hub
   (magit-run-git-sequencer "am" args (plist-get pull-request :url))))

[tarsius]

How sad would you be if a precedent was set for plugins inserting themselves into internal popups

I have no objections to that, but would like to ask you to provide an option to keep magithub from doing it. Personally I would probably use that and use magithub mostly for informational purposes.

[vermiculus]

Would interactive functions like magithub-integrations-inject and magithub-integrations-withdraw be sufficient? I imagine these could be placed on the magithub-dispatch-popup -- personally I'd configure them in my (use-package magithub :config).

[mgalgs]

Maybe I'm missing what you're proposing but I don't think additional configuration should be required to be able to merge in pull requests... Seems like the sort of batteries that should be included in a github PR management package...

[vermiculus]

@mgalgs The interactive functions would still exist -- they just wouldn't be completely integrated into Magit's popups. The goal of Magithub was to be a truly integrated plugin for GitHub-related tasks -- I'd rather not completely separate it from Magit's established workflows.

FWIW though, ever since #49 I've been thinking about a completing-read index of integration points that can be accessed from the dispatcher -- to help discoverability.

[blaenk]

Don't mean to add noise, but originally I thought this was a feature of magithub and when I found out it wasn't I found this. Glad it's being worked on! Any updates?

[vermiculus]

@blaenk I'll see what I can do about getting a magit-integrated solution into master by the new year, but it will not be batteries included per Jonas' request. I'll probably have to include some sort of message saying something like 'magithub not fully setup' when magit starts and magithub is turned on (which turned off by, say, (setq magithub-setup-complete t)).

[blaenk]

Sorry for the mess with issues/comments. I too got confused about this PR and what it aimed to do. This looks nice, but I personally would also like to see a feature to simply fetch/checkout PR changes into their own branch, preferably with the choice of 2 strategies: 1) adding the PR source remote and checking out the PR branch and 2) fetching the PR ref github creates for the PR.

Then I can simply use magit/git to merge the branch if I want to.

See this comment on #14 for more information about what I mean.