Re: SVN with code review workflow

Reading over some of this I think there is a big point that needs to be made - what is it that the review process cares about? and what is it that a version control system cares about?
The two are not fully mutually exclusive, but not fully mutually inclusive either.

Your review process cares about the change that is being made to the product.
The version control system, however, cares about every little step made to make that change.

So think about it this way:

You have your product in the repository (say http://svn/product_name, so use another example) with its own trunk (e.g. http://svn/product_name/trunk).
To allow your developers to work, you make a copy (aka branch) of the product (e.g. http://svn/product_name/branches/developer/change-A) - it doesn't matter whether this branch is a 'feature' branch or a developer branch; the only thing that matters it that it is going to capture the changes made for 'change-A' - it might be for a bug request, feature enhancement, or general development.

The key to remember is that the developer makes many changes in the process of making that change that will be reviewed. They might create add 200 lines of code, modify 400 lines of code, and then delete 300 lines of code. The question comes - how, then, do you track it? Your review process doesn't really care that 200 lines were added, 400 lines were modified, and 300 lines removed; it really only cares about the net changes - 100 lines of code removed and 400 lines modified. However, the developer may very well care about how they went about those changes, and adding version control will give them the ability to manage those changes. Further, having a single version control system will allow the company to backup their work between the changes.

Okay - time for an example. One job I worked at did not have any version control in place. (No reviews either.) The original developer would make his changes to the code base and when he was satisfied copy the directory as a marker. Management didn't care that there was no version control system either. (sadly) So when I started, they wanted me to do the same. After a little work, I installed TSVN on my system, and checked in everything to a local repository on my laptop, and then exported to make the copies they wanted. This gave me the ability to version my changes - and track what I did - in an easy to use manner with full version control, but outside of management. If anything happened to my laptop, then all changes were lost - the whole history was lost. I later convinced them to give me a system to host SVN on, and converted my local repository into a hosted repository where others could gain access and we could do daily and monthly backups.

Fact is, your developers that care about version control are going to have a version control system installed on their own development systems - whether it is SVN, CVS, git, or whatever else they can get their hands on. As an organization, you will not be able to capture those changes in the history, nor make backups. Developers do care about all changes as they'll be able to undo things in their working copy a lot easier with version control than without when they find that a change did not work. Version control - any version control - makes this far easier, and I'd probably argue SVN makes it among the easiest. (There's probably a good sized group that would argue that git makes it easier yet - I don't know - haven't used it; but that's a different topic altogether.)

So the organization is faced with the question: What is it that we care about? Capturing the history of changes to the product and every step of the way? Or capturing only the major changes? Backuping up the developers work and aiding them in the development process? Or making the development process a non-trivial task?

A smart manager will recognize that version control is an aid to help the developer AND the organization, and the permissions and layout of the repository aid the process the organization wants to use.
So, based on what I've read in this conversionation, you'd probably have a great mix by doing a 'trunk' for each 'product' (however granular you want to make it) and then splitting off a branch for each developer to use for a given development task. The developer can then track their own changes throughout their development process, and when they're done it can be turned over for review and integration back into the trunk. The review process would have previously only seen the change set comprising the beginning (r0) and end (rN) of the branch; now you will also get to see r1 to r(N-1) as well in the history. Further, if the developer needs a change from trunk to complete their branch, then they can reintegrate trunk into their branch as well (if they had read permissions to trunk). This can all be managed by permissions - only reviewers get read/write permissions to trunk; everyone else gets at best read-only or even no permissions. (If a developer
 has no permissions, then they won't be able to even see the contents of trunk.) Developers would have read/write permissions to the specific branch they are working on/under - it might get a bit tedious to always be granting/denying permissions, so it might be wise to give the developer a general workspace to which their branches are placed under for work.

Further, permissions would also allow you to assign a group of developers to work on a single task - and they could branch/integrate between themselves as well if the repository supported it.

So in the end, version control gives you tracking of all changes, you can still have your review process, and you can get a more collaborative development environment that is also more developer friendly in the process with a code-base that has its full history and can be backed up as desired by the organization.

HTH,

Ben

________________________________
From: Brian FitzGerald <bmfitzgerald3_at_gmail.com>
To: Mark Phippard <markphip_at_gmail.com>
Cc: users_at_subversion.tigris.org
Sent: Monday, June 29, 2009 1:20:49 PM
Subject: Re: SVN with code review workflow

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=2366702

To unsubscribe from this discussion, e-mail: [users-unsubscribe_at_subversion.tigris.org].
Received on 2009-06-30 15:14:00 CEST