Review a patch

Reviewing a patch consists of two steps:

  1. Code-review
  2. Testing the change

You have the option to contribute to both stages or just a single stage in the review process.

If you’re able to improve the patch yourself, your contribution would be very much appreciated. Visit Upload a new Patch Set to find out more about how you can help improve patches.

Code Review

A basic code review is possible by using the Gerrit web interface.

  1. Select the latest patchset

    This should already be selected by default, but if you changed the patchset, you should select Go to the latest patch set (above the list of changed files).

  2. To leave a comment, click on one of the files

    ../_images/gerrit_review_click_on_file.png

    Commenting in the files directly is optional, but strongly recommended. If you just want to leave a general comment which needs no context, you can skip to step 8 (Reply).

  3. Optionally change the view

    You will see a diff of this file against the current codebase.

    You can change the view in the top right (Diff view) e.g. side-by-side or unified (or change this in your settings).

  4. Leave one or more comments

    Click on the line number in the file and a comment box will open.

    ../_images/gerrit_review_comment_click.png ../_images/gerrit_review_comment_file.png
  5. Click Save.

    Important

    You comment(s) will not yet be visible for others until you go back and click Reply.

  6. Go back

    ../_images/gerrit_review_go_back.png
  7. (Optional) Add comments to more files.

  8. Now press Reply.

    Using the Reply button, you can post your comments (and optionally add an additional note).

    ../_images/VoteUser.png

    Vote by clicking the Reply button

    Of course you should also vote for the change (Be graceful with -1 votes though).

Test a patch

For testing the patch you need to import the change into your local repository.

Look at Cherry-pick a patch for information on how to do this.

Test the patch in your local TYPO3 installation and verify the reported bug is fixed and no other bugs are introduced with the change.

Depending on the outcome of your tests, place your positive/negative vote in Gerrit, using the Reply button.

If you want to help the author and provide an improved patch, continue with the section Upload a new Patch Set.

Otherwise throw the changes away, to bring your repository back to a clean state:

git reset --hard origin/master

Vote

In order to comment or vote on a change you can click on the Reply and enter your comment. Here, you can also apply your votes.

../_images/VoteUser.png

Chose your vote, say something nice and click SEND

  • +1 : you approve of the patch
  • -1 : you do not approve, in this case give your reason as a comment

Click on Send and your comments will be saved. At the same time all other contributors who either watched this change or have already voted on this change will get notified.

Policy for votes

Code Review: Needs +1 of two reviewers, one of them being a Core Merger.

Verified: Needs +1 of two reviewers, one of them being a Core Merger.

Votes from the Bamboo build server (user TYPO3com) do not count. This means that a patch which is fully reviewed usually has at least 3 Verified +1 votes, two from humans and one from Bamboo.

Authors should not vote for their own patches, unless the patch has been changed substantially by other developers.

As soon as the patch has reached the approved status by getting a +2 on Code Review and Verified, a Core Merger can decide to push the Submit button, finally pushing it to the main repository.

Hints on voting -1

See also Review a patch about the policies on voting and how to vote.

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 logical 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 pinpoint 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 ask person x or person y, who has a deeper knowledge of this subsystem, to take a look at it. I do not want this patch to be merged until this is sorted out and will vote -1 for now for this reason.”

How to handle [WIP] patches

It is possible to push patches as “WIP” (work in progress) patches. This 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 your patches evolved.