Properly merging pull requests

[Rainer Müller]

On 2016-11-03 16:59, Mojca Miklavec wrote:
> Hi,
> 
> We have recently experienced problems with pull requests showing up as
> rejected on the web interface rather than merged (while the changes
> were in fact accepted, possibly with some modifications).
> 
> I admit my sins. In one case I did a minor modification (squashed
> commits, removed the "Id" line, edited the commit message), while
> basically everything else stayed the same as the author submitted and
> everyone would have preferred if GitHub displayed "Merged" rather than
> the red rejected sign.
> 
> We could have asked the author to keep fixing the branch forever, but
> at some point it's simply faster and easier if an experienced
> MacPorter just finishes the obvious rather than turning the users away
> due to some stupid mistakes. The users can fix the mistakes in the
> next PR.
> 
> In another case I (stupidly?) decided to add "Closes: #N" at the end
> of the commit message (I thought it would be useful to have a link to
> the original PR which is something that the committer cannot do anyway
> without knowing the number of PR in advance). I probably also sinned
> by thinking of "playing safe" and using cherry-pick rather than
> "wrongly rebasing" and risking some undesired merge.

GitHub will already add an annotation right next to the branch name in
the commit view to show that this commit came from a pull request. As an
example, note "master (#2)" right below the commit message here:

https://github.com/macports/macports-ports/commit/ec150ad742e511cc38e5a0815c8b5805125d2aea

I think the proper way to do it on GitHub would be as follows:
When the pull request author checked the box for "Allow modifications by
maintainers" [1], you can force-push your changes to the pull request
branch, replacing the original commits. Then you can merge the pull
request from the GitHub web interface.

> Can someone provide some guidelines of DOs and DONTs when accepting
> (and modifying) pull requests? That should cover the cases that need:
> - squashing commits
> - minor edits of commit messages
> - other minor edits of the sources

If commits were made to address review comments, but these commits do
not follow our rules for commit messages, they need to be squashed or
edited. Otherwise, after the changes were brought to master with a
rebase, looking at these commit messages in the linear history would not
clearly show what they addressed.

Rainer

[1]
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/