Gerrit Review Workflow
warning - Message
- the already moved information has been deleted. The remaining text needs to be migrated to the contribution guide
- 1 Best practices, TYPO3 CMS team working modes and how to survive in reviews
- 1.1 Basic considerations for contributors
- 1.2 Hints for reviewers and active contributors
- 1.2.1 Act especially helpful to newcomers
- 1.2.2 Do not merge hasty after pushing a new version
- 1.2.3 Don't be scared of pushing new patch sets
- 1.2.4 First, think about the solution itself and if that is ok, fix nitpicks
- 1.2.5 Fix on your own instead of -1 for CGL issues
- 1.2.6 Hints on voting -1
- 1.2.7 Using -2
- 1.2.8 Using [WIP]
- 2 Historical review
Best practices, TYPO3 CMS team working modes and how to survive in reviews
This chapter is dedicated to the TYPO3 CMS core patch reviews and sheds some light on the working modes and things to keep in mind when improving the code base with patches through the review system. It is dedicated to everyone working with gerrit and tries to give some hints on how not to get frustrated if something does not happen as expected, eg. if things are not merged quickly. Also, some hints for reviewers and active contributors are given on how to behave in a friendly way to not destroy other peoples motivation.
Basic considerations for contributors
You are a fresh contributor. You found an issue in the core you wanted to see fixed, you fought your way through the code to find a solution, you are satisfied with it, you did all this git stuff, you read about gerrit workflow, you figured how to push to gerrit, you managed to create a forge issue and you finally pushed your patch! That is so great!
Now your patch is hanging around in the review queue and either nothing happens, or it is voted down, or people tell you your patch is not good enough.
Don't despair young padawan! The community around TYPO3 tries to act friendly and helpful, we're not hurting anyone on purpose. We're open to anyone and especially like to see new contributors and try to help getting them onboarded and up to speed. Please consider the following points before going mad:
- Most of the time spend on TYPO3 CMS core development is free and not payed. This gives contributors and reviewers freedom on what they work on. Unfortunately, this also means that sometimes a patch is not given the dedication it should receive.
- The review queue is often longer than the review power available. Sometimes patches are lost in the queue and are forgotten just because there is other hot stuff being worked on.
- Parts of the team usually work on something bigger. It may happen that some patches in the hot areas are merged very quickly while others get stuck. This is somehow natural since at any given time some reviewers usually focus on certain areas to get them right, so there is no time for everything else.
- Sometimes the whole team focuses on different stuff. For example, usually around "big" releases, the team focuses on bug fixing, so feature patches may get stuck for some time.
- Sometimes large parts of the active contributors are inactive at the same time. There are for example main holiday sessions with low overall activity.
- Sometimes getting little or no feedback on patches also has technical reasons. Maybe the issue description is unfortunate or not understood, maybe there is no documented way on how to reproduce a certain issue, or the bug or feature is bogus, or the issue is so complex and hard to solve that no one really wants to get hands dirty on it.
Here are some options you can take to raise awareness and get more feedback on your patch and to help getting it merged:
- Raise awareness: You are allowed to "ping" a patch once in a while. For example, you could add a comment "Hey, what is the status here, how to proceed with this one. I'd like to see this solved" once a week. This will raise the issue to TOP 1 in the queue and increases your chance of feedback. In general: Ask what is wrong, raise awareness, ask what is missing.
- Ask for help: It may be helpful to get in closer contact to the active contributors. Sometimes, issues can be solved in a better way if they are coordinated in direct chat. The TYPO3 CMS team uses "slack" as instant communication platform. You are always welcome to join #typo3-cms channel and ask for direct help on your pending patches. Please see this page for more information on how to join slack. Joining slack usually helps sorting out things and gives a much better feeling on the working modes of the team and what is going on.
- Update your patch: It happens quite often that the first version of a patch is not merged directly and needs a couple of patch sets before it is finalized. In fact, merging "Patch Set 1" is the exception, and having something merged in less than an hour is rather seldom. We're trying to do things right, sometimes patches go through a whole lot of patch sets and evolve in gerrit until everyone is satisfied. This can take some time. You can help with amending your patch and pushing new patch sets after feedback came in. If you're not sure on how to do that, please see the documentation or join slack and ask for help.
- Act helpful and friendly: Chances are higher to get your stuff in if your way of communication is fair. This is even more important for reviewers and active contributors, we do not allow ranters and haters in our team and we try to stick to our code of conduct. Please keep that in mind when working with us. You will be rewarded with the pleasure of working with a group of pretty smart people if you act accordingly.
- Don't get frustrated by -1 votes: Voting a patch down is a natural process in the review system and is not a sign that we hate you, your patch or your kitten. Our goal is to only merge things that are verified to be ready to go. A -1 basically blocks a patch from merging for a reason, and reviewers always add information on what is wrong or missing. Everyone wants to improve the system, so a -1 is just one method to achieve this.
Hints for reviewers and active contributors
First, everyone is allowed to vote positive and negative on pending patches in the gerrit review queue for the TYPO3 CMS core product. We appreciate good reviews on both code review and testing and this entry point was used by quite some people already who were later nominated to the active contributor role. Frequently giving good reviews and improving patches is noticed and rewarded and the team recognizes new helping hands. Another good entry point is joining code sprints, look out for some if you have ambitions to be involved in core development.
Acting as a reviewer and especially as an active contributor gives some additional responsibility in dealing with the project and working with other people in the review system. While technical discussions are sometimes mastered with different opinions clashing each other, it is always important to be respectful and fair to each other. There is no reason to act rude just because some other reviewer has a different view to a specific point.
That said, now some common guidelines and practical hints on reviewing habits.
Act especially helpful to newcomers
TYPO3 CMS is a living product and people join and leave the project. It is important to welcome fresh blood helping them going up to speed. Not every effort in this area will be successful, but we must not scare away people just because someone did something wrong with their first patches. Help getting patches right: Fix commit messages, push new patch sets, add forge tickets, friendly hint about problems with the current state of patches.
Do not merge hasty after pushing a new version
Reviews of the final merged patch version are mentioned in the git commit message, reviewers of patches get karma by having their name in the core commit log. This can be a nice motivator, so it is good habit to wait for re-vote or to hint people for a re-vote if a last version of a patch was pushed and the patch is close to be merged. Some pages count successful votes on patches and have metrics for persons being active in the project based on final reviews mentioned in the git log. Give them a chance to get their review points, it is great to see those numbers.
Don't be scared of pushing new patch sets
There are two different information on a merged patch set: The author of a patch and the committer. Usually, the author of a patch is the person who pushed the first patch set to gerrit, while the committer is the person who finally pressed the merge button. When pushing new patch sets, the author information is usually not overwritten, so you do not "steal" the authors name by pushing new patch sets. However it *is* possible to explicitly overwrite the author name with its own name, but this is used seldom if a patch changed so heavily that the person who pushes a new patch set says "This is now my patch since it has nothing to do with the initial version anymore". Usually, it is a good idea to keep the original author name when updating a patch. You do not need to do anything for this if a pending patch set from gerrit is cherry-pick, amended and pushed in to keep the original author information.
First, think about the solution itself and if that is ok, fix nitpicks
Core patches must follow our general coding guidelines to get maintainable, readable and quickly understandable source code. In general, patches are not merged before the CGL are followed. However, if looking at patches, it is important to first answer different questions:
- Is the issue itself relevant?
- Does the solution embed well in current development strategies?
- Is the architectural solution ok or is the patch just fixing some symptom instead of a different, maybe bigger problem?
- Is the solution contrary to other things we have in the same area?
So, first it should be decided if the architectural solution is fine. If that is the case, the patch can then be optimized towards CGL needs. It happened sometimes that a first patch was pushed, and some reviewers immediately started doing nitpicks on CGL stuff, leading to tons of new patch sets until the patch itself was codewise clean. After that, someone else showed up, looked at the real issue and came to the statement that the solution itself is bogus. Such things can easily kill motivation: Having a patch in the review queue, updating it because of CGL stuff and later being voted down because of systematic concerns about the solution after lots of energy was already put into the patch. It should be the other way round: First align if a solution itself is accepted, and if that is clear, do details like coding guidelines and minor improvements.
Fix on your own instead of -1 for CGL issues
As said, we only want code merged that follows CGL. However, voting -1 because of simple CGL violations can easily scare away people and kill motivation. Therefore it is a very appreciated habit to not vote -1 because of CGL issues, but to cherry-pick those patches, fix those issues and to push again, instead.
This has several positive side effects: A trained reviewer is used to quickly cherry-pick, amend and push new patch sets anyway, so reading and fixing at the same time usually takes less time than voting -1 and forcing someone else to read through comments, apply and push again. And it will kill less motivation for the patch committer who will not be forced to push again, wait for reviews, push again, and so on, "just" because of CGL issues.
A friendly reviewer just fixes those issues on its own an pushes a new set, then does the real review: "Hey, nice patch set. I like it and it works. Just pushed a new version fixing minor CGL stuff. Would be cool if you read through my changes compared to your version and if you try to follow those in the first place next time. But your fix itself is great, I verified it and it works. +1 for that, thank you!". Gives better karma, right?
As usual there are exceptions to those rules. Sometimes a reviewer has no time to do those CGL fixes and decides to vote -1 instead, or maybe a patch is codewise so ugly that is does not make much sense to put energy into that. This is ok, just keep in mind that in general we appreciate minor issues to be fixed by the reviewer directly.
Hints on voting -1
In general, -1 on reading and/or testing of a patch is a mechanism used to improve a patch. Still, -1 still takes a risk to kill someone elses patch and it usually actively prevents a merge. There are ways to override a -1, but those are not pushed through in real live gerrit habits. In general, if voting -1, you take some responsibility for this patch by saying "This one shouldn't be solved until this or that is fixed". Some hints on using -1 in reviews:
- Think about your vote and always give a sane explanation. "-1 looks ugly" is not enough.
- If a patch is broken, does not fix the issue, is bogus, architecturally wrong or collides with other goals, a -1 is clearly ok.
- -1 can also be used if you are actively working on a patch and want to prevent a quick merge: "-1, working on it now, will push soonish".
- -1 may be ok if you have general doubts but you can not pin point it and need a second opinion: "Hey, this solution looks somehow weird and I doubt this is what we should do here. I think we should have a statement by x or y who have a deeper knowledge of this subsystem to have an eye on that. I do not want this patch to be merged until this was sorted out and will vote -1 for now for this reason."
A -2 vote by an active contributor blocks a patch from merging. In contrast to -1, a -2 persists new patch sets is a "veto". Use with care. With a -2, you are taking responsibility of this patch and basically state that it will not be done until you actively removed your vote again. In the past, we usually had no real problem with someone giving -2 and then not acting responsible. It would be great if it stays this way.
Prefixing your commit message with a [WIP] (work-in-progress) in the title is a way to show people a quick-shot version of something that is not finished yet, but goes into the right direction. Usually, WIP patches are not actively reviewed by others and the original author should take responsibility to finish this patch later.
As a contributor, you usually can not expect someone else to pick up your WIP patch and finish it, except you stated that goal clearly: "Hey, I've done a quick shot with this patch to show a possible solution for this issue, but the patch is not finished yet. Foo and bar is missing and we still need a concept for foobar. I'll probably not work actively work on it anytime soon, but maybe the current state is helpful already".
Better: "Hey, I worked in this area and came up with this WIP patch for now. I wanted to show into which direction this patch is leading, but we currently have some open questions. However, it would be great if you can give me feedback to the general approach at this early state already to decide if it is worth following this solution to its end."
Having too many WIP patches in the review queue is not really helpful. Consider to fork the project in github or somewhere else and push to gerrit again if you patches evolved.
Before Git and Gerrit, the TYPO3 CMS core team used the "core mailing list" as a tool for reviewing patches in form of "RFC" (request for comments) or "FYI" (for your information). Git was introduced on 1st of March 2011 and replaced the workflow using the core mailing list. The old policy was adapted to the Gerrit system, as shown above. The old policy was that a RFC required:
- 2 reviews by reading (now "reviewed" in Gerrit), one of them being a core developer
- 2 reviews by testing (now "verified" in Gerrit), one of them being a core developer