Codereview with Review Board, git/SVN

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Codereview with Review Board, git/SVN

Nikolai Prokoschenko
Administrator
Hello fellow developers,

(warning, long-ish text ahead)

in the previous weekly meeting we discussed code reviewing using Review
Board (http://www.review-board.org). I'd like to follow up on that with
some details and also initiate a discussion about adopting or not
adopting this method.

*Current state*

I've setup an instance of Review Board 1.0b2 at
http://codereview.musicbrainz.org. Currently, two repositories exist,
our main SVN repository and also a local mirror of Oliver's github
repository (more on that down below). We've put up some fictive and one
real patch online and reviewed it there -- it seems to work fine and
apart from several workflow-induced problems and a couple of minor
annoyances it's a good fit for us.

Anybody caring to test, please don't hesitate to register on the site
and try stuff out. Ping me on IRC if you need help. Other guys (Oliver,
Rob, etc) could probably help too if I'm not around.

*Problems*

Review Board is a product created internally and for internal use at
VMware (thanks guys!). Since VMware uses SVN (or used at that time), RB
is best suited for SVN at the moment, even though other RCS backends
have been developed. It shows when you compare the way it works with
repositories: for SVN a repository can be a remote one, for Git only
local repositories are understood for now. This is important, since RB currently
requires RCS-diffs, i.e. output produced by "svn diff", "git diff" and
other diff outputs with integrated revision numbers. No plain diffs
allowed! Those RCS-diffs get matched against real repositories, so every
diff is shown in context of the repository, i.e. with possibility to see
more diff context, comment on areas outside of diff ("This line will
have to be changed too") etc. I consider this requirement valid, even
though I would really like having remote git support (shouldn't be
difficult to patch either myself, I guess).

Nevertheless, this workflow gives us a bit of a headache because of
different developer preferences. Let's consider workflows which are
currently employed at MusicBrainz:

1. Direct SVN usage. This one is a no-brainer: upload a diff against
trunk, done. Somewhat cumbersome to develop without clean feature
branches, but it's what you pay for.

2. Working with git using git-svn. This is also supported directly as
far as I could see, the "post-review" tool used to upload diffs without
web UI can detect a git-svn repository and calculates SVN revision
numbers accordingly. From there it's like item 1.

3. Working with pure git repositories cloned from a (git-svn) mirror. This
one is somewhat difficult, but doable. It requires a central git
repository (which we currently don't have officially) whose mirror would be
locally on the server. A developer would clone his copy from that
central repository. Then, each diff *must* be done against clean
master branch. In this case, "post-review" doesn't work properly, since
it doesn't detect repository's URL correclty (have to see yet whether we
can persuade it a.k.a. override the URL). Much discipline is needed to never
commit to the master branch and also keep it synced.

4. Working with Bazaar with bzr-svn. I haven't tested it, but I assumme
it is possible to make bzr-svn produce SVN-compatible diffs like git-svn
does. If that's the case, bzr-svn is supported just as is git-svn.

5. Working with Bazaar using pure bzr. We have a bzr mirror of our SVN
repository on Launchpad at https://launchpad.net/mbserver/trunk (again,
rather unofficial), so everyone can branch off of it. I can setup that
one in RB and assuming my bzr knowledge is correct and RB's bzr support
worth its name, this way of working will essentially boil down to the
same as with git under item 3. Alas, I haven't tested that yet,
volunteers are welcome to try it.

These five scenarios should cover up all use cases for now. I really
hope that items 4 and 5 really work this way and I hope those suit
Lukáš' way of working (otherwise please correct me!). If that's the
case, we *can* build up a proper review-board installation and use that
in our daily work.

Apart from that, small nit-picks about RB include missing "renames" support, so
you mostly have huge delete/insert areas if you move files around. Git's
"-M" diffs are not supported, I've opened a ticket about that
(http://code.google.com/p/reviewboard/issues/detail?id=1089), no answer
up to now.

*Alternatives*

We've got some other possibilities for code reviews I've found, but it
seems none of those fits completely:

 * Github has some basic code reviewing capability, which will probably
   get better over time. There is no central place for reviews, so every
   developer would request reviews in his own repository. Good for
   informal reviews, not so good for us, I guess. Completely git-based.

 * Gerrit2 (http://gerrit.googlecode.com) used to be tailored to
   Perforce, then to something else and then to git, runs on JEE,
   haven't looked at it in detail, mostly because of installation
   overhead. Since it's used at Google, e.g. for Android project, it
   could be good, but for git-only workflows. Also probably too
   bureaucratic, judging from Android's workflow
   (http://source.android.com/submit-patches/workflow). One advantage
   could be that Gerrit explicitely merges patches itself, so this would
   enforce "review before commit".
 
 * Launchpad has some *very* limited review capability. It's bzr-based
   and doesn't even let you comment on lines. Otherwise it features
   review requests, called "Merge proposals", which github lacks.

 * Some other tools that look like they are from last century (and they
   probably are), I'm not touching those, especially since they mostly
   don't know either of git nor of bzr.

 * Some commercial tools, especially Crucible from Atlassian, who have
   free licenses for Open Source projects
   (http://www.atlassian.com/software/views/opensource-license-request.jsp). I
   tend to dislike Atlassian, but they might be good. I could try to set
   up a trial version. They support SVN and git from the looks of it,
   but no Bazaar.

So this is it. I'm looking forward to your nitpicks, constructive
discussion and general bashing :) Please raise your voice and tell how
and whether you like it and how you would prefer to work with it. Thanks.

Nikolai.

PS. All of that circus because our BDFL can't choose a decent DVCS for the bright
future.... *scnr*


_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel
Reply | Threaded
Open this post in threaded view
|

Re: Codereview with Review Board, git/SVN

Robert Kaye
Administrator

On Apr 28, 2009, at 1:08 PM, Nikolai Prokoschenko wrote:

> Nevertheless, this workflow gives us a bit of a headache because of
> different developer preferences. Let's consider workflows which are
> currently employed at MusicBrainz:


I'd like to hear from the each of the people who'd be subjected to  
these rules. Brian, Oliver, Luks, please let us know what you think.  
My personal workflow wouldn't be impacted much by this, so I'm ok, but  
I'm not one of the heavy hitting code pounders...

--

--ruaok      A village in Texas has its idiot back!

Robert Kaye     --     [hidden email]     --    http://mayhem-chaos.net


_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel
Reply | Threaded
Open this post in threaded view
|

Re: Codereview with Review Board, git/SVN

Brian Schweitzer
Number 3 I could live with, so long as we do try (better than perhaps we've done in the past) to truly keep a clean and sync'd git master.

I've tried putting the Open Edits patch on code-review; http://codereview.musicbrainz.org/r/8/

Brian


On Tue, Apr 28, 2009 at 6:10 PM, Robert Kaye <[hidden email]> wrote:

On Apr 28, 2009, at 1:08 PM, Nikolai Prokoschenko wrote:

> Nevertheless, this workflow gives us a bit of a headache because of
> different developer preferences. Let's consider workflows which are
> currently employed at MusicBrainz:


I'd like to hear from the each of the people who'd be subjected to
these rules. Brian, Oliver, Luks, please let us know what you think.
My personal workflow wouldn't be impacted much by this, so I'm ok, but
I'm not one of the heavy hitting code pounders...

--

--ruaok      A village in Texas has its idiot back!

Robert Kaye     --     [hidden email]     --    http://mayhem-chaos.net


_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel


_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel
Reply | Threaded
Open this post in threaded view
|

Re: Codereview with Review Board, git/SVN

Lukáš Lalinský
In reply to this post by Nikolai Prokoschenko
On Tue, Apr 28, 2009 at 10:08 PM, Nikolai Prokoschenko
<[hidden email]> wrote:
> 4. Working with Bazaar with bzr-svn. I haven't tested it, but I assumme
> it is possible to make bzr-svn produce SVN-compatible diffs like git-svn
> does. If that's the case, bzr-svn is supported just as is git-svn.

No, bzr-svn can't produce svn-like diffs (it's meant to integrate svn
into bzr, not the other way around). I've written a simple plugin that
does that http://bzr.oxygene.sk/bzr-plugins/svndiff/__init__.py

> 5. Working with Bazaar using pure bzr. We have a bzr mirror of our SVN
> repository on Launchpad at https://launchpad.net/mbserver/trunk (again,
> rather unofficial), so everyone can branch off of it. I can setup that
> one in RB and assuming my bzr knowledge is correct and RB's bzr support
> worth its name, this way of working will essentially boil down to the
> same as with git under item 3. Alas, I haven't tested that yet,
> volunteers are welcome to try it.

https://launchpad.net/mbserver/trunk is a different branch than what
bzr-svn would produce. This is fine for non-committers, who only want
to use it to produce patches against svn, but not for people who want
to use bzr-svn to work with MB's svn server.

> So this is it. I'm looking forward to your nitpicks, constructive
> discussion and general bashing :) Please raise your voice and tell how
> and whether you like it and how you would prefer to work with it. Thanks.

I have mixed feelings about review board. I definitely like the way it
handles patches and comments, I like having a manageable patch queue,
but:

 * I'd like to receive mail on every new patch or comment.
 * I think it's natural that a patch might lead to larger discussion,
and we would probably have to move such discussions from RB to a
mailing list. But then both RB and the ML would miss some context, and
we would have to switch between them.

--
Lukas Lalinsky
[hidden email]

_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel
Reply | Threaded
Open this post in threaded view
|

Re: Codereview with Review Board, git/SVN

Nikolai Prokoschenko
Administrator
Lukáš Lalinský wrote:

> https://launchpad.net/mbserver/trunk is a different branch than what
> bzr-svn would produce. This is fine for non-committers, who only want
> to use it to produce patches against svn, but not for people who want
> to use bzr-svn to work with MB's svn server.

Yes, just as the git mirror it's meant to help you manage your work in bzr
and be able to upload a patch against current (bzr mirror) HEAD. There is no
connection to the SVN server, so committing will be an entirely different
task, probably done by other people.

>  * I'd like to receive mail on every new patch or comment.

As far as I know (and see in source), every time a review is submitted you
get an email about it, which requires a valid e-mail address configured of
course. Also, some RSS and Atom feeds are included in the source, I don't
see yet how to activate them. General direction is apparently: you'll get
informed on every bit you've worked on.

>  * I think it's natural that a patch might lead to larger discussion,
> and we would probably have to move such discussions from RB to a
> mailing list. But then both RB and the ML would miss some context, and
> we would have to switch between them.

I'd prefer to keep all discussions in RB because smaller chunks can be
discussed in independent threads and thus will hopefully each remain small.
Any general discussion should go into the ML of course.

Nikolai.




_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel
Reply | Threaded
Open this post in threaded view
|

Re: Codereview with Review Board, git/SVN

Brian Schweitzer
I've been able to add 2 patches to code-review.  However, I have no idea what triggers this, or what it means, but when I try to submit the newUI patch as well, I get a "diffviewer_filediff.dest_detail may not be NULL" error.

Brian

_______________________________________________
MusicBrainz-devel mailing list
[hidden email]
http://lists.musicbrainz.org/mailman/listinfo/musicbrainz-devel