Review 101

[yvi]

Hello and welcome to today's feature on dw_dev_training. I'll be your host today and we will be discussing:

Review 101
Or: How to get that queue a tiny bit smaller

[Note: There is also a Wiki page on code reviewing http://wiki.dwscoalition.org/notes/Code_Review ,which you might want to read. I will likely expand that page soon]

part 1: Why review?

Review? Why would I do that? It looks hard! I don't know enough for that! It doesn't help me!

Three reasons:

• Other people will love you. No, seriously, there are lots of people who will want to kiss you if your review something. a) if you review their stuff, they will be very grateful (keep in mind that people aren't allowed to review their own patches), b) if you review someone else's, that's one less patch in the queue that someone else needs to look at. Which brings us to the next point:


• The less time people spend on reviewing other people's patches, the more time they have to look at yours :)


• You learn something doing it. Reviewing a patch takes a lot les stime than writing it in most cases and still you learn something about various parts of the code doing it.

And there are probably half a dozen more. I find it relaxing and rewarding after i did some coding myself.

part 2: Who can review?

Have a Dreamwidth development environment, for example a Dreamhack?

Yes? Then you can. For the formal part, you will also need a Bugzilla account. Most of those who have a development environment probably already have one. If not, you can set up an account there in less than two minutes.

That's all. Don't be afraid of reviewing - just take on the patches you feel comfortable with and a committer will always double-check for anything bad anyway. And letting mistakes through is really not the end of the world. And if there isn't a patch for you today, there might be tomorrow.

part 3: So, how do review?

You can find the current review request-queue here.

Usually, you can just scroll down a bit. The ones on top that have been there a while are usually there for a reason (CLA not yet submitted or needs to be looked over by mark). I say usually because that's not always the case, but mostly.

Anyway, look at the bug titles you will see in the third column. Maybe, hopefully something will catch your eye. Or pick one at random.

And now, a step-by-step on how I tackle a review and then two short examples.

Warning: This is just what I think and how I do it - other people might have different opinions or do things differently and cringe at what i write

Step One: Read Bugzilla entry Quite straight-forward, I think. See whether you understand what this Bugzilla entry is all about, already try to imagine what the end result would look like. Are there any comments that tell you something more? Did the patch uploader leave a comment at the time of uploading explain something?

Step Two: Look at patch Just clicking on the attachment, you can sometimes already tell whether you maybe don't want to review this after all. 1,000 lines patches are usually out of the scope of a new reviewer. But a few to a few dozen lines can definitely be done!

I then look at what files this patch touches. Some files, like core2.s2 can be scary at first, while others, especially the .bml files are much easier for someone not that familiar with the code.

Then I do a cursory look-through to see whether I understand to see what this patch actually *does*. For that, I might look at the complete file on my Dreamhack and follow what is being done now vs. what is being done in the new version.

At this stage, you can also discard bugs you *can't* review. Most people don't have Memcache running on their Dreamhacks, so that can't be adequately tested, for example..

Step Three: Style review The Programming Guidelines are posted on teh DW wiki. It is easiest to just copy the patch text into a text file for this. In my text editor (kate), for example, I just check for trailing whitespace by searching for " " and seeing whether anything that is highlighted shouldn't be.

Except for whitespace, the main things are: logic (postfix conditionals and using unless instead of if not), proper use of brackets and hashes, and English-stripping, if that is needed.

Does the patch fail on any of those? Double-check and note it down. If possible, don't negatively review yet - something like that can be fixed at the time of commit and it is much better for the dev person if they also get comments on whether the patch actually *works*. Having a patch rejected, changing one postfixconditional, resubmitting and then hearing that it doesn't work anyway is not that great and means the patch won't be committed for a few more days.

Step Four: Build a Test Case After that, I log into my Dreamhack. To seee the latest version of the code, I first need to update. I am only familiar with Dreamhacks, so this is DH-specific. First I see whether I still have some changes made to my DH.

dh-yvi@hack:~/dw$ perl bin/cvsreport.pl -d

Oh, I have. So I put that into a file and back it out to reapply after I am done with reviewing.

dh-yvi@hack:~/dw$ perl bin/cvsreport.pl -d > ../activeentries.txt
dh-yvi@hack:~/dw$ patch -p0 -R < ../activeentries.txt

dh-yvi@hack:~/dw$ perl bin/cvsreport.pl -d now comes up empty.

Then I update my DH to the latest code if I haven't already. I do that with the "dwu", "dws" and "dwdb" from this page: http://wiki.dwscoalition.org/notes/Dev_Maintenance . You might use other methods.

Afterwards, I log into it on the web. If the patch is a bugfix, I see how I can reproduce that bug. So I don't have to back out the patch and reapply it a few times, I try to think of stuff to do: it's hard to generalize, but some ideas for what to think of:
* communities versus personal journals doing something, maintainers versus members in communities
* anonymous/OpenID/registered users doing something
* different styles and siteschemes if the patch touches layout-y stuff
* screened, frozen, and deleted comments
* account levels
* security levels

You might get the idea :)

Step Four: Applying the patch Now the fun starts. We need to transfer the patch to the Dreamhack. Personally, for applying the patch I just copy and paste it. I just open a new file on my Dreamhack, edit that and copy in the code from the patch, then save.

After that, apply.

dh-yvi@hack:~/dw$ patch -p0 < patchname.txt

Now if that somehow doesn't succeed, you can probably go back and make a comment saiyng "looks good, but didn't apply, sorry" or something like that and set the "review" flag to -. If it only mumbles a bit about "hunks", that's fine. Something else in the file was changed in the meantime, so the line numbers are a bit off. Nothing dramatic.

If the patch touches anything in /dw/bin/upgrading , you need to re-compile the database afterwards. If that fails, well, see above.

But if everything goes well, you can have fun.

Step Five: testing Go back to your browser and test the hell out of this patch. Try to break it. Really, be mean and try to break it [but remember the scope of the bug - some things might be unrelated or just your personal preference - don't do a negative review based on "this should have also included", that can be a separate bug].

There's not a lot to say about this step. If you come across anything funny, try backing out the patch again:

dh-yvi@hack:~/dw$ patch -p0 -R < patchname.txt

and see whether that already happened beforehand.

Step Six: Reviewing

So, at one point you decide you have reasonable tested the patch. It worked or didn't work, either way you can now finish the review. You now have three options:

* review+: The patch's syntax was fine, it applied without problems and did what it was supposed to do. Change flag on attachment from ? to + and submit.
* review-: The patch had grave mistakes, never applied, broke your Dreamhack or didn't fix the issue. Change flag on attachment from ? to -, comment with your reasons and submit.
* none of the above: the patch had minor style problems, left out a very rare case, or anything where you are just not sure. In these cases I prefer to just leave a comment explaining what I mean. Minor things might be fixed by a committer and they will thank you for the comment, or someone else can do the review and profit from what you think. Other people might do a review-. For other things (like a trailing whitespace), one might do a review+ and note it down so. It really depends :)

Congratulations, you are done.

part 4: Examples

If you made it through this novel, don't be frightened off. It's a lot less time-consuming than it sounds. let me give you a brief overview on how I would do two review:

1

http://bugs.dwscoalition.org/show_bug.cgi?id=1707 : Basic Boxes Green has two color_module_titles has a patch by kareila. From the summary, it is quite easy to understand the scope. Somethng in the code has an unwanted duplicate.

Looking at the patch, it also looks simple enough. There's two lines with "set color_module_title =" and one of them is simply removed. basically, I am going to test whether the color is set to the correct value after this patch. The syntax is, of course, perfect :)

So I log into my Dreamhack as a user and change my journal layout to "Basic Boxes Green". color_module_title is responsible for the module headers, like "Profile" and "Most Popular Tags". From http://www.yvi.hack.dreamwidth.net/customize/options?group=style I can see that it is currently set to "#e9eed3". That's the value that stay in the patch, so it shouldn't change when I apply the patch.

I make a new file on my Dreamhack:

dh-yvi@hack:~/dw$ vim ../boxesgreenmodulecolor.txt

Then I copy&paste the patch into that file, save and apply.
dh-yvi@hack:~/dw$ patch -p1 < ../boxesgreenmodulecolor.txt
patching file bin/upgrading/s2layers/basicboxes/themes.s2
Hunk #1 succeeded at 199 with fuzz 2 (offset 165 lines).

That#s all fine. Now a quick 'dwdb' and the database is re-compiled.

Then I just hit 'refresh' and yes, the color stays the same. It's also still the same in the customization wizard. That#s pretty much everything i can test here.

dh-yvi@hack:~/dw$ patch -p1 -R < ../boxesgreenmodulecolor.txt
patching file bin/upgrading/s2layers/basicboxes/themes.s2
Hunk #1 succeeded at 199 with fuzz 2 (offset 165 lines).
dh-yvi@hack:~/dw$ dwdb

gets my Dreamhack back to its original state.

Now I go to http://bugs.dwscoalition.org/attachment.cgi?id=2169&action=edit and set the review flag from ? to + and submit that change. That was it. And 5 minutes later, afuna commited the patch :)

2

http://bugs.dwscoalition.org/show_bug.cgi?id=1648 : FAQ preview escapes HTML has a patch from... me. So I won't actually review it here. But it'a nice way to explain how to test a patch that might at first be a bit of a mistery.

denise opened this bug report saying The FAQ preview function escapes HTML rather than interpreting it, so you don't actually see the FAQ the way it will display.

The patch itself: http://bugs.dwscoalition.org/attachment.cgi?id=2154 only touches one line in one file, htdocs/admin/faq/faqedit.bml . Style seems good, but how does one test that? Hands up everyone who has ever used that file.

But that doesn't mean you can't test it.

What I'd do is logging into my admin account on my Dreamhack and then going to "http://www.yvi.hack.dreamwidth.net/admin/". There's an option called

FAQ Tools (faqadd, faqedit, faqcat)
Allows you to manipulate the FAQ.

Ah, faqedit! *clicks* And then there's an 'edit' button! *clicks*

And yes, we are now actually on "http://www.yvi.hack.dreamwidth.net/admin/faq/faqedit", which is the file edited in that patch. The Bug is about the preview function, so let's try that. type something with HTML into one of the boxes and click on preview and you will see the problem, the escaped HTML. That should be changed to display how the actual FAQ will look later.

So now, a testcase. I'd say varied HTML in various places in that case. Links, text style, user links and images should suffice, though.

Then, same as above, copy patch over and apply it. In this case, there's no database to compile, and it doesn't even require re-loading, just push the 'preview' button again.

And voila, the HTML isn't escaped anymore.

I won't dive into the patch any more, though, since I can't be absolutely sure it will actually be positively reviewed. I just hope you get the idea :)

---

Sorry if this was short in places or unclear, I wrote this down rather quickly and my fingers are starting to hurt. Comments welcomed, as I would love to post a finished up version of this on the Wiki.

And I hope this helps someone :) Reviewing is nice, needed and not that difficult!

And nobody will kill you if you do something wrong. Really! For any questions, the Dreamwidth channels #dw and #dw_kindergarten are also welcoming of questions regarding reviewing.

Comments

[kareila]

Thanks for describing how to review your FAQ preview patch! ;)

[wyntarvox]

Thanks for this! I wasn't aware that anyone who is willing is welcome to review patches.

[cesy]

You said "If the patch touches anything in /dw/bin/upgrading , you need to re-compile the database afterwards."

How do I re-compile the database? Is it
$LJHOME/bin/upgrading/update-db.pl -r -p --innodb
$LJHOME/bin/upgrading/update-db.pl -r --cluster=all --innodb
$LJHOME/bin/upgrading/texttool.pl load
as in this wiki page?

[yvi]

Yes, exactly.