Properly merging pull requests

[Sterling Smith]

On Nov 3, 2016, at 8:59AM, Mojca Miklavec <mojca at macports.org> 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.
The user will not learn if you change it under his/her feet.  I think that you should make line by comments of changes that need to be made in the files tab of the pull request and ask the pull request submitter to make those changes.  Or you can make them yourself and push them to the branch of the pull request and ask the submitter if s/he is OK with them.  Either way, the submitter learns of the MacPorts Way, and will be less likely to make those mistakes in the future.  
> 
> 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.
> 
> 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

When a committer is happy with the changes of the pull request, but not the commit messages, then they should request that the submitter make appropriate changes to the branch (with an interactive rebase), and force push those changes.  The branch will then be in a position to push the GitHub rebase button.
> 
> - other minor edits of the sources
See my comment above about pushing changes to the branch of the pull request and asking the submitter for their approval.
> 
> Thank you,
>    Mojca

My two cents.

-Sterling