Milan Jurik writes: > > I would do this for efficiency reasons. I don't want to pester the > > same list of code reviewers N separate times for each of N CRs. I > > How did you minimize code review by including several CRs in one webrev? > They need to check the same amount of changes. Are you sure it's easier > to review several intermixed changes?
I'm resisting the strong urge I have to illustrate the point by responding to that question by sending multiple email messages, with one word in each message. ;-} There's an amount of overhead involved with requesting a code review. One must gather and post the materials, along with any supporting reference information. The reviewer must stop whatever he's working on, examine the changes, check that there aren't other things that need to be changed or related problems, verify that the code matches design and/or CR evaluation, and so on. The RE must then gather the code review comments, analyze the responses, devise new fixes (if appropriate), discuss with the reviewer, and so on. It's not a simple "please check this box when done reviewing" sort of thing -- at least if the review is competent at all. It *might* be possible in some cases to send a big pile of webrev links, each for an individual CR's diffs, in order to amortize some of the cost. But then the recipient has a hard time getting the *whole* picture. If there are unexpected interactions among the CRs, then the reviewer's job just became much harder, and in some cases perhaps impossible to do right. I don't think there's a single "right" answer here such that we can really say "don't bundle changes." > > to keep these changesets separate all the way up to a "push." > > > > As I wrote if CRs are splitted in suggested fix (bad that it isn't > available on bugs.opensolaris.org) only regression hunting remains > complicated a bit. So this policy should be enforced where possible. It's only _slightly_ harder. Look at the 'hg history' output for the file of interest. Ignore CRs that are obviously unrelated. Examine the ones that are. I can see that it might involve a little more effort in some cases, I'm just not buying the story that this is so much worse than the previous situation that we must institute new policies that will in fact cause yet more problems. > > More importantly, *not* doing this sort of bundling means that > > low-priority (but annoying and customer-dissatisfaction-generating) > > CRs are simply never addressed. Ever. They rot in the database, and > > the code rots as a result. > > > > So maybe we should improve QA and RTI to decrease amount of work spent > there? As long as human review is involved, I think there's a certain amount of irreducible cost. Yes, it'd be great to chase after that dream, but many of us have to live with the reality that some kinds of tests take days or weeks or even longer to complete, and that good RTI (and other) reviews are lengthy steps. > > You're probably right that bundling together (say) "update Perl to > > version 1234" and "fix TCP retransmit bug" would be foolish. I don't > > think anyone's going to do something like that, though, and thus my > > previous comment. "Prohibit" is just too strong. Heck, even > > discouraging bundling might be wrong, as it encourages poor behavior, > > and allows the system to rot over time. > > > > In your last 2 paragraphs you are saying that bundling CRs in one > changeset is OK because it workarounds procedural pain, aren't you? Essentially, yes. -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
