Patch Review Workflow
The glibc project now uses patchwork to track its patches; its instance is maintained at http://patchwork.sourceware.org.
Getting a Sourceware Patchwork Account
If you are one of the glibc MAINTAINERS then you need an account on the patchwork instance to maintain state for patches you review or post.
- Click "register" from the top right.
- Enter in your details and register.
- Once you have your registration complete and can login, you should select "profile" and add in all the aliases you use to send patches to the list. This step helps patchwork assign patches to your account.
Lastly contact SiddheshPoyarekar or CarlosODonell to get added to the maintainers list on patchwork. This allows you to edit the state of any patch for the project. Please use this ability with caution; it is intended to allow maintainers to help with the maintenance of the patch list. We trust your judgement e.g. marking patches as committed if you see a commit for it, and marking patches under review that you are reviewing.
We support 2 CLI interfaces and 1 web interface. The web interface is at http://patchwork.sourceware.org, and the patch queue can be seen directly here: https://patchwork.sourceware.org/project/glibc/list/. The two supported CLI interfaces are git-pw https://github.com/getpatchwork/git-pw and pwclient https://github.com/getpatchwork/pwclient.
To configure git-pw you should setup token-based authentication for the REST API login. To do this you login to patchwork, select your name in the top right, select View Profile, and under Authentication generate the token. Open your ~/.gitconfig and add the authentication like this:
[pw] server = https://patchwork.sourceware.org/api/1.2 token = XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX project = glibc states = new,under-review,accepted,rejected,rfc,not-applicable,changes-requested,awaiting-upstream,superseded,deferred,committed,dropped
Where the XXX... is the token for authentication. The states are required because the project uses custom states. With this saved to ~/.gitconfig this will be the default project and states for git-pw unless a git local configuration overrides this. You may have multiple projects using patchwork, and so you can configure this on a per-repo basis.
When you commit your patch you can automatically update the patch in patchwork like this:
git-pw patch update --commit-ref 4cab20fa49b3ea3e3454fdc4f13bf3828d8efd19 --state committed 38972
If you want to pull down a patch (whose ID you know) and review it you can do this:
git-pw patch apply 38972
You can work directly with a series and apply all patches in a series:
git-pw series apply 162
Reviewing a series
Patchwork allows you to review a series of patches, and you can quickly download and apply a series of patches like this:
git-pw series apply 359
Once you have the series downloaded you can use git's difftool feature to do higher level review like this:
git difftool -t meld -d HEAD~4..HEAD~3
If the series had 5 patches, and you diff HEAD~4..HEAD~3 you will be reviewing the 2nd of 5 patches. You can repeat the difftool invocation changing the number to review all patches in the series. This makes it easy to quickly review high-level changes at a glance with the local diff tool of your choice. In this example we use meld as the suggestion which is a good side-by-side difftool that gives full file context with colouring during the review. The use of -d allows you to see a full directory diff and see where files were deleted and created at a high level for review.
Following are the main points of the workflow:
- The subject line of the email that includes the patch must accurately reflect its content. The subject line is what appears automatically on patchwork and having an ambiguous or inaccurate subject line will only result in your patch being lost in the noise.
A reviewer may pick up a patch by delegating it to themselves by: following the link to the patch and changing the Delegate to value to their patchwork username. Do not change the status of the patch since it will then disappear from your default view. The patch would still be view-able in a list by modifying the Filters on the list.
- The following states may be used once the patch has reached a resolution or is being actively worked on by the submitter and one or more reviewers:
New - Nobody has reviewed it yet. This is the default status.
Under Review - Someone is looking at the patch. One may alternatively use the Delegate to field to indicate that they are interested in reviewing the patch.
RFC - Mark patches that aren't meant to be included as is and are posted as a proof of concept or just to solicit comments on the approach.
Accepted - There is consensus for committing this change.
Rejected - There is consensus that it's a bad idea.
Dropped - The patch was rejected and the developer dropped it.
Superseded - A newer version of the patch exists.
Deferred - Review of this patch is waiting for another event to happen.
Change Requested - Reviewed and needs rework.
Committed - The change has been committed.
Not Applicable - The patch was cross posted and is not applicable to the project.
When the status of a patch is Change Requested, a submitter may post an updated version e.g. [PATCH vN ...] of the patch and mark the older version as Superseded.
- The submitter is still responsible for pinging the patch regularly if there have been no responses.
- Once review is done and the patch is pushed, the committer of the patch is responsible for changing the status of the patch.
Lifecycle of a patch
New patch submitted: New
A reviewer picks up the patch for review: Under Review (Note that the reviewer may also just use the Delegate to field so that the patch remains visible to other potential reviewers)
Reviewer thinks patch is OK: Accepted
Reviewer thinks patch is OK, but needs another opinion: New so that the patch becomes visible in the queue again
Reviewer thinks patch is OK, but another reviewer disagrees: Under Review
Reviewer thinks patch is not OK for glibc: Rejected
Submitter drops the patch: Dropped
Reviewer thinks patch needs changes: Change Requested
Patch committed: Committed
Submitter has posted an updated patch, then the old patch status: Superseded
Any maintainer thinks that the posted patch is an RFC and not an actual patch: RFC
Any maintainer sees a cross-posted patch not for glibc: Not Applicable
The state chart below should give a clear idea of the patch lifecycle. States marked in green are considered final states, i.e. patches in those states will not be visible in the default view.
Notes to developers
When working on your own patches and using patchwork to maintain a list of actionable patches it is often useful to review rejected patches to determine which need more work using a different solution. The problem with this is that reviewing your rejected patches to see which need work gets harder and harder as the list gets longer and longer. There is a need to distinguish, for the developer only, the difference between rejected and dropped versus rejected and needs more work. If you are dropping the patch mark it with the Dropped state, otherwise keep it Rejected if you are going to do more work on the patch. Neither the Dropped or Rejected states are actionable and don't show up for reviewers looking to review patches.