On Mon, Jun 29, 2009 at 10:20 AM, Brian FitzGerald
<bmfitzgerald3_at_gmail.com>wrote:
> 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.
>
>
I agree with almost all the responses. Let me, however, add one more thing.
One can only take the process so far with the addition of svn and any other
workflow. It will really start working for you only when whoever is 'in
control' starts to trust the developers and also the people who take care of
the process. Perhaps you want to demo it in such a way that this new process
was always 'their' idea :-)
At my first work place in the US, my manager had told me (paraphrasing): "In
the US, business runs on trust. Watchign CNBC, you might think that the only
thing that matters is the 'bottom-line'. While that is mosty true, the
businesses that run well achieve the 'bottom-line' by trusting the
employees". I have not yet seen an instance where that statement was proven
wrong.
thanks,
-Hari
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=1065&dsMessageId=2366490
To unsubscribe from this discussion, e-mail: [users-unsubscribe_at_subversion.tigris.org].
Received on 2009-06-29 20:23:20 CEST