> On Nov 3, 2016, at 11:59 AM, Mojca Miklavec <mo...@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 _______________________________________________ macports-dev mailing list macports-dev@lists.macosforge.org https://lists.macosforge.org/mailman/listinfo/macports-dev
