(see also [wiki:Patches How To Submit Patches]) = Why review patches = We want more patches to be contributed to Tahoe-LAFS. Patches languishing in the "waiting to be reviewed" state discourages contributors. Getting feedback on patches encourages them. (By the way, something else that encourages them is users saying "Thank you.".) = Who can review patches = Pretty much anyone reading this! Knowledge of Python is helpful, but some patches are so simple that reviewing them is a reasonable task for a beginner who is learning Python. Some patches require more specialized knowledge to review, but most don't. = How to review patches = Here is the overall process for patch review. For technical tips, see below. 1. Go to http://tahoe-lafs.org . Click on "[http://tahoe-lafs.org/trac/tahoe/wiki/ViewTickets View Tickets]". Click on "[query:keywords~=review-needed&status=!closed&group=milestone review-needed]". 2. You can read everything without registering, but to add comments or change tickets you have to be logged in. Registering is easy -- click the "Register" link at the top right of the page. 3. Read tickets until you find one that you can review. 4. (optional) Expand "modify ticket" and click "accept", then submit changes. This marks you as the person reviewing this patch. If you don't want to commit to this then you can skip this step. 5. Read the patch until you understand the docs, tests, code and comments in it. You can use the "Browse source" button at the top of the page to read the current versions of the files that the patch changes. a. If you can't understand the patch after spending some time on it, then say so in a comment on the ticket! This might mean that we need to add documentation or comments or to refactor the code. On the other hand, it might just be that you don't have enough context to understand the code. That's okay too, so go ahead and speak up. b. Check whether every feature or bugfix in the patch has an accompanying test in the patch. c. If you find errors or omissions in the docs, tests, code or comments then write that down in the ticket, remove the "review-needed" keyword from the keywords, and assign the ticket to someone other than yourself. (Assign it to the original author of the patch, or someone who seems likely to fix the patch, or "nobody".) d. If you understand the patch and find no errors or omissions then write a comment on the ticket saying that you reviewed it, remove the keyword "review-needed", add the keyword "reviewed" and assign it to someone with repository write access (currently 'zooko', 'warner' and 'davidsarah'). We'll commit it to trunk. e. Feel good about yourself. Thank you for helping with our little project attempting to improve the world! = Design reviews = To request a design review, explain your design (with or without a patch) and then add the [query:keywords~=design-review-needed&status=!closed&group=milestone design-review-needed] tag. Completion of a design review will normally not directly result in a patch being committed. The main goal of a design review is to give the person working on the ticket confidence that there are no show-stopping issues with the approach they are taking, and to get feedback on smaller issues that are useful to take into account before doing further work on a patch. = Advanced = Once you decide that you like reviewing tickets and you get into the habit of doing it frequently, then read this essay by Jonathan Lange on how to do it well: [http://mumak.net/stuff/your-code-sucks.html Your Code Sucks and I Hate You: The Social Dynamics of Code Reviews]. Two simple suggestions: 1. Say something nice about the author or their code. 2. You don't have to be extra picky. = Technical Tips = Please suggest improvements here if you run into technical snags while reviewing a ticket. == Ticket attachment links == Attachments in a ticket have two links, a name, which points to an html page with prettification, and a raw download link. You can save the download link (such as by {{{wget}}}) and use {{{darcs apply}}} to configure your repository to test a particular patch. For the #1149 ticket, for example, there are two links for the attachment named "test-for-webopen.darcs.patch": * The link with a name points here: http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1149/test-for-webopen.darcs.patch * The link with a download icon, here: http://tahoe-lafs.org/trac/tahoe-lafs/raw-attachment/ticket/1149/test-for-webopen.darcs.patch The latter can be used to apply a patch, like this example shows: {{{ $ wget 'http://tahoe-lafs.org/trac/tahoe-lafs/raw-attachment/ticket/1149/test-for-webopen.darcs.patch' --2011-07-31 20:23:39-- http://tahoe-lafs.org/trac/tahoe-lafs/raw-attachment/ticket/1149/test-for-webopen.darcs.patch Connecting to 127.0.0.1:8123... connected. Proxy request sent, awaiting response... 200 Ok Length: 18275 (18K) [text/x-diff] Saving to: “test-for-webopen.darcs.patch” 100%[==============================>] 18,275 21.8K/s in 0.8s 2011-07-31 20:23:54 (21.8 KB/s) - “test-for-webopen.darcs.patch” saved [18275/18275] $ darcs apply test-for-webopen.darcs.patch Finished applying... }}}