possible to merge PR's from magit?

[titaniumbones]

I s it possible to merge PR's (in my case, Github PR's) from magit? I had the impression in the Forge announcement that it was:

Forge also fetches pull-request references using Git. I find it quite surprising that there aren’t more tools that take advantage of this easily accessible information. Forge fetches these refs even for the forges whose APIs are not supported yet. The user experience is much better when the API data is also available but even just being able to checkout and merge pull-requests using Magit is a big win.

But I can't find mention of it in the docs or in the code.

Also just in passing forge continues to be my biggest quality-of-life improvement of 2019!

[tarsius]

You have to checkout a local branch for the pr first (b y) then you can use magit-merge-into (m i) to merge it. The correct target branch will be offered and you will be asked whether you want the pr branch and the pr remote (if appropriate).

I have decided to not add a command to merge without creating a branch first because I think its a good thing to do some review in Magit before doing so. On the other hand since the pr's commits are now listed when viewing the pr, that is possible without creating a branch first. Then again you might want to rebase and/or make small changes and for that you need a branch.

Please try using the "create branch and then merge that" approach for a while and let me know how it works out for you.

Also just in passing forge continues to be my biggest quality-of-life improvement of 2019!

Thanks!

[titaniumbones]

ok thank, I see now how to do it, I'll try for a while.

I have decided to not add a command to merge without creating a branch first because I think its a good thing to do some review in Magit before doing so

by review you mean just reviewing the code in one's own environment, rihgh? Only asking because I use github code reviews for student work and if I could do those from Emacs it would be one less reason to ever leave.

[tarsius]

by review you mean just reviewing the code in one's own environment, rihgh?

rihgh :-)

Only asking because I use github code reviews

And you are talking about those "new" inline reviews, right? Like what I have done for #90?

Forge doesn't support that yet and I have to admit that I never quiet got used to that feature. I got the impression that it makes it hard to keep track of "obsolete" discussions after a new iteration was force-pushed. So I am trying to force myself to use that feature more now.

Maybe you could share your experience? For example how do you verify that "I have taken all your feedback into account" actually is true?

The plan currently is to convert inline comments to Git notes and to use "iteration N of pull-request P" refs to avoid loosing "obsolete" reviews, but that will be quite difficult to implement.

[titaniumbones]

rihgh :-) (that was my second try, too! Jeez.)

Maybe you could share your experience? For example how do you verify that "I have taken all your feedback into account" actually is true?

:-) I also find the feature not completely helpful. but it is an easy way to comment directly on one or more lines of someone's code. This allows me to discuss exactly what kind of error the student is making. This can of course be done in an ordinary comment, but then I have to cut and paste the relevant lines and enclose them in a markdown code block with the language identifier... that becomes a little tiresome. If there was a way to grab a chunk of code with line numbers from the diff view in Magit? If so, then it might be more straightforward to include the code review directly into the PR itself when it gets created, or in a later comment.

[stig]

Maybe you could share your experience? For example how do you verify that "I have taken all your feedback into account" actually is true?

I'd love to know this too, because I frequently experience that the GitHub inline comments are left unaddressed, presumably because they disappear too quickly if you change something else nearby.

[titaniumbones]

Maybe you could share your experience? For example how do you verify that "I have taken all your feedback into account" actually is true?

I'd love to know this too, because I frequently experience that the GitHub inline comments are left unaddressed, presumably because they disappear too quickly if you change something else nearby.

well, this is an excellent point. Since I mostly use this feature to give students feedback on their assignments, it's not really my responsibility to make sure that the problems have been addressed. On the other hand, I spend some time in another project where people do seem to use the line-by-line review pretty successfully: https://github.com/edgi-govdata-archiving . However, this is in the context of a group of people who are working closely together. So the line-by-line review is more along the lines of a conversation, than, say, a formal requirement.

On this last assignment I tried to just quickly shift back and forth between an org-mode buffer with student comments, and a diff buffer (diff between student work handed in, and original state of the assignment). I copy the lines I'm interested in over to an org-mode source block and write comments there.

That method works OK. It's a bit awkward and involves quite a few keystrokes. I might be able to make things easier by writing a few simple functions and giving them keybindings, but I haven't really thought about how best to manage the workflow.

Hopefully that is a little bit of an explanation.... What do you do when you need to comment on individual lines or functions in a PR?

[stig]

I tend to use inline comments quite a bit, but because of the problem I mentioned above, if there's something I really want them to change I mention it also in the general comments section, e.g. "Approved on the assumption that X will be addressed".

[xendk]

We use the review functionality extensively.

For example how do you verify that "I have taken all your feedback into account" actually is true?

Well, we don't really let the review system enforce that, as you've pointed out it really doesn't. We assume that everybody is grown enough to read all comments and act appropriately. We use the approved/changes requested status of the review to give an overall sort of pass/fail, but for things that doesn't actually break something we'll approve with comments and let the developer decide if it's passable or if they'll address the comments.

I'd like to be able to see the comments and reply to them, and set the overall review status. Being able to see the comments in the original file (with collapsible markers) would be awesome for bigger reviews where you're jumping around, so you always knows where you've commented, but that's a bit more involved.

[titaniumbones]

Just thought I'd note that there's a new package github-review that allows at least preliminary editing of code reviews in Emacs, and as of the last day or so hooks into forge. I haven't used it much yet but it seems like a promising start.

[paulRbr]

I would like to jump in this conversation as I have been trying the magit-merge-into to merge PRs on a forge (Github in my case).

Overall it's fine from a git perspective, however I have some context missing in the merge commit message.

Indeed when I merge from Github, the merge commit looks like:

Merge pull request #<ID> from <namespace>/<branch_name>

<PR title>

When I merge manually with Magit the merge commit looks like:

Merge branch '<branch_name>' into <target_branch_name>

I believe forge could inject the pull request/merge request ID into the merge commit message automatically. The <namespace> is not so important in my case, but the PR #<ID> is. Indeed it helps to find PRs really quick by browsing the git history (instead of going to Github)

WDYT?

[tarsius]

magit-merge-absorb and magit-merge-into now append " [#N]" when merging a pull-request. Other merge commands don't.

Doing this did require any major changes. Using the same message as Github would be complicated, so I am not doing that.

[tarsius]

So:

1. possible to merge PR's from magit?

   Yes. :stuck_out_tongue_closed_eyes:

2. Please support review comments.

   See #75.

3. Please include the pr number in merge commit messages.

   Just did. :yum:

[titaniumbones]

Am finally trying this workflow and am not sure I have it right. Is there a final step that causes the PR to be closed?

[tarsius]

Is there a final step that causes the PR to be closed?

Pushing to master (or whatever the "target" of the pr is). It's actually the Forge that detects that the tip of the pr branch is reachable from master. If you made changes to the pr branch, then that automatically closes it as "merged".

It's important that you push the pr branch before master otherwise you have no choice but to manually close the pr. The pr would then forever remain in the "closed" state instead of being "merged" and the listed commits would also forever be wrong. This complication is due to unfortunate oversimplification on the server side.

[titaniumbones]

Ah ok. Am I supposed to merge into master or origin/master?

[tarsius]

Am I supposed to merge into master or origin/master?

The former, not least because the latter is not possible. It's not possible to check out a so called remote-tracking branch (i.e. a local ref (usually in the refs/remotes/* namespace), which tracks the state of a remote ref).

You can checkout the commit that origin/master points at. That results in HEAD being detached, but does not prevent merges. However creating commits (not limited to merge commit) does not change what origin/master points at and certainly does not push to origin to make sure that its refs/heads/master is updated.

So the expected workflow is:

1. Optionally make changes to the pr branch.

2. If (1), then force push the pr branch (e.g. feature to origin/feature).

3. Merge into default merge target using m i RET. This checks out the local representation of the target (master), merges the source, deletes the local source (feature), and leaves the target checked out.

   This does not delete refs/heads/feature on origin (nor does it delete refs/remotes/origin/feature), among other things because CI may not be done yet. If you delete a pr branch too early, then the CI status for that pr does forever is "could not run tests because there is no such branch".

   This does not update origin/master to give you a chance to review the result.

4. Push master to origin/master.

5. Eventually delete the pr branch using b k feature RET (which deletes both the remote-tracking ref as well as the actual remote ref).

[asymmetric]

Can we change the [#N] in the commit message to #N, so that it's clickable on GitHub?

i.e. the actual # character should show up.

[tarsius]

@asymmetric Could you please point me to an instance where Github fails to do the current format so that I can confirm that without having to setup a test repository just for that purpose.

[asymmetric]

@tarsius sure, here you go. For comparison, this is the standard GitHub format.

[tarsius]

I've added the number sign. I think that always was the intention.

[asymmetric]

@tarsius can't see any new commits yet, are you planning on pushing the changes later?

[tarsius]

The change was to Magit: https://github.com/magit/magit/commit/2dd12b289b41f7da11c9d609aee2519db99e6661.

[wraithm]

@tarsius I totally agree you should pull in PRs locally to review. However, my organization disallows direct pushing to the mainline repo; that's an option in the branch protection settings if you pay GitHub for it. The only way to merge a PR is explicitly on the website or through the API after an accepted review. So, that's a counter-point to having an inability to edit the status to merged. What are your thoughts on this issue?

[tarsius]

What are your thoughts on this issue?

Well, you asked:

The only way to merge a PR is explicitly on the website or through the API after an accepted review.

That's awful.

Github creates somewhat corrupted commits by default. The message does not end with newline as it is supposed to. In some cases it claims "github" was the committer. It does not simply fast-forward when it should do so. Etc...

So, that's a counter-point to having an inability to edit the status to merged.

"counter-point to [...] an inability" Uhuh.

I think what you are saying is: Since some people cannot use the recommended workflow would you reconsider adding a command that performs the merge using the API?. To which my answer is: I can write that command and post it here, but I don't want to add it to the package. Doing that would encourage people to use it even when that is not necessary and since I have such a low opinion about how github performs merges I really don't want that to happen.

[wraithm]

Yeah, that's all fair, @tarsius. I completely agree with you. Thanks for your thoughts! I'd rather we just pushed if we can figure out a nice way to do it.

I don't know much about the Github API or ghub. Is it possible to automate better merge messages (newlines at the end, fast-forward only, etc) and maintain these branch protection rules? To be clear, I'm not necessarily asking you to make any changes, just exploring the options. 👍

For context: My company actually just switched from Mercurial to git+github, so we're still figuring this stuff out. I'm worried about giving out push --force ability. Under mercurial, we were able to manage an immutable main repo easily. We're also in a heavily regulated industry, so we have requirements on reviews and such. I don't see any thing in the branch protections about like a "fast-forward push only" ability. That would be ideal, but I don't think that's an option yet.

[xendk]

@wraithm I'm pretty sure that branch protection stops non-ff pushes. I remember having to turn it off to fix up mistakes.

[wraithm]

Yeah, @xendk. You're totally right. Force pushes can be disallowed! That's great. Okay. Nevermind about my questions then.

[amuckart]

I have a similar issue to @wraithm - except that my company uses GitLab internally not GitHub.

The primary repo my team works with is configured to prevent remote pushes directly to master. There are good reasons for that and it's not going to change, so at the moment the only way to get changes into master is to push a branch (which creates a merge request and kicks off CI pipelines) then use the web interface to merge it.

It's not a show-stopper, and I'm still in a much better place with magit and forge than without, but it would be really nice to be able to manage GitLab MRs with labels etc. and work within our established workflow directly from Emacs.

Thanks.

[shindere]

I, too, like 'forge' quite a lot. Many thanks for this extremely useful program.

I, too, would like to be able to merge pull requests without having to checkout the branch first. I have several reasons for that:

1. Sometimes I made the review in aonther way, e.g. for a documentaiton change, I don't feel the need to checkout the branch.

2. Sometimes a merge decision has been made and my organisaiton is just waiting for the CIto be happy. IN such cases, I just want to "click the merge button".

   3. I would like my merge to be as close as possible to the merge a sighted user would do through e.g. the GitHub interface, but I would like this to happen without having to open my browser, meaning from within emacs because it is a place which feels way more secure ot me than my browser and a web page.

Is there any hope of an evolution here?

[tarsius]

Is there any hope of an evolution here?

I actually started working on this in January (on the96-api-merge branch), but I never finished it. So the good news is that I plan to implement that eventually and that it shouldn't be too hard. The bad is that I haven't been prioritizing it and that Iately don't have time to work on forge and magit.

[shindere]

Thanks a lot for your response, Jonas!

To me the good news is way more important than the bad one, there is no hurry on my side so please don't feel any pressure!

Will you report back here when something is ready, even if it's to request testing?

Would be happy to try it out!

[tarsius]

Will you report back here when something is ready, even if it's to request testing?

Yes. And I am reopening this issue so I won't forget.

[shindere]

Jonas Bernoulli (2021/04/03 03:58 -0700):

Will you report back here when something is ready, even if it's to request testing?

Yes. And I am reopening this issue so I won't forget.

Many thanks, that's great and lovely from you.

[tarsius]

I've added a new forge-merge command to the magit-merge popup. It's only marginally more advanced than the minimal viable implementation but might proof useful anyway.

[tarsius]

Certain transient popups hide some of their suffix commands by default. These additional suffixes can be enabled interactively. TL;DR Enter the transient as usual and type C-x l.

_{This is a canned response; it may not apply 100% in this case.}

[shindere]

Jonas Bernoulli (2021/06/19 13:02 -0700):

I've added a new forge-merge command to the magit-merge popup. It's only marginally more advanced than the minimal viable implementation but might proof useful anyway.

Many thanks! Will try to provide feedback!

[rafonseca]

I've added a new forge-merge command to the magit-merge popup. It's only marginally more advanced than the minimal viable implementation but might proof useful anyway.

This feature is great. Thanks!