ReviewBoard

[Mark Côté]

The BMO team has set up a ReviewBoard instance for experimentation &
evaluation at https://reviewboard.allizom.org. Please check it out,
although for the time being don't use it for non-public or really
critical reviews, since it's still in an evaluation phase.

More info can be found in my blog post:
http://mrcote.info/blog/2013/11/26/reviewboard/

Mark

[James Graham]

On 26/11/13 17:42, Mark Côté wrote:
> More info can be found in my blog post:
> http://mrcote.info/blog/2013/11/26/reviewboard/

Based on the blog post, it sounds like you are still interested in
knowing if ReviewBoard is the right choice. I don't know exactly what
the state of modern ReviewBoard is, but I do have some experience using
a code review system that I consider to be high quality and well suited
to development at Mozilla scale, so, in the spirit of gps's post about
Phabricator I can outline the features that I particularly grew to
appreciate that seem to be missing in Mozilla's current setup and may or
may not be present in ReviewBoard.

The code review system I am using as a benchmark is OperaCritic (aka
critic) [1]. It was developed by an Opera employee a couple of years ago
in response to significant issues experienced using other review systems
at the time. It is probably not directly suitable for use at Mozilla
since it assumes git is being used for code review and heavily depends
on git features. Nevertheless I think it indicates a standard that we
should aim for.

From my point of view the most appealing features of critic are:

* Trivial to start a review without leaving the command line. In critic
a review is just a remote branch with a special naming convention.
Therefore starting a review (at least when there is only one commit on
the review branch) is possible without interacting with the web UI at
all and without learning any kind of special tools. It simply looks like:

git push --set-upstream critic HEAD:r/jgraham/my_review_name

* Trivial to update a review from the command line. Updating a review is
simply a case of pushing more changes to the review branch. Again there
is no new tool to learn other than the VCS. This also means that it is
very easy for multiple people to contribute to the same review branch,
making collaborative changes easier.

* Automatic identification of reviewers for a change. Instead of making
it the requestee's responsibility to know who owns the code that they
are changing, critic requires code owners to register themselves as
reviewers for specific parts of the tree. When a review comes in
everyone who owns one or more files touched by the review is assigned as
a reviewer for the subset of the changes in files they own. There are
then manual overrides for these defaults which can be used to e.g.
assign one specific person as the reviewer for a particular review.

* Great diff support. Because critic has access to the whole repository
including all commits on the review branch, it isn't limited in the way
that a static diff based tool often is. In particular one can view
interdiffs (these are just the individual commits), or the sum of any N
contiguous commits in the review branch, right up to the whole review
branch squashed into a single diff. Diffs can also be expanded in place,
and the whole tree can be browsed with the changes applied.

* Side by side diffs. Sounds obvious, but, incredibly, GitHub doesn't
have it… Also, highlighting of common errors like trailing whitespace in
the diff.

* Differentiation between "issues" and "notes". With a review there are
often "must fix" items and general comments that might not need a change
for the review to be accepted. Critic calls the former "issues" and the
latter notes.

* Automatic tracking and updating of review progress. Every time that a
change to a specific file in a specific commit is reviewed — whether or
not there were issues found — it is marked as "reviewed". This is used
to give an indication of the overall progress of the review, and to
allow collaborative review. In addition the number of outstanding issues
is tracked so it is clear what remains to be addressed. Together these
mean that there is no need for a final "r+"-type comment; the review is
done when all commits are reviewed and there are no remaining issues.

* Automatic marking of issues fixed when the line(s) they refer to are
changed. Critic is optimistic about issues getting fixed; it assumes
that any new commit that touches a line with an issue filed against it
probably fixes that issue and closes the issue. If this is not in fact
the case it is the responsibility of the reviewer to reopen the issue
against the new code. This turns out to be a great default because it
significantly reduces the amount of tedious make-work that the review
system imposes.

* Support for merges and rebases. In particular critic allows review of
the way that merge conflicts are resolved. Rebases are handled in terms
of an "equivalent merge commit" and so can be reviewed in just the same
way. Modifying branch history (e.g. squashing several commits together)
is also supported. Issues are not lost in the face of this kind of
history modification.

* Batched notifications. This is, again, as a counterpoint to GitHub
which produces one email per change; a setup which is unbelievably
spammy. Critic allows multiple changes to be made to a review, and then
the whole lot sent at once, so that the amount of mail is kept to a minimum.

* Extensible. Critic allows writing extensions in js, which can be used
to add (preject-specific) support for bug trackers, editors, build
systems, etc.

If you want to see a critic instance, there is one that we use for
reviewing web platform tests tests at [2]. Note that login is via github
credentials (using OAuth) and that because of the way this integrates
with GH, some of the details around e.g. creating a review are slightly
different (you push to GH, not to the critic instance directly, for
example). Also note that some people are significantly put off by the
clearly engineer-designed UI. This is not a fair proxy for the overall
quality of the tool. My version is also slightly old and doesn't have
extension support enabled.

Again, I understand that using critic directly is implausible, at least
for mozilla-central. However I think it is a great benchmark of the
level of functionality that we should be aiming for. Having an awesome
code review tool makes a *huge* difference in the overall productivity
of a software development project, so we really want to aim as high as
possible here.

[1] https://github.com/jensl/critic
[2] https://critic.hoppipolla.co.uk

[Matej Cepl]

Let me add a comment about the experience we had at my work.
Yes, we were trying the version current probably a year ago, but
we then found using ReviewBoard for git repositories (or any
DVCS to be honest) as extremely difficult. RB was obviously
designed for centralised VCS (originally ClearCase or Perforce?)
and it seemed to have a problem with independent commits from
various repositories. In the end, we use Gerrit and with some
hesitation it works for us fairly well. There seems to be
a reason why so many large open source projects (outside of
GitHub) use Gerrit (aside from Android, also LibreOffice,
Eclipse, OpenStack, and many smaller ones). Looking at
http://www.reviewboard.org/users/ it has probably better
commercial support (is there any commercial support for Gerrit
at all?), but the list doesn't seem to be that persuasive.

Also, have you seen http://gerrithub.org/ ?

Best,

Matěj

--
http://www.ceplovi.cz/matej/, Jabber: mcepl<at>ceplovi.cz
GPG Finger: 89EF 4BC6 288A BF43 1BAB 25C3 E09F EF25 D964 84AC

We are told that [St. Anthony] once fell into dejection, finding
uninterrupted contemplation above his strength; but was taught to
apply himself at intervals to manual labour by a vision of an
angel who appeared platting mats of palm-tree leaves, then rising
to pray, and after some time sitting down again to work; and who
at length said to him, "Do thus, and thou shalt be saved."
-- Life of St. Anthony

[Steven MacLeod]

I'm a comitter on the Review Board project as well as a Mozillian. I'll
reply inline to each of your points and compare Review Board to critic.

> It is probably not directly suitable for use at Mozilla
> since it assumes git is being used for code review and heavily depends
> on git features.

Review Board supports a number of repository types: git, hg, cvs, svn, bzr,
clearcase, perforce, and plastic. additional repository types can also be
added through extensions.

> From my point of view the most appealing features of critic are:
>
> * Trivial to start a review without leaving the command line. In critic
> a review is just a remote branch with a special naming convention.
> Therefore starting a review (at least when there is only one commit on
> the review branch) is possible without interacting with the web UI at
> all and without learning any kind of special tools. It simply looks like:
>
> git push --set-upstream critic HEAD:r/jgraham/my_review_name

Review Board is hands-off of your repository. It will only read, and the
workflow does not involve pushing commits, or committing to a central
repository.

There are two options for creating "review requests" in Review Board. One
is by using the actual web frontend to upload a diff, or select a range of
commits which have already been pushed. The preferred method is using the
command line client "rbt".

In the git case, to create a review request of your feature branch off of
master you would simply run:

$ rbt post

And to update an existing review request with a new patch you would run:

$ rbt post -r <review-request-id>

or more recently support for heuristically guessing the previous request:

$ rbt post -u

The "rbt" tool is part of the "RBTools" [1] package which is written in
python. RBTools also includes a python binding for the Review Board REST
API, and new commands can be plugged in by third party packages.

So, while using Review Board does require learning a new tool, that same
tool is used for all repository types, and is extensible (mach commands
could even be written to take care of posting review requests for bugs
etc.)

>
> * Trivial to update a review from the command line. Updating a review is
> simply a case of pushing more changes to the review branch. Again there
> is no new tool to learn other than the VCS. This also means that it is
> very easy for multiple people to contribute to the same review branch,
> making collaborative changes easier.

Like previously mentioned, rbt takes care of updating.

Review Board currently supports only a single "owner" of a review request,
so collaboratively updating that request isn't supported.

> * Automatic identification of reviewers for a change. Instead of making
> it the requestee's responsibility to know who owns the code that they
> are changing, critic requires code owners to register themselves as
> reviewers for specific parts of the tree. When a review comes in
> everyone who owns one or more files touched by the review is assigned as
> a reviewer for the subset of the changes in files they own. There are
> then manual overrides for these defaults which can be used to e.g.
> assign one specific person as the reviewer for a particular review.

Review Board can automatically assign "Review Groups" or specific users
to a review request based on the files touched, or repository being
referenced. These groups must have users manually added, and the rules
for automatically assigning groups must be created manually.

A suggestion feature for reviewers (similar to bugzilla's) is currently
in the works, but is not complete. I feel at Mozilla this information
might be taken care of in Bugzilla anyways?

> * Great diff support. Because critic has access to the whole repository
> including all commits on the review branch, it isn't limited in the way
> that a static diff based tool often is. In particular one can view
> interdiffs (these are just the individual commits), or the sum of any N
> contiguous commits in the review branch, right up to the whole review
> branch squashed into a single diff. Diffs can also be expanded in place,
> and the whole tree can be browsed with the changes applied.

Review Board generates and displays very nice diffs [2]. Review Board
differs from critic here, as it deals with "patches" instead of commits,
so the interdiffs it generates are between versions of patches uploaded.

For example, I create a review request with a patch. It is reviewed, and
I update the patch with changes. Review Board will then allow you to see
the differences between the old patch, and the new patch (You can actually
ask for the interdiff between any two versions of the patch).

Another feature is things other than diffs are also reviewable. Review Board
supports displaying and commenting on regions of images and other file types.

> * Side by side diffs. Sounds obvious, but, incredibly, GitHub doesn't
> have it… Also, highlighting of common errors like trailing whitespace in
> the diff.

Review Board uses side by side diffs. Trailing whitespace highlighting can
be turned on or off.

Also, Review Board will highlight not only changes to lines, but the smaller
portion which was actually changed on the line (you can see this as a darker
yellow highlighting on some of the modified lines in [2])

Another cool feature of Review Board, is detection of moved lines. If a
section of code was deleted in one part of the diff, and added somewhere else,
Review Board will annotate the deletion and addition as a move, saying where
it was put or came from ([2] also includes an example of this).

> * Differentiation between "issues" and "notes". With a review there are
> often "must fix" items and general comments that might not need a change
> for the review to be accepted. Critic calls the former "issues" and the
> latter notes.

Review Board uses "issues" and "comments". When you make a comment on part
of the code, you may opt to open an issue. These issues are tracked, and
the review request owner has the option to "fix" or "drop" them.

> * Automatic tracking and updating of review progress. Every time that a
> change to a specific file in a specific commit is reviewed — whether or
> not there were issues found — it is marked as "reviewed". This is used
> to give an indication of the overall progress of the review, and to
> allow collaborative review. In addition the number of outstanding issues
> is tracked so it is clear what remains to be addressed. Together these
> mean that there is no need for a final "r+"-type comment; the review is
> done when all commits are reviewed and there are no remaining issues.

Review Board doesn't track review progress automatically. The workflow that
Review Board development uses is comments are made and issues are opened
until a patch is posted that the reviewer is happy with. Then they click
the "Ship-it" checkbox when publishing their review, which marks that
review as signing off on the code.

> * Automatic marking of issues fixed when the line(s) they refer to are
> changed. Critic is optimistic about issues getting fixed; it assumes
> that any new commit that touches a line with an issue filed against it
> probably fixes that issue and closes the issue. If this is not in fact
> the case it is the responsibility of the reviewer to reopen the issue
> against the new code. This turns out to be a great default because it
> significantly reduces the amount of tedious make-work that the review
> system imposes.

Review Board does not do this automatically. It is the responsibility of
the review request owner to mark issues as "fixed" as they fix them. I
actually use this as a sort of checklist, and mark each as fixed as I
fix them, and then post the new patch once I have fixed all of the issues.

> * Support for merges and rebases. In particular critic allows review of
> the way that merge conflicts are resolved. Rebases are handled in terms
> of an "equivalent merge commit" and so can be reviewed in just the same
> way. Modifying branch history (e.g. squashing several commits together)
> is also supported. Issues are not lost in the face of this kind of
> history modification.

Since your aren't pushing commits, you can merge / rebase all you want
locally, and just post the diffs to Review Board. If you would like to
review a merge, it can be posted as a diff before or after pushing.

> * Batched notifications. This is, again, as a counterpoint to GitHub
> which produces one email per change; a setup which is unbelievably
> spammy. Critic allows multiple changes to be made to a review, and then

> the whole lot sent at once, so that the amount of mail is kept to a minimum.

Notifications are batch in Review Board. This is dealt with using "drafts"
for everything. When you're making a review, you can modify it throughout
updating your draft, and then you eventually "publish" it, which sends
a notification and posts it publicly. Replies to reviews also use drafts.

> * Extensible. Critic allows writing extensions in js, which can be used
> to add (preject-specific) support for bug trackers, editors, build
> systems, etc.

Review Board supports both python extensions to the backend which can
modify the HTML, CSS, JS which Review Board outputs (Among many other
things), and front-end js extension support.

There is also support for extending review board to review other file types.
For example, there is a (commercial) plugin [3] for Review Board which allows
reviewing pdf files directly in the browser.

> If you want to see a critic instance, there is one that we use for
> reviewing web platform tests tests at [2]. Note that login is via github
> credentials (using OAuth) and that because of the way this integrates
> with GH, some of the details around e.g. creating a review are slightly
> different (you push to GH, not to the critic instance directly, for
> example). Also note that some people are significantly put off by the
> clearly engineer-designed UI. This is not a fair proxy for the overall
> quality of the tool. My version is also slightly old and doesn't have
> extension support enabled.

Review Board design strives to be as simple and usable as possible, you
can look at [2] for examples of the interface.


[1] https://github.com/reviewboard/rbtools/
[2] https://reviews.reviewboard.org/r/4919/diff/#
[3] http://www.reviewboard.org/powerpack/

[Mark Côté]

The number one problem with Gerrit, which is, at this time, a
show-stopper, is that it only supports git. Getting really tight
integration with source seems to be possible only if you use one, and
only one, VCS. At many organizations that seems like a no-brainer, but,
well, Mozilla isn't like many organizations. :) If we ever decide to be
100% git, we can revisit, but with all our various priorities, I have a
hard time imagining that happening for some time, if ever.

That doesn't mean we can't use it as inspiration, but I want to
reiterate that many things are made more complicated when you have to
support multiple VCSs.

FWIW, I used Review Board and git while I worked on some patches for
Review Board itself. I wasn't doing anything particularly complicated,
but I was impressed with how easily it fit into my workflow.

Mark

[Matěj Cepl]

On 26/11/13 22:54, Mark Côté wrote:
> FWIW, I used Review Board and git while I worked on some patches for
> Review Board itself. I wasn't doing anything particularly complicated,
> but I was impressed with how easily it fit into my workflow.

Perhaps RBTools help (I don't think we had them) or perhaps our workflow
was too much in collision with RB.

Of course, I know about Mercurial being still main VCS in MoFo. Just
that I have been around Firefox OS and Jetpack a bit, and it seems to me
that push towards git is really strong. With Mercurial it seems to me
(forgive me, I don't want to troll here, honestly) that MoFo jumped on
the second best system (at least as it looks now) and it regrets it a
bit now. Isn't switch to RB similar? Perhaps tool for code review
doesn't have that many long-term consequences as the choice of the VCS
tool itself.

But I guess I said what I wanted, and dragging this thread further
doesn’t make much sense.

Best,

Matěj

--
http://www.ceplovi.cz/matej/, Jabber: mc...@ceplovi.cz

GPG Finger: 89EF 4BC6 288A BF43 1BAB 25C3 E09F EF25 D964 84AC

This is a test of the emergency signature system. Were this an
actual signature, you would see amusing mottos, disclaimers,
a zillion net addresses, or edifying philosophical statements.
this is only test.

[tr...@adblockplus.org]

Hi Mark,

it's interesting that Mozilla is evaluating Review Board now - we did the same only a few weeks ago. Currently the Adblock Plus project uses Rietveld which serves our needs but clearly has its issues. We would prefer something that is simpler to use for our contributors. However, when we looked at Review Board we decided that the issues with it are significantly worse - and we only evaluated the user interface briefly.

For reasons that are completely beyond me Review Board hides the code behind a tab selector. When you open a review what you see is an assorted selection of comments without any context. My initial reaction was clicking the Review link - and all I got was a text field. In order to see the code you have to click the "View Diff" link which took me some time to figure out.

In general, Review Board UI is designed in such a way that it puts lots of focus on metadata - and completely fails to point out the important things. When viewing the comments or reviewing code there is always a huge block with status information at the top. The comments emphasize the name of the commenter instead of the actual comment text. Status changes take up more space than comments. And so on.

There are also lots of minor usability issues of course, like a tooltip on a star icon saying "Click to star" (thank you for explaining, now if somebody told me why I would want to star a review...) or adding a comment to a line requiring a click on the line number and not working on the line itself.

Feature-wise there is a feature in rietveld that is important for me and that I couldn't find in Review Board: an interdiff will show all the comments that were made for the old patch so that you can immediately see whether these comments have been addressed. But features are really not the issue here, the user interface is.

regards
Wladimir

[James Graham]

On 26/11/13 20:50, Steven MacLeod wrote:

> In the git case, to create a review request of your feature branch off of
> master you would simply run:
>
> $ rbt post
>
> And to update an existing review request with a new patch you would run:
>
> $ rbt post -r <review-request-id>
>
> or more recently support for heuristically guessing the previous request:
>
> $ rbt post -u
>
> The "rbt" tool is part of the "RBTools" [1] package which is written in
> python. RBTools also includes a python binding for the Review Board REST
> API, and new commands can be plugged in by third party packages.

So "heuristically guess" makes it sound like "will go wrong, sometimes".

In general I think that working with commits directly makes for a
superior experience compared to working with patch files, even if the
tool tries to reconstruct the post-patch tree. Not only does it mean
that you can work directly with all the features of your VCS, it allows
for workflows that seem difficult when working with patches alone. For
example consider a change that splits into two parts, A and B, where A
might be uncontroversial core part and B a more controversial additional
part that depends on A. It seems desirable to post the two parts as
seperate reviews. With a commit-based system this is natural. With a
patch based system the fact that a patch for B won't apply to the
mainline seems likely to break the world, and you would be forced to
wait for A to be accepted before even letting people look at B.

> Review Board currently supports only a single "owner" of a review request,
> so collaboratively updating that request isn't supported.

This seems like a huge loss. I know it is more or less the status-quo at
Mozilla, but the ability to have multiple committers to a review is a
huge boon. For example it allows the original reviewer to make quick
fixups to minor style issues in a review, and makes it easy for a third
party to take over when the original author is unable to respond to
review issues for whatever reason. Of course these things are possible
with patches, but the process is considerably more cumbersome, and the
original author may lose their credit from the commit history. Being
able to have multiple collaborators on a review branch also enables
whole new kinds of workflow. For example long-running projects can use a
branch in the review system as their shared branch, and do internal
review of each other's code as they go along.

> A suggestion feature for reviewers (similar to bugzilla's) is currently
> in the works, but is not complete. I feel at Mozilla this information
> might be taken care of in Bugzilla anyways?

FWIW, I strongly feel that the reviewer suggestion feature isn't a good
enough solution for the problem of assigning a reviewer to a particular
change. The person submitting a patch shouldn't be expected to make
*any* choices about who should review it because, especially in the case
of new contributers, they are the people least qualified to make that
decision.

> Also, Review Board will highlight not only changes to lines, but the smaller
> portion which was actually changed on the line (you can see this as a darker
> yellow highlighting on some of the modified lines in [2])

Yes, critic has this too, and also syntax highlighting of files being
reviewed, which it looks like rb also has, but I should have mentioned
on my list of "essential features".

>> If you want to see a critic instance, there is one that we use for
>> reviewing web platform tests tests at [2]. Note that login is via github
>> credentials (using OAuth) and that because of the way this integrates
>> with GH, some of the details around e.g. creating a review are slightly
>> different (you push to GH, not to the critic instance directly, for
>> example). Also note that some people are significantly put off by the
>> clearly engineer-designed UI. This is not a fair proxy for the overall
>> quality of the tool. My version is also slightly old and doesn't have
>> extension support enabled.
>
> Review Board design strives to be as simple and usable as possible, you
> can look at [2] for examples of the interface.

I think opinions may differ on the extent to which it succeeded in that
goal. At least I recall finding it reasonably unintuitive a couple of
years ago. The visual design is certainly better though.

Based on your response it feels like rb would be a step up from the
current Mozilla tools, but that there would still be notable
deficiencies compared to other available systems. Of course the multiple
VCS issue is a big problem; there generally don't seems to be that many
code review tools that work directly with mercurial repositories.
Possibly this is because local branches and history modification are so
important for the repo-based code review workflow and, at least until
recently, there has been poor support for this in hg. However it seems
that this is changing and there are at least some attempts to fix this
e.g. kiln seems to try to abstract over the differences. I haven't used
it so I don't know how well it works, and the closed-source nature means
that I'm not suggesting it as a serious alternative, but it does at
least suggest that I might not be asking for the impossible.

[Gijs Kruitbosch]

I tried this just now and got a 500 error:

https://reviewboard.allizom.org/r/11/


Something broke! (Error 500)

It appears something broke when you tried to go to here. This is either
a bug in Review Board or a server configuration error. Please report
this to your administrator.



Not a great start...

~ Gijs

[Mark Côté]

Hm yeah I've seen a few of these. I will take a look today.

Mark

[Mark Côté]

Aha, Review Board usernames aren't supposed to have plus characters in
them. The error was a bit obscure because users are written directly to
the database, bypassing the standard registration form, which presumably
would have displayed a more helpful error.

I'm submitting patches to upstream to allow plusses; should be deployed
to our installation before too long.

Mark

[Steven MacLeod]

> From: "James Graham" <ja...@hoppipolla.co.uk>
> So "heuristically guess" makes it sound like "will go wrong, sometimes".

It will prompt the user to confirm its guess. It works by trying to match
your commit messages to the summary and description Review Board has for
your review requests.

> In general I think that working with commits directly makes for a
> superior experience compared to working with patch files, even if the
> tool tries to reconstruct the post-patch tree. Not only does it mean
> that you can work directly with all the features of your VCS, it allows
> for workflows that seem difficult when working with patches alone. For
> example consider a change that splits into two parts, A and B, where A
> might be uncontroversial core part and B a more controversial additional
> part that depends on A. It seems desirable to post the two parts as
> seperate reviews. With a commit-based system this is natural. With a
> patch based system the fact that a patch for B won't apply to the
> mainline seems likely to break the world, and you would be forced to
> wait for A to be accepted before even letting people look at B.

So, you have:

master --> A --> B

In Review Board world you would deal with this like so:

$ git checkout A
$ rbt post # Creates a request between master and A
$ git checkout B
$ rbt post --parent=A # Creates a request between A and B

You would end up with two Review Requests where the second is only the
diff between A and B. You then mark that review request as depending on
the first. Two separate review requests, patch B applies cleanly and is
shown.

Yes, this still isn't as nice as if the commits were maintained and
reviewable as a series in a single request, instead of separate manually
created requests. Better DCVS support is something being worked on and
planned for future Review Board versions.

> > Review Board currently supports only a single "owner" of a review request,
> > so collaboratively updating that request isn't supported.
>

> This seems like a huge loss. I know it is more or less the status-quo at
> Mozilla, but the ability to have multiple committers to a review is a
> huge boon. For example it allows the original reviewer to make quick
> fixups to minor style issues in a review, and makes it easy for a third
> party to take over when the original author is unable to respond to
> review issues for whatever reason. Of course these things are possible
> with patches, but the process is considerably more cumbersome, and the
> original author may lose their credit from the commit history. Being
> able to have multiple collaborators on a review branch also enables
> whole new kinds of workflow. For example long-running projects can use a
> branch in the review system as their shared branch, and do internal
> review of each other's code as they go along.

Yes, I agree Review Board could be better here. There has been work to allow
the owner of the review request to be switched, but it's not complete, and
that only really deals with the case where another person takes over the
request completely.

> > A suggestion feature for reviewers (similar to bugzilla's) is currently
> > in the works, but is not complete. I feel at Mozilla this information
> > might be taken care of in Bugzilla anyways?
>

> FWIW, I strongly feel that the reviewer suggestion feature isn't a good
> enough solution for the problem of assigning a reviewer to a particular
> change. The person submitting a patch shouldn't be expected to make
> *any* choices about who should review it because, especially in the case
> of new contributers, they are the people least qualified to make that
> decision.

Is this not how things are done currently, I don't understand how you could
have something *better* than just a suggestion? Should we expect the review
tool to not let code by unless the person it *thinks* should review it does?

In the case of new contributors, they'll probably be dealing with mentored
bugs, or at least have contact with someone who can identify a good reviewer.

I feel like this is a hard problem, that goes beyond just the review tool.

[chi...@gmail.com]

Hey everyone,

I'm one of the founders of Review Board. I wanted to thank everyone for your feedback. It's great to know how people use other tools and where they find Review Board comes short, or the differences in how you interact with those tools vs. ours. I also want to thank Mark for getting this server going and for all the contributions :)

I'm not going to reply to every comment (at least not yet -- out visiting family for the holidays), but feel free to ask me any questions. Or ask Steven, who's been helping us out with the project for a while now and knows some parts of it better than I.

I wanted to give some of my thoughts an rationale on Review Board, its status, plans going forward, and how it differs from other code review tools.

Review Board started as a hobby project to scratch an itch. Nights and weekends. You know how it goes. We've been building it for many years, iterating on it. It's come a *long* way over the years, and I personally have years worth of solid improvements on my To Do list.

We've never tried to mimic other tools (there were only a couple others in the early days anyway), or match every bullet point, every workflow, or solve every need. You can't please everyone -- there are a lot of different methodologies to code review -- but the bulk of the feedback is very positive. There's a couple thousand companies using the product now, so I take pride in that. (Too bad we can't list them all..)

A few months ago, Review Board became our full-time job. We've made tons of progress since then. I personally dedicate around 14 hours a day to Review Board, far more than the few hours a day I could dedicate after coming home from $DAYJOB. That means a great many improvements are coming.

Take our upcoming 2.0 release. We've rewritten a lot of our foundation to be more flexible, extensible, and to let us start working on the next evolution of the product. This release will be bringing things such as Markdown comments/reviews, image diffs, an improved diff viewer, better moved line detection, tighter integration with repositories (making it easier to post changes sitting on a branch), and a lot of under-the-hood changes we need to begin DVCS support.

Early on, we decided it was important not to limit ourselves to just code review. Yes, code review is extremely important, but projects often consist of more than just code. If you're reviewing a UI change, you should be able to see screenshots and review them, so we added image review. The docs team may have a KB article or manual for engineers to look at, so we're building PDF document review. Our ability to review things is extensible, so third parties can add review support for other types of files. We'll be adding more in time as well.

Some of our UI reflects that. That's why the diff isn't shown on the review request page, since it's not always used for diffs. There's room for improvement in a lot of the interaction on this page, I'll admit, but some of it depends greatly on how you choose to use the tool.

We also decided early that it was a bad idea to restrict ourselves to one type of repository. If we had done so, it would not have been Git, or Mercurial, as these were nowhere near as wide-spread when we began to write this. It would have been Subversion or Perforce or something. As some have pointed out, this influenced the design early on, but that's far from set in stone.

These days, a good chunk of our user base uses Git, as do we. There's room for improvement here as well. Patchset support, for instance, and working with forks of repositories. We'll be starting on some of this pretty soon. Filling up a notebook with designs.

So, this product is actively, heavily maintained, and not set in stone. We're striving to build a tool that saves people time and headaches. This is our job, our passion. Not everyone's going to agree with how we're doing this, but that's okay. We like the feedback. It helps us continue to evolve the product and make it even more useful.

So keep it coming, and thanks :)

Christian

[mike.d...@gmail.com]

Hey, part-time Review Board contributor reporting in.



On Wednesday, November 27, 2013 2:06:08 AM UTC-5, tr...@adblockplus.org wrote:
> Hi Mark,

>
>
>
> For reasons that are completely beyond me Review Board hides the code behind a tab selector. When you open a review what you see is an assorted selection of comments without any context. My initial reaction was clicking the Review link - and all I got was a text field. In order to see the code you have to click the "View Diff" link which took me some time to figure out.

This is true, and it's not the first time I've heard this remark.

Luckily, it is incredibly easy to link to the View Diff page where a review can be kicked off, and I imagine that might play into how Moz's workflow could operate.

Much the same way people have been posting links to Github pull requests with Github attachments, we could post links to the review request diff.

Even better, this could be made into a command-line tool - either an extension to rbt or for mach.

Imagine doing:

./mach post -b 993523

And that'd post a review request on reviewboard.moz.org, and simultaneously post an attachment in bug 993523 which links to the view diff.

That sounds workable to me.

At least from my perspective, what's lovely about Review Board is how it doesn't really impose much of a workflow. Instead, we impose *our* workflow on Review Board, and make it work for us. The idea I posted above is just off the top of my head - I'm sure we could improve on it.

Just my 2c,

-Mike

[Chris Peterson]

Mark, I log into Bugzilla using Persona and my mozilla.com LDAP
credentials. So I don't actually know what my Bugzilla username or
password are! <:)

How can I log into Review Board staging server on allizom.org?

https://reviewboard.allizom.org/account/login/


chris

[Mark Côté]

Fix has been deployed.

Mark

[Mark Côté]

You can reset your password from within Bugzilla--it does the standard
email-confirmation process. Your username is the email address you use
in Persona (probably cpet...@mozilla.com). After you give a new
password, you can still log into Bugzilla with Persona, and you can use
your new Bugzilla password in Review Board.

I know the Review Board devs have been working on Persona support; I'll
check with them as to the current status.

Mark

[Mark Côté]

Thanks for posting this, Christian. I would love to hear more detailed
responses from you, whenever you have time.

Mark