Project:GitHub/Pull requests
Pull request assignment
The new pull requests are automatically assigned using a cronjob run every 4 minutes. The assignment script leaves a comment @-mentioning appropriate developers/teams and setting labels appropriately.
The assignment is performed based on the list of changed files. The script reads the current maintainers from the metadata.xml files, and maps them to GitHub developers/teams. It should be noted that the files from the current ::gentoo version are used rather than ones from the pull request.
The following additional rules apply:
- New and unmaintained packages are assigned to Project:Proxy Maintainers as candidates for proxy maintenance.
- Eclass and profile changes are not assigned automatically, and instead assumed to be handled by other assignees of the pull request (if any).
- Pull requests involving packages with more than 5 unique sets of maintainers are not assigned automatically (to prevent spam).
- Packages whose maintainers have no GitHub account (as in, none of the maintainers do have one) are marked as non-assignable.
If the script was able to find at least one assignee for each of the modified packages, the pull request is marked assigned. Otherwise it is marked can't assign.
In order to prevent automatic assignment, please set the assigned label or add at least one explicit assignee. In order to have the pull request re-assigned, remove all assignees (if any) and the appropriate label. The script will automatically remove the previous assignment comment.
Maintainer address verification
The assignment script additionally verifies that the e-mail addresses of the maintainers are registered on the Gentoo Bugzilla. If an unregistered address is found, the assignment comment contains a verbose warning about it.
When the submitter fixes the issue, or the package list changes, you can remove the assigned label in order to force reassignment. This will also repeat the verification.
Pull request CI
All pull requests are subject to CI runs that run pkgcheck on the whole repository. The CI script is run every 20 minutes, and processes a single pull request (and leaves ETAs on all remaining pull requests).
The CI run consists of pregenerating caches and running pkgcheck on the whole repository. This makes it possible to catch indirect dependency breakages resulting from the changes. If any issues are found, the result is compared to the repository state on which the PR was last rebased. The report indicates appropriately which issues are new to the pull request, and which are persisting (i.e. could be fixed by rebasing).
In order to rerun the CI, force push new commits over the pull requests. The script will detect changed commit identifier, and rerun the checks. it is currently not possible to disable CI on a pull request.
Developers are encouraged to file pull requests in order to CI-test their package removals.
The CI checks only report severe problems. Furthermore, pkgcheck is imperfect (yet very fast) and may fail to notice some subtle breakages (especially if USE flag masking/forcing) is involved. CI is not an excuse not to run
pkgcheck scan, at least for the packages being committed.Pull request reviews
Who is eligible to review?
Everyone can review pull requests. However, please make sure that your comments are correct and on topic. It is a bad experience for our users if they are confused by incorrect suggestions, and then asked to revert them.
Pull request reviews are non-binding. They are supposed to improve the pull request and make it more likely to be accepted as-is by the assignee. The assignee may choose not to accept a pull request independent of positive or negative reviews.
How to review?
A different levels of review are appropriate to the different kinds of commits. In particular:
- If a pull request affects a small part of ebuild (adding a patch, fixing a small bug), don't request the author to fix irrelevant bugs (unless he explicitly wants to).
- If a pull request does a version bump, don't ask the author to fix minor bugs that are unchanged since the previous version (unless he explicitly wants to).
- If a pull request does an EAPI bump, it might be reasonable to request changes semi-relevant to the EAPI bump.
- If a pull request submits a new ebuild, it is highly desirable to bring it to top-quality before being committed.
- If a pull request requests proxied maintenance of an existing ebuild, it is desirable to ask the new maintainer to fix some bugs, however not necessarily all of them at once.
Be clear in your comments. Avoid unnecessary nitpicking, and even more importantly — avoid wars between developers over ebuild style. If something is purely your preference or recommendation, state that clearly and explain why. If you're not sure, say that, suggest how to check. Feel free to @-mention other developers or teams (QA for technical issues, licenses for license doubts, python for Python doubts, etc.) to get help.
For simple additional changes (revbump, typo fix), don't request the submitter to update the pull request. Instead, leave a comment stating what is wrong and that it will be fixed by the person merging it, so that the submitters knows how to avoid the problem in the future. Add a 'revbump on merge' or 'fix on merge' label appropriately, so that people won't merge it accidentally.
How to submit reviews?
Preferably, use the GitHub review feature to submit reviews. This makes it possible to combine all comments into a single mail rather than sending one mail for each comment.
You can either review the final result of the pull request (Files changed tab) or single commits separately. In order to add a comment, click a line to be commented on, write the comment and use Start a review. This will cause this and your further review comments to be marked pending — which also allows you to modify or remove them before sending.
Please note that GitHub stores your pending, so you can freely navigate through multiple commits and tabs, or even leave the pull request without losing your input. Once ready to submit, navigate to the Files changed tab and use the Review changes button on top.
You can write the general comments here (e.g. applying to the whole pull request, if necessary) and additionally decide if you approve or request changes to the pull request. Neither is obligatory — you can also use the review feature to leave a few neutral comments in a single mail.
Merging pull requests
Do not use the big green Merge pull request button. The repository is a mirror, so the changes will be discarded. You need to merge the pull request into git.gentoo.org instead.
Pull request approval
Please remember that pull requests follow the same rules as other commits to the Gentoo repository. Therefore, you can only merge a pull request if you would be permitted to commit the changes to the Gentoo repository. Furthermore, you take responsibility for the changes you are merging.
In particular, changes affecting packages maintained by other developers/projects may require their permission to be merged. If the maintainers do not use GitHub, you may need to use other media to contact them and obtain approval for the merge.
Common tips
Adding the git remote
Most of the methods listed here require you to fetch pull requests from GitHub. In order to do that most conveniently, create a new remote for GitHub and enable fetching of pull request refs:
user $git remote add github https://github.com/gentoo/gentoo.git
user $git config --add remote.github.fetch '+refs/pull/*/head:refs/remotes/github/pull/*'
Afterward, each call to:
user $git fetch githubwill fetch new pull requests and expose them as github/pull/NNNN refs.
Closing pull requests
There are three ways of closing a pull request:
- Via a merge commit.
- Via a Closes tag.
- Manually.
If you do a merge commit involving unmodified upstream commits, GitHub will detect commit id match and close the pull request automatically.
Alternatively, you could add a Closes tag to your commit message to make GitHub close the pull request:
dev-foo/bar: fix fnord, #mmmmmm
Closes: https://github.com/gentoo/gentoo/pull/NNNN
Finally, if you did neither of that you need to close the pull request manually.
Inline merges
Using git cherry-pick
In order to cherry-pick all new commits from the pull request, do:
user $git cherry-pick master..github/pull/NNNNAfterwards, edit the commits as necessary, rebase and sign them and push.
Using git am
It is also possible to use GitHub generated patches to avoid adding the GitHub repository locally. It should be noted, however, that git am is weaker than git cherry-pick at conflict resolution.
user $wget -O - https://github.com/gentoo/gentoo/pull/NNNN.patch | git amDoing a true merge
True merges should be only used whenever there is a good reason to do so. Expect a long bikeshed following each one.
In order to create a true merge commit for a pull request, do:
user $git merge -S github/pull/NNNNPlease do not use the default merge commit message. Instead, modify it to adhere to the Gentoo commit message rules, indicating the most important changes included in the merged branch and include the pull request URL in it.
Remember that you need to explicitly make git preserve merge commits when rebasing, e.g.:
user $git pull --rebase=preserve -S