Greg Stein <gstein@lyra.org> writes:
> Two alternatives were present:
>
> 1) check in some work that is still small enough to be reviewable, but the
> tests will be broken until part two arrives. when that does, it will be
> reviewable, too.
>
> 2) hold off, continue merging other work, then drop in an unreviewable
> widespread change to svn. "but the tests work!"
Er, -1.
Those weren't the only choices here.
Reviewing has nothing to do with committing. Mike can post a diff to
get review on changes in progress. I've done that before, and
profited from the resulting review, all without affecting other
people's working copies before the change is ready.
Also, you're asserting that the difference between Mike's changes in 1
(where the tests are still broken) and 2 (where the tests pass) is so
large as to make one reviewable and the other unreviewable. There's
no basis for this assertion. The code change doesn't get "larger" or
less reviewable as one fixes bugs and makes tests pass -- sometimes
quite the opposite.
(The change to the *test code* might be larger, of course, but that's
a separate module and doesn't affect the reviewability of the real
code change).
> Relying on testing _rather_than_review_ is an incorrect pattern of behavior.
Sure. No one is proposing relying on tests rather than review. Both
are important.
> When reviewing, the brain looks a TON more stuff than a test does. And at
> some point, those tests will return, and we'll get a second pass on the
> work. It's just time-shifted is all.
Yes, and during that time, all other developers' work is interfered
with, and possibly they are in danger of committing with a binary that
will corrupt their working copies (see commits 1625-1630!).
> In this case, Mike's checkin gives us something that is still somewhat
> grokkable, so it can be reviewed.
A commit email is just a patch and a log message; therefore a posted
patch with log message is just as grokkable as a commit, but *without*
the other inconveniences.
I don't see what was gained by committing this sooner, that couldn't
have been gained without committing code *known* to be unready.
As the number of developers grows, the inconvenience of breaking the
tests grows too. There was no need to do it here, and we shouldn't do
it in the future, at least not without a better reason.
I'd like some discussion on this, if there is disagreement. Although
we may squeak by without serious inconvenience in this case, we might
not be so lucky next time. The "changes should pass 'make check'"
policy helps us in very concrete ways, and it seems to me that we
tossed that policy entirely without need in this instance. I *don't*
want to make a habit of that. :-(
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Apr 4 19:20:22 2002