First of all, thanks to everyone for the helpful replies.
> 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.
You're absolutely right. I believe this is a social problem in our
organization. Those in charge treat everything with a militaristic rigidity
and at this point are absolutely unwilling to give up on the idea that
everything must be code reviewed before being (currently manually) merged
with the main development codebase. Adding to the pain for developers is a
4 hour turnaround time and a ridiculously strict set of standards (i.e. if
you have an extra space in your code, it will get kicked back from review).
But the point is, this code review policy is the excuse for not having
Subversion in place. At this point I'm just trying to come up with an
irrefutable setup using SVN that will be more productive than what we have
currently.
> 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.
>
I see, so you are suggesting that you basically get with the reviewer
one-on-one before the commit is made, perhaps at the desk of the developer,
to look at the changes, and then after everything is approved, make the
commit. Unfortunately, at this point, I think the social aspect is at a
point where those in charge will not "allow" the code-reviewers to meet at
the developers desk to look over the code, and instead the reviewer must
review the code independently at his own workstation, then return the
results to the developer. At which point, changes are made, and
resubmitted, and the 4 hour clock starts ticking again.
>
> 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.
>
This is a great idea and one I will bring up when I go to fight the good
fight. Sadly, at this time, not even typos can sneak by the 4 hour review
turnaround.
>
>
> > 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.
>
Understood. But compared to what we have now it sounds like a weight of
developers' shoulders already. Russ suggested earlier different branches
based on features / tickets, etc., but with the amount of projects we have
running through here, I think they would argue that this would get far too
cumbersome very quickly (not that I agree, just looking to prepare myself
here). Also, others have suggested setting up a branch before a launch and
taking care of everything there before commit. The thing is, here, we do
sort of a piecemeal deployment, each small project at a time, so bundling
everything together into a new branch for "Version 1.6" or something would
be shot down I'm sure. I'm thinking continuous integration is the solution
to this, but first thing first and I'm just trying to get Subversion in
place as priority number one.
Also, Les made a comment: "Just because someone commits a change to the
trunk doesn't mean you have to use it in production." What you say is true,
of course, but is there a good way to keep track of what should and
shouldn't be part of a deployment when there are production ready and non
production changes being committed to the same trunk? Or are you just
suggesting that another committ can follow right behind to clean it up?
What I'm getting at is, right now we manually (with an inhouse tool) keep
track of what should and shouldn't be deployed by means of creating a
"depoyment script" which says, ok, methodA, methodB, methodC, and viewA,
viewB, viewC, and imageA, should be part of this deployment, and nothing
else. I think the other argument I will get is "Using Subversion, with all
the projects we have going on simultaneously, how are we going to keep track
of what items should be part of each deployment?"
> 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.
I'm not sure I follow here. They mail and later apply the patch? What do
you mean by this?
> 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.
Another great idea, thank you.
>
>
> 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.
>
Very true. Thanks to everyone for their thoughts, you guys have given me
some good fuel.
Best,
Brian
>
> --
> Thanks
>
> Mark Phippard
> http://markphip.blogspot.com/
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=1065&dsMessageId=2366468
To unsubscribe from this discussion, e-mail: [users-unsubscribe_at_subversion.tigris.org].
Received on 2009-06-29 19:21:56 CEST