Marking this as resolved. One thing I don't like about the bitbucket reviews
I've been involved in is that they leave no trace on the tracker, so the
discussion there seems to be lost except to the people who were there to see it.
Just something to consider in the future (especially since there have more than
a few cases already where I said something in these code reviews that was
ignored without further comment, which is quite frustrating since it's hard to
find time for these code reviews).

Since both bitbucket and rietveld proved suitable for our code reviews, I think 
we can close this issue, right?

I added a section about Bitbucket code reviews at http://www.fast-
downward.org/ForDevelopers/CodeReview

I've asked Martin von Löwis about how they did the Roundup/Rietveld integration
for the Python project. He pointed to this:

http://svn.python.org/projects/tracker/instances/python-dev/rietveld/
http://svn.python.org/projects/tracker/instances/python-dev/rietveld.wsgi

...which in turn builds on:

http://code.google.com/p/django-gae2django/

The integration should be simpler for us because we can work around the main
problem they have in the Python codebase, which is to find out which revision a
patch attached to an issue is based on. In our case, we would (usually) not
attach patches to issues manually, but use an hg plugin to do that work, which
could automatically do the necessary behind-the-scenes work to set up an
appropriate Rietveld issue.

Sounds pretty straightforward at least in theory.

Here's the link without the period, which Roundup unfortunately considers part
of the URL: http://www.fast-downward.org/ForDevelopers/CodeReview

I've made some first experiments with Rietveld and wrote the info on the wiki to
conserve it in more readable form:
http://www.fast-downward.org/ForDevelopers/CodeReview. Please check it out.

I've also tried out the process to create a Rietveld issue from Erez's latest
work on issue122. If you have some time to play around with it at
http://codereview.appspot.com/3110041/ and add some comments (they don't have to
be topical ;-)), this would be good. You'll need a Google account, but you
already have one anyway. (You can use the same one you use for Google groups.
I'm using my regular uni-freiburg.de address.)

OK, I'll look into this then. One question is whether we want to run our own
instance of the code review tool or use the existing installation run by Google.

Using our own instance would of course be nicer because we don't rely on anyone
else and could also potentially reuse the login credentials from the tracker
rather than having an additional set of logins (although I'm not sure how
difficult that would be). The disadvantage is that it might be significant work
to set up and maintain. (AFAIK, Rietveld is based on Google AppEngine, and I
think that's fairly heavy-weight.)

For now, I'd suggest using the existing installation on Google and see how well
that works for us. I'm very open to running our own instance if someone else is
willing to help me set it up.

Nice. Let's use it.

Agreed

On Sun, Nov 14, 2010 at 7:11 PM, Jendrik <downward.issues@googlemail.com>wrote:

>
> Jendrik <jendrik.seipp@mars.uni-freiburg.de> added the comment:
>
> Looks nice. I think the inline comments are very helpful.
>
> _______________________________________________________
> Fast Downward issue tracker <downward.issues@gmail.com>
> <http://issues.fast-downward.org/issue148>
> _______________________________________________________
>

-- 

--------------------------------------------------------------
"Adventure is just bad planning."
    Roald Amundsen
    Norwegian Arctic & Antarctic explorer
    (1872 - 1928)
--------------------------------------------------------------

Looks nice. I think the inline comments are very helpful.

Reviewing patches is currently quite tiresome because the reviewer has to write
stuff like "In method X of class Y, you write Z, which I think should be W." It
would be much nicer to have a visual diff of the patch where the Z in method X
of class Y could be annotated directly.

This can be nicely integrated into the issue tracker: once you've fixed an
issue, run a script that uploads an appropriate patch (or sufficient metadata to
fetch the patch from your repo) to the issue tracker. The issue tracker then
gets a "Review" button that shows the diff and can be annotated.

To see something like this in action, Python has recently started doing this,
using the "rietveld" code review tool. Since they also use Roundup (and will
start using Mercurial in the hopefully near future), they have a quite similar
setup to ours, so it's worth having a look. If you go to
http://bugs.python.org/issue2001, you see that there is a patch attached which
has a "review" link. Follow it, then click on the "View" link for Lib/pydoc.py
and you'll see a side-by-side diff with the reviewer's comments in four places.

Thoughts, comments?