The important thing to realize is that policies do not make good code. A good culture that encourages good code makes good code. Two companies I have worked for (market caps of over $5B each) simply had a culture that you'd e-mail diffs to the team before you hit commit. It wasn't even an official rule: in your first week you'd just see people doing it, and so you did it too. There was a mutual understanding of "we're in this together, and so we'll work as a team." As a result, the code quality was quite good, and the system was never abused.
Jason Malinowski
-----Original Message-----
From: Mark Phippard [mailto:markphip_at_gmail.com]
Sent: Monday, June 29, 2009 11:35 AM
To: Brian FitzGerald
Cc: users_at_subversion.tigris.org
Subject: Re: SVN with code review workflow
On Mon, Jun 29, 2009 at 11:38 AM, Brian
FitzGerald<bmfitzgerald3_at_gmail.com> wrote:
> Greetings all,
>
> I am a Web developer with a company who (rightfully so) places a large
> amount of importance on code review. For this reason, up to this
> point, they have not implemented a traditional version control system,
> such as Subversion.
>
> The concern of certain key people in the organization is that putting
> SVN in place would mean losing control of the code base in that
> developers could simply commit whatever, whenever they wanted, without
> going through the highly valued code review process.
Yes, code review is good. But I do not see the need to try to enforce
it so strongly. This can be treated as a social problem. If you want
all code to be reviewed before committing, then just make that policy.
Ask people to include information about the reviewers in the commit
message. Setup the post-commit hook so that all commits are sent to a
mailing list that all your developers see. If you see commits come
through without the review info, then ask why it is not there and
whether the review happened. Developers are not going to want to make
public mistakes again and again and they will get it right.
If you really want some enforcement, a pre-commit hook could reject
any commit that does not contain the "Reviewed by:" section of the
message. But surely there are some commits, such as typos etc. that
can be committed without review.
> I understand their concern, and I have a few ideas about what could be
> some potential solutions to our problem, but I wanted to get thoughts
> from others about ways they have seen this tackled.
>
> One solution I have heard of large shops using is to have each
> developer have their own branch in the repository, so that once their
> coding is complete, they would commit to their personal branch. Then,
> the code reviewer would perform a commit from the developers branch to
> the (and I get hazy about the specifics here) main development branch
> (would this need to be a separate repository?).
>
> At this point, the code reviewer could look at all the diffs between
> the code in the developers branch, and the code in the main
> development branch, and either reject or approve and commit the code.
> For updates, developers would grab from the main development
> repository (or branch), so that they would always be synced up with
> the changes committed (and approved) by other developers.
There is no question this works, but it is a very heavy process to put
in place just for code review. If that is your only motivation, there
are better ways, IMO, such as handling it as mentioned above.
> I have also heard of tools such as Codestriker
> http://codestriker.sourceforge.net/,
>
> which apparently integrate with SVN and may be valuable in this
> process as well.
>
> What solutions have you all seen in action? Thanks in advance for any
> other thoughts on the topic.
I've used Review Board
http://www.review-board.org/
It is nice and works well with SVN. I think the best approach is the
way open-source projects like SVN handle it. Developers use svn diff
to create a patch with their changes and mail it, along with the
commit message to a mailing list. Developers can apply the patch and
reply to the email for the review. When you commit your changes, you
note in the commit message who reviewed it. In addition, all commits
are also sent to a mailing list. This allows developers to do
post-commit reviews when they notice something that does not look
right. After all, they may not have been the developer asked to do
the original review, so this is the first time they saw it.
Post-commit reviews can be just as effective as pre-commit, but you
need a team that is committed to doing them.
Finally, in open-source world if committers do not follow the project
standards you use social pressures to get them to comply and if that
fails you simply revoke the commit privs. This works in corporate
settings too. If a developer cannot commit, they will have to send
all their patches to someone else to review and commit. They'll
learn.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=1065&dsMessageId=2366451
To unsubscribe from this discussion, e-mail: [users-unsubscribe_at_subversion.tigris.org].
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=1065&dsMessageId=2366457
To unsubscribe from this discussion, e-mail: [users-unsubscribe_at_subversion.tigris.org].
Received on 2009-06-29 18:49:10 CEST