Properly merging pull requests

[Lawrence Velázquez]

> On Nov 3, 2016, at 11:59 AM, Mojca Miklavec <mojca at macports.org> wrote:
> 
> 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.

I think (don't quote me on this) that GitHub considers a PR "merged"
when the base branch is updated and has incorporated the changes from
the head branch. If a committer modifies their local branch before
merging, the base branch does not look like it has merged in the head
branch because the new content is in fact different.

(This makes sense, in a way. After all, if you had to change a PR branch
before merging it, did you really accept the PR?)

> 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.

To reduce excessive communication, it does seems like a good idea to ask
contributors to allow us to push changes to their PR branches. Those
modified branches should then merge as desired.

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

> 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).

Why was that stupid? Did it cause a problem?

(Note that GitHub shows PR links on closing commits. Look just under the
commit message, next to "master":
https://github.com/macports/macports-infrastructure/commit/1e6b092)

> I probably also sinned by thinking of "playing safe" and using
> cherry-pick rather than "wrongly rebasing" and risking some undesired
> merge.

You cherry-picked instead of rebasing the PR branch onto master? That
shouldn't cause any problems, I think.

> 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

My sense is that the only real rule is that you need to push
modifications before doing the merge. For most PRs, that would require
the contributor to enable the setting that I mentioned above.

vq