Excellent points, Hari. Thank you for your insight. That trust is
certainly key, and is something that is severely lacking at this point. I
will certainly look to put a tactful spin on the proposed changes, as you
suggested.
Thanks again for all the feedback, I should be able to put together a strong
case with the additional insight from each of you.
Kind Regards,
Brian
On Mon, Jun 29, 2009 at 2:22 PM, Hari Kodungallur <hkodungallur_at_gmail.com>wrote:
>
>
> 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=2366493
To unsubscribe from this discussion, e-mail: [users-unsubscribe_at_subversion.tigris.org].
Received on 2009-06-29 20:33:06 CEST