Code Review & Workflow

I’d like to get some discussion about Gerrit and our current workflow.

I’ll start out with a suggestion. I think we should try out Gitorious and its merge request code review instead of Gerrit.

I think our old system (i.e., no review, frequent merging) wasn’t stable. I personally pushed code when it wasn’t yet considered ready by the author. I think the code review is good and is catching problems and improving code quality, in general.

But I think we’re really slowing down our progress. So I’d like to see if we can find a happy medium. In particular, not as many people check the “verified” box.

-Geoff

15.08.10, 05:33, “Geoffrey Hutchison” geoff.hutchison@gmail.com:

I’d like to get some discussion about Gerrit and our current workflow.

I’ll start out with a suggestion. I think we should try out Gitorious and its merge request code review instead of Gerrit.

Hi Geoff,
I’m using Gitorious for a long time, but never used its merge facility,
because nobody tried to collaborate with me on it :slight_smile:

However, I can give some feedback about it. In particular, Gitorious isn’t
100% stable - sometimes half-day server outages occur, or simply push fails
for 5 minutes, than works

I think our old system (i.e., no review, frequent merging) wasn’t stable.

I agree

I personally pushed code when it wasn’t yet considered ready by the author. I think the code review is good and is catching problems and improving code quality, in general.

But I think we’re really slowing down our progress. So I’d like to see if we can find a happy medium. In particular, not as many people check the “verified” box.

I don’t think speed is so crucial here (earlier I thought though!). If workflow
is correct (independent changes are committed to independent branches,
and than these branches are squashed), we can merge less frequently, but larger
chunks of code. If somebody didn’t take part in review today, tomorrow he will not
regret we made decision without his approval :slight_smile:


Regards,
Konstantin

On Sat, Aug 14, 2010 at 9:33 PM, Geoffrey Hutchison geoff.hutchison@gmail.com wrote:

I’d like to get some discussion about Gerrit and our current workflow.

I’ll start out with a suggestion. I think we should try out Gitorious and its merge request code review instead of Gerrit.

I think our old system (i.e., no review, frequent merging) wasn’t stable. I personally pushed code when it wasn’t yet considered ready by the author. I think the code review is good and is catching problems and improving code quality, in general.

But I think we’re really slowing down our progress. So I’d like to see if we can find a happy medium. In particular, not as many people check the “verified” box.

I agree with what you’re saying about the problems, but I don’t think
moving to Gitorious will make much of a difference. Personally, my
problem is more of discipline – doing code review means setting aside
a block of time to do several compilations, tests, etc, before pushing
the changes. I try to do a review when possible, since those are fast,
but verification takes more effort. Whether we’re doing review through
gitorious or gerrit, the same problems are there.

Some ideas:

  • We could assign different portions of the code to different people,
    that way there is someone who is responsible for each change, and
    they’re less likely to fall through the cracks.
  • We could each set up a time each week to do review. If we can get
    ourselves in the habit of going through all the changes for an hour
    e.g. each Saturday, perhaps discussing them on IRC as needed, the
    process would move faster. I’m not sure if we have enough activity to
    make an “IRC review meeting” worthwhile, but (next point):
  • We can each set aside a block of time to do review regularly, and stick to it.

My two cents,

Dave

On Sat, Aug 14, 2010 at 9:33 PM, Geoffrey Hutchison geoff.hutchison@gmail.com wrote:

But I think we’re really slowing down our progress. So I’d like to see if we can find a happy medium. In particular, not as many people check the “verified” box.

I just went through and pushed several of the changesets that I felt
comfortable with.

Part of the reason I don’t verify or +2 review many of the commits is
because I’m not familiar enough with the code to spot any problems
that may be lurking, hiding deeper than what is shown in the diff.
This is part of the reason I think we should assign parts of the code
to specific people. Let anyone +1 review the code, but +2/verification
should be reserved for someone who is very familiar with the code.
After all, if I sign off on code that I’m not familiar with, there’s
really no point in having the review, since I will likely test the one
case that the author reports, but nothing more in depth.

Dave

On Saturday 14 August 2010 21:33:39 Geoffrey Hutchison wrote:

I’d like to get some discussion about Gerrit and our current workflow.

I’ll start out with a suggestion. I think we should try out Gitorious and
its merge request code review instead of Gerrit.

I don’t think Gitorious gives us more, in fact the feature set is not as well
developed for what we would be moving to it for… At Kitware, part of my
responsibilities have involved examining development workflow.

I honestly believe Gerrit is one of the best of breed tools, and I have
started to communicate with Gerrit upstream a little. They have added a new
feature in 2.1.4 where you can tag all commits as belonging to a particular
topic branch (this help in looking at an entire topic branch) for example.

I think our old system (i.e., no review, frequent merging) wasn’t stable. I
personally pushed code when it wasn’t yet considered ready by the author.
I think the code review is good and is catching problems and improving
code quality, in general.

Agreed.

But I think we’re really slowing down our progress. So I’d like to see if
we can find a happy medium. In particular, not as many people check the
"verified" box.

I think we need to find a happy place with code review, and I don’t think
changing tools will do that. I am trying to build it into my week to sit and
do code review (as it happens I had set aside some time to do so today). I
would rather slow development a little and improve our code quality.

Code review has many benefits, including educating new contributors, preventing
bad code from being merged, catching new issues earlier, rubber ducking
(spotting obvious mistakes the original developer missed)… Gerrit has great
Git integration, and is being actively developed.

Code review is new to us, and for small obvious patches it should be much more
of a two minute “yep, that looks fine”, whereas bigger changes do warrant more
thorough review. It would probably be helpful to go over workflow, and document
some of these thoughts.

I upgraded us to the latest release earlier in the week too.

Marcus

Marcus D. Hanwell, Ph.D.
R&D Engineer, Kitware Inc.
(518) 881-4937

Some ideas:

I think we should also set a general rule: new plugins/extensions need less thorough review. If someone creates a new plugin, it doesn’t mess up the rest of the code. Changes to key files, obviously should get much thorough review. (For example, maybe we set the social custom that changes to libavogadro itself require three separate reviewers?)

For example, the changes for GAMESS-UK and Molden support in the surface/orbital extensions are pretty clean, but need some checking. New engines

  • We could assign different portions of the code to different people,
    that way there is someone who is responsible for each change, and
    they’re less likely to fall through the cracks.

This is generally a good idea, but much harder to implement with only a few volunteer contributors. I think if the “core group” gets to about 6-8, it’s much more useful.

  • We could each set up a time each week to do review. If we can get
    ourselves in the habit of going through all the changes for an hour
  • We can each set aside a block of time to do review regularly, and stick to it.

I think this is what just happened this weekend. I’d be glad to establish that people will commit some time each weekend to go through patches. Then new contributors know when review will happen and when their patches will be incorporated.

But I think this discussion is important. Our previous release schedule was warp speed, but we haven’t had a release in almost a year now. Somewhere in-between would be good for the community and to attract new developers.

-Geoff

16.08.10, 18:19, “Geoffrey Hutchison” geoff.hutchison@gmail.com:

Some ideas:

I think we should also set a general rule: new plugins/extensions need less thorough review. If someone creates a new plugin, it doesn’t mess up the rest of the code.

I disagree. If somebody writes new plugin, he/she is free to distribute it as “3rd party”. If we accept it to our source tree, it should have some degree of quality


Regards,
Konstantin

On Aug 16, 2010, at 10:30 AM, Konstantin Tokarev wrote:

I think we should also set a general rule: new plugins/extensions need less thorough review. If someone creates a new plugin, it doesn’t mess up the rest of the code.

I disagree. If somebody writes new plugin, he/she is free to distribute it as “3rd party”. If we accept it to our source tree, it should have some degree of quality

I’m not saying a new plugin should get no review. What I’m trying to say (if not clearly) is that we clearly need to have some levels of review. New code in our source tree needs to have some degree of quality. New code in libavogadro needs to be high quality, careful not to break ABI, well-commented, etc. These changes need more scrutiny than a new plugin or modifications to an existing plugin.

Is that clearer?

-Geoff

16.08.10, 18:47, “Geoffrey Hutchison” geoff.hutchison@gmail.com:

On Aug 16, 2010, at 10:30 AM, Konstantin Tokarev wrote:

I think we should also set a general rule: new plugins/extensions need less thorough review. If someone creates a new plugin, it doesn’t mess up the rest of the code.

I disagree. If somebody writes new plugin, he/she is free to distribute it as “3rd party”. If we accept it to our source tree, it should have some degree of quality

I’m not saying a new plugin should get no review. What I’m trying to say (if not clearly) is that we clearly need to have some levels of review. New code in our source tree needs to have some degree of quality. New code in libavogadro needs to be high quality, careful not to break ABI, well-commented, etc. These changes need more scrutiny than a new plugin or modifications to an existing plugin.

Is that clearer?

Yes.

And new plugins need to be segfault-free (as possible) and not confuse users :slight_smile:


Regards,
Konstantin

I propose to alias code review (http://gold.cryos.net:8080) to subdomain like review.avogadro.openmolecules.net (inspired by https://review.source.android.com/).
Also it would be good to add some logo or textual identification of Avogadro and link to main page


Regards,
Konstantin

On Wed, Aug 18, 2010 at 2:21 AM, Konstantin Tokarev annulen@yandex.ruwrote:

I propose to alias code review (http://gold.cryos.net:8080) to subdomain
like review.avogadro.openmolecules.net (inspired by
https://review.source.android.com/).
Also it would be good to add some logo or textual identification of
Avogadro and link to main page

I think there is already an alias there, I need to set the canonical URL
but whenever I do that and you log in it makes new accounts (same name/email

  • new user). I was hoping to find a solution to this issue before moving, I
    wonder if the account linking would work. I am not totally sure how the open
    id stuff works, but I did expect the same open id login to create the same
    account no matter what the URL.

There is a config option to insert some custom HTML, and so that is doable.
This was a trial though, and so branding was bottom of the list until we
decided to keep it.

Marcus

Marcus D. Hanwell, Ph.D.
R&D Engineer, Kitware Inc.
(518) 881-4937

On Aug 18, 2010, at 9:48 AM, Marcus D. Hanwell wrote:

I think there is already an alias there,

There is an alias: http://review.avogadro.openmolecules.net:8080/ works.

What would be nice is to set up a redirect from review.avogadro.openmolecules.net/index.html to :8080.

I need to set the canonical URL but whenever I do that and you log in it makes new accounts (same name/email - new user). I was hoping to find a solution to this issue before moving, I wonder if the account linking would work. I am not totally sure how the open id stuff works, but I did expect the same open id login to create the same account no matter what the URL.

Maybe this is something to ask the Gerrit developers?

-Geoff

On Wednesday 18 August 2010 10:27:23 Geoffrey Hutchison wrote:

On Aug 18, 2010, at 9:48 AM, Marcus D. Hanwell wrote:

I think there is already an alias there,

There is an alias: http://review.avogadro.openmolecules.net:8080/ works.

What would be nice is to set up a redirect from
review.avogadro.openmolecules.net/index.html to :8080.

I changed the canonical URL, and did a little work on the database. Things
look OK, but let me know if you have trouble. When I have a little time I will
put Gerrit behind an Apache reverse proxy so that it can serve its content on
port 80. This is not very difficult, but I didn’t see the point if we were not
keeping it.

I need to set the canonical URL but whenever I do that and you log in it
makes new accounts (same name/email - new user). I was hoping to find a
solution to this issue before moving, I wonder if the account linking
would work. I am not totally sure how the open id stuff works, but I did
expect the same open id login to create the same account no matter what
the URL.

Maybe this is something to ask the Gerrit developers?

I did, known problem and they gave me a few pointers. It is done now.

Thanks,

Marcus

Marcus D. Hanwell, Ph.D.
R&D Engineer, Kitware Inc.
(518) 881-4937