Patches on a centralized VCS in a small team
Apr 30 2008 My team is always looking for ways to improve on quality and one of the proposed ideas this year was to follow a model similar to large open source projects where basically every checkin is first proposed as a diff/patch and then vetted into the code base AFTER review.
Our current setup is a single large subversion repository where our team (~14 developers) all checkout the entire mainline branch we're working on and make commits to mainline as desired. Bugfixes are typically checked into mainline, then merged as requested by QA into the appropriate build. Once checked in the bug is updated in our bugtracker and handed over to another developer for code review.
But of course, the code is now already checked in, and we're in a state of flux until the second developer gets around to reviewing the code. If problems are found (more often than you might expect) then subsequent checkins are made and not only is mainline in a state of flux (what happens if QA branches here?) but we also have lost the nice clean atomic nature of a single checkin. Merges get harder and tracking the code associated with a bug is just a tiny bit more overhead.
I also believe that knowing a checkin has already been committed does something mentally to the reviewer. It's no longer as easy to request changes that might seem superficial or strictly style based. In the back of your mind as the reviewer you know that your comments will introduce a messy string of commits that you unconsciously weigh against the importance of your review.
Our QA lead has often suggested a solution to this problem where code reviews must be done in person and before the checkin. As much as I can see the improvement this would have on quality I personally have issues with the frequent interruptions and delays this would add to the process. Not to mention the challenge of working from home and/or late. Not a good idea.
So patches it is. I managed to convince my team leads that this was worthy of an experiment and so we chose three people out of a hat and for two weeks it was patches and only patches for all bug fixes. For those two weeks we would use TortoiseSVN to create patches of our changes and attach those patches to bugs in BugTracker. A reviewer would examine the diff rather than a subversion commit log and the process would repeat until the developer got it right.
I liked the process, but overall it just didn't really fly with the test group. Here are a sampling of some of the reasons :
What's interesting is that in retrospect I am having a hard time seeing what the real issue here was. I believe that with diff being as standard as it is and the tool support available out there that almost all of these issues can be solved with some infrastructure and automation around the process. As it is the resistance to the idea and my unwillingness to force unnecessary process has killed this effort for now.
Given the size of our team and the fact we are all working with fast direct access to subversion it's hard to justify needing to work with patch files. Particularly since "revert changes from this revision" is so easy to do.
I do think this is one of those things worth pushing again, I may just have to do the legwork to smooth the process and prove the utility before getting everyone on board.
Our current setup is a single large subversion repository where our team (~14 developers) all checkout the entire mainline branch we're working on and make commits to mainline as desired. Bugfixes are typically checked into mainline, then merged as requested by QA into the appropriate build. Once checked in the bug is updated in our bugtracker and handed over to another developer for code review.
But of course, the code is now already checked in, and we're in a state of flux until the second developer gets around to reviewing the code. If problems are found (more often than you might expect) then subsequent checkins are made and not only is mainline in a state of flux (what happens if QA branches here?) but we also have lost the nice clean atomic nature of a single checkin. Merges get harder and tracking the code associated with a bug is just a tiny bit more overhead.
I also believe that knowing a checkin has already been committed does something mentally to the reviewer. It's no longer as easy to request changes that might seem superficial or strictly style based. In the back of your mind as the reviewer you know that your comments will introduce a messy string of commits that you unconsciously weigh against the importance of your review.
Our QA lead has often suggested a solution to this problem where code reviews must be done in person and before the checkin. As much as I can see the improvement this would have on quality I personally have issues with the frequent interruptions and delays this would add to the process. Not to mention the challenge of working from home and/or late. Not a good idea.
So patches it is. I managed to convince my team leads that this was worthy of an experiment and so we chose three people out of a hat and for two weeks it was patches and only patches for all bug fixes. For those two weeks we would use TortoiseSVN to create patches of our changes and attach those patches to bugs in BugTracker. A reviewer would examine the diff rather than a subversion commit log and the process would repeat until the developer got it right.
I liked the process, but overall it just didn't really fly with the test group. Here are a sampling of some of the reasons :
- Complexity in managing multiple ongoing bugs at once
- Reviewing via diff lacked enough context, whereas reviewing a subversion log using TortoiseSVN actually let you see the entire files with highlighted differences
- Adding binary files seemingly couldn't be done in a patch (rare but annoying)
- Extra steps required are often slow on our repository because of size
What's interesting is that in retrospect I am having a hard time seeing what the real issue here was. I believe that with diff being as standard as it is and the tool support available out there that almost all of these issues can be solved with some infrastructure and automation around the process. As it is the resistance to the idea and my unwillingness to force unnecessary process has killed this effort for now.
Given the size of our team and the fact we are all working with fast direct access to subversion it's hard to justify needing to work with patch files. Particularly since "revert changes from this revision" is so easy to do.
I do think this is one of those things worth pushing again, I may just have to do the legwork to smooth the process and prove the utility before getting everyone on board.